Skip to content
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

Remove the repeated logic of the method that overwrites the lower nibble of container type descriptor. #548

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

linlin-s
Copy link
Contributor

@linlin-s linlin-s commented Aug 9, 2023

Issue #, if available:
N/A

Description of changes:

This PR optimize the process of overwriting lower nibble of type descriptor by avoid unnecessary method calls:

index = index(position)
offset = offset(position)

Context:

While writing containers, if the container length is small enough to fit in the lower nibble of the type descriptor byte, we will overwrite the lower nibble of the type descriptor byte with encoded length.

Before change, we need to use method buffer.getUInt8At () to calculate the OCTET of the type descriptor then combine it with the length value and then write the value to buffer by using buffer.writeVarUInt8At(). Both of these methods including calculations index = index(position), offset = offset(position). In this PR, we will pass the length value directly tobuffer.writeVarUInt8At()to avoid repeated calculations of index and offset.

Here are the benchmark results before and after changes:

Results from benchmarking a write of data equivalent to a stream of 194617 nested binary Ion value(3 forks, 2 warmups, 2 iterations, preallocation 1).

Benchmark Before After Units
Bench.run 3919.464 3815.232 ms/op
Bench.run:Heap usage 3502.561 3311.067 MB
Bench.run:Serialized size 201.663 201.663 MB
Bench.run:·gc.alloc.rate 160.113 164.271 MB/sec
Bench.run:·gc.alloc.rate.norm 687893000.4 687857152.9 B/op
Bench.run:·gc.churn.G1_Eden_Space 15.839 20.501 MB/sec
Bench.run:·gc.churn.G1_Eden_Space.norm 68273948.44 85750215.11 B/op
Bench.run:·gc.churn.G1_Old_Gen 29.175 40.867 MB/sec
Bench.run:·gc.churn.G1_Old_Gen.norm 125954616.9 170904007.1 B/op
Bench.run:·gc.count 6 8 counts
Bench.run:·gc.time 216 233 ms

Results from benchmarking a write of data equivalent to a stream of 59155 nested binary Ion values(3 forks, 2 warmups, 2 iterations, preallocation 1).

Benchmark Before After Units
Bench.run 504.162 490.292 ms/op
Bench.run:Heap usage 353.553 367.848 MB
Bench.run:Serialized size 21.271 21.271 MB
Bench.run:·gc.alloc.rate 126.217 129.877 MB/sec
Bench.run:·gc.alloc.rate.norm 70042252.14 70052759.49 B/op
Bench.run:·gc.churn.G1_Eden_Space 7.497 7.652 MB/sec
Bench.run:·gc.churn.G1_Eden_Space.norm 4161015.873 4127727.746 B/op
Bench.run:·gc.churn.G1_Old_Gen 144.318 147.497 MB/sec
Bench.run:·gc.churn.G1_Old_Gen.norm 80097528.48 79557871.75 B/op
Bench.run:·gc.churn.G1_Survivor_Space 0.307 0.224 MB/sec
Bench.run:·gc.churn.G1_Survivor_Space.norm 171300.86 120510.984 B/op
Bench.run:·gc.count 90 106 counts
Bench.run:·gc.time 599 695 ms

Results from benchmarking a write of data equivalent to a single nested binary Ion value(3 forks, 2 warmups, 2 iterations, preallocation 1).

Benchmark Before After Units
Bench.run 0.087 0.082 ms/op
Bench.run:Heap usage 13.589 14.288 MB
Bench.run:Serialized size 0.005 0.005 MB
Bench.run:·gc.alloc.rate 90.841 96.427 MB/sec
Bench.run:·gc.alloc.rate.norm 8719.18 8697.858 B/op
Bench.run:·gc.churn.G1_Eden_Space 90.847 96.498 MB/sec
Bench.run:·gc.churn.G1_Eden_Space.norm 8719.42 8703.814 B/op
Bench.run:·gc.churn.G1_Survivor_Space 0.004 0.004 MB/sec
Bench.run:·gc.churn.G1_Survivor_Space.norm 0.385 0.342 B/op
Bench.run:·gc.count 443 473 counts
Bench.run:·gc.time 222 230 ms

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
...rc/com/amazon/ion/impl/bin/IonRawBinaryWriter.java 92.03% <100.00%> (-0.03%) ⬇️
src/com/amazon/ion/impl/bin/WriteBuffer.java 95.54% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!

// XXX we'll never overrun a block unless we're given a position past our block array
final Block block = blocks.get(index);
block.data[offset] = (byte) value;
long bitValue = block.data[offset];
if (value <= 0xD) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest creating a new method (something like 'writeLowerNibbleAt'), and call it for this purpose, instead of branching within this method.

Copy link
Contributor

@tgregg tgregg left a comment

Choose a reason for hiding this comment

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

Nice

@linlin-s linlin-s force-pushed the optimize-overwriterLowerTypeNibble branch from 336a846 to 0769dc0 Compare November 7, 2023 04:34
@linlin-s linlin-s merged commit 2f48f49 into master Nov 7, 2023
14 of 33 checks passed
@linlin-s linlin-s mentioned this pull request Nov 8, 2023
@linlin-s linlin-s deleted the optimize-overwriterLowerTypeNibble branch January 16, 2024 19:25
linlin-s added a commit that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants