-
Notifications
You must be signed in to change notification settings - Fork 25.2k
reduce bytes used when serializing Extent #52549
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
Conversation
Pinging @elastic/es-analytics-geo (:Analytics/Geo) |
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 left a suggestion, otherwise LGTM.
negLeft = Math.toIntExact(posRight - input.readVLong()); | ||
posLeft = 0; | ||
negRight = 0; | ||
break; | ||
default: | ||
case ALL_SET: | ||
posRight = input.readVInt(); | ||
posLeft = Math.toIntExact(posRight - input.readVLong()); | ||
negRight = input.readInt(); | ||
negLeft = Math.toIntExact(negRight - input.readVLong()); |
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.
we should probably update ALL_SET
to have the same logic as POSITIVE_SET
and NEGATIVE_SET
combined?
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.
updated to what I think you meant
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.
Actually I was thinking of doing this:
posLeft = input.readVInt();
posRight = Math.toIntExact(input.readVLong() + posLeft);
negRight = -input.readVInt();
negLeft = Math.toIntExact(negRight - input.readVLong());
There is no reason to serialize posLeft/posRight differently in the POSITIVE_SET
case or in the ALL_SET
case? And likewise for negLeft/negRight in the NEGATIVE_SET
or ALL_SET
cases?
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.
my bad. I misunderstood. will update thanks
@jpountz thanks for the suggestions. I did a rough speed/size test of these changes and found the following before changes: writeSpeed: 172.679105 ns after changes: writeSpeed: 169.303268 ns |
@elasticmachine run elasticsearch-ci/1 |
This commit reflects comments made by Adrien in #50834 surrounding the Extent serialization. it re-orders and negates a few values in order to save more space
This commit reflects comments made by Adrien in #50834 surrounding the Extent serialization.
it re-orders and negates a few values in order to save more space