Skip to content

do not use Objects.hash in ExpressionContext or FunctionContext #8124

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.

Sign up for GitHub

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

Jump to bottom
Merged
siddharthteotia merged 1 commit into from
Feb 4, 2022
Merged

do not use Objects.hash in ExpressionContext or FunctionContext #8124

siddharthteotia merged 1 commit into from
Feb 4, 2022

Conversation

@richardstartin
Copy link
Member

@richardstartin richardstartin commented Feb 4, 2022 •
edited
Loading

I noticed very high allocation rate from using ExpressionContext as a HashMap key when profiling a benchmark:
Screenshot 2022-02-04 at 00 19 36

This is because Objects.hash boxes its arguments.

Computing the hash code manually reduces allocation in benchmark runs significantly:

before

Benchmark                                                                               (_intBaseValue)  (_numRows)  Mode  Cnt        Score          Error   Units
BenchmarkFilteredAggregations.testFilteredAggregations:·gc.alloc.rate.norm                            0     1500000  avgt    5   477029.513 ±     4265.923    B/op
BenchmarkFilteredAggregations.testNonFilteredAggregations:·gc.alloc.rate.norm                         0     1500000  avgt    5  1453348.554 ±    38671.676    B/op

after

Benchmark                                                                               (_intBaseValue)  (_numRows)  Mode  Cnt        Score          Error   Units
BenchmarkFilteredAggregations.testFilteredAggregations:·gc.alloc.rate.norm                            0     1500000  avgt    5   372573.234 ±   299663.490    B/op
BenchmarkFilteredAggregations.testNonFilteredAggregations:·gc.alloc.rate.norm                         0     1500000  avgt    5  1085451.680 ±  1751019.851    B/op

@richardstartin richardstartin changed the title do not use Arrays.hashCode in ExpressionContext or FunctionContext do not use Objects.hash in ExpressionContext or FunctionContext Feb 4, 2022
siddharthteotia
siddharthteotia reviewed Feb 4, 2022
View reviewed changes
pinot-common/src/main/java/org/apache/pinot/common/request/context/FunctionContext.java
@Override
public int hashCode() {
return Objects.hash(_type, _functionName, _arguments);
return 31 * 31 * _type.hashCode() + 31 * _functionName.hashCode() + _arguments.hashCode();
Copy link
Contributor

@siddharthteotia siddharthteotia Feb 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to see if arguments.hashCode() internally uses Objects.hash() and if that needs to be fixed too ?

Copy link
Member Author

@richardstartin richardstartin Feb 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately we don’t- the frames for ArrayList are in the flame graph above. After this change, that whole stack trace disappears from allocation profiles.

@siddharthteotia siddharthteotia merged commit af742f7 into apache:master Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddharthteotia siddharthteotia siddharthteotia approved these changes

@Jackie-Jiang Jackie-Jiang Awaiting requested review from Jackie-Jiang

Assignees

No one assigned

Labels

performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@richardstartin @siddharthteotia