You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2024/09/19 18:13:01 UTC
[PR] only registers cleaner in native map when needed [accumulo]
keith-turner opened a new pull request, #4909:
URL: https://github.com/apache/accumulo/pull/4909
Registering cleaners takes up resources and currently bumps into a global lock. The native map code is registering a cleaner for iterators, even in the case where the native code did not allocate an iterator. This code is called very frequently by scan threads.
This change does not register the cleaner in the case where the native code did not allocate a native iterator. This should cause less contention between scan threads that are unrelated to each other.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] only registers cleaner in native map when needed [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #4909:
URL: https://github.com/apache/accumulo/pull/4909#discussion_r1767492112
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java:
##########
@@ -304,7 +304,13 @@ private class NMIterator implements Iterator<Map.Entry<Key,Value>> {
hasNext = nmiPointer != 0;
nmiPtr.set(nmiPointer);
- cleanableNMI = NativeMapCleanerUtil.deleteNMIterator(this, nmiPtr);
+ if (nmiPointer == 0) {
+ // registering a cleaner takes resources and may lock, so only bother registering if there
+ // is something to delete
+ cleanableNMI = null;
+ } else {
+ cleanableNMI = NativeMapCleanerUtil.deleteNMIterator(this, nmiPtr);
+ }
Review Comment:
Implementation looks fine, but I think it can be written more concisely as:
```suggestion
// avoid registering a cleanable if there's nothing to delete
cleanableNMI = hasNext ? NativeMapCleanerUtil.deleteNMIterator(this, nmiPtr) : null;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] only registers cleaner in native map when needed [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4909:
URL: https://github.com/apache/accumulo/pull/4909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] only registers cleaner in native map when needed [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4909:
URL: https://github.com/apache/accumulo/pull/4909#issuecomment-2361957461
Notice this while researching the lock contention seen in #4910.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org