-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use thread local for groupby raw key holders #5419
Conversation
646b149
to
1655c27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's figure out a way to not cache the huge map from expensive queries
...n/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java
Outdated
Show resolved
Hide resolved
if (longOverflow) { | ||
_globalGroupIdUpperBound = numGroupsLimit; | ||
_rawKeyHolder = new ArrayMapBasedHolder(_globalGroupIdUpperBound); | ||
if (!mapBasedRawKeyHolders.containsKey(ArrayMapBasedHolder.class.getName())) { | ||
mapBasedRawKeyHolders.put(ArrayMapBasedHolder.class.getName(), new ArrayMapBasedHolder(_globalGroupIdUpperBound).getInternal()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think initializing to _globalGroupIdUpperBound
got introduced in #5291. For many cases with multiple group by columns (high cardinality and/or MV columns) this number can be huge. Unclear to me if making this thread-local will protect against such cases that may require allocating huge chunk of memory upfornt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I feel we may need to have a range of groupIdBound, and only do thread local for them. if it's too small or too large, maybe just create new objects without and with initial size.
3b96ab5
to
3698af0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please resolve the conflict before checking this in. On my side, I have profiled two different use cases locally and verified that this fixes the hotspot issue for hash map init.
@fx19880617 Thank you for working this quickly!
3698af0
to
9584e4c
Compare
No description provided.