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

BigDecimal serializer memory and throughput optimizations #1014

Merged
merged 4 commits into from Oct 13, 2023

Conversation

gdela
Copy link
Contributor

@gdela gdela commented Oct 11, 2023

This is an optimization of the default serializer for BigDecimal. Main goal is to avoid constructing BigDecimal in the inflated form during deserialization to save on memory and GC pressure of the program that did the deserialization. Avoiding constructing it with a BigInteger unscaled value is possible if the unscaled value fits in a long. More reasoning behind this can be found in my article: https://medium.com/@gdela/hidden-memory-effects-of-bigdecimal-caa0bfdb1e87

A nice side effect of this is improved throughput of serialization/deserialization for the cases where unscaled value fits in a long, i.e. if there are less than ~19 precision digits in the BigDecimal:

bigdecimal-optimiziation

The changes in the BigDecimalSerializer do not change how the serialized form of BigDecimal looks like. They are backwards-compatible.

Wojtek Gdela added 3 commits October 6, 2023 17:44
Main purpose is to avoid inflating `BigDecimal` during reads,
i.e. avoiding creation of `BigInteger` unscaled value as if it
is passed to `BigDecimal` constructor it makes this object bigger,
i.e. instead of 40 bytes it would be 104 bytes. Additionally now
the throughput of serialization/deserialization is better for some
values.
@NathanSweet
Copy link
Member

Nice PR and blog post! Very thorough. Reading the blog post I wondered if it was a naive benchmark or a proper one using JMH. I see the PR does use JMH.

Minor: newCustomBigInteger would be slightly clearer as newBigIntegerSubclass. Same with newCustomBigDecimal.

input.readBytes(length) allocates, as does the new byte[8]. If we're looking for places to micro-optimize, it'd be nice to avoid those allocations. For new byte[8] we need the length first. Maybe could use one of these:

long unscaledLong = 12131241241243L;
System.out.println((int)Math.ceil((64 - Long.numberOfLeadingZeros(unscaledLong)) / 8f));
System.out.println((int)((64 - Long.numberOfLeadingZeros(unscaledLong)) / 8f + 0.5f));

I'm not sure about that sign bit code though. To write the bytes after the length, it would be nice to be able to require and write to the Output byte[] directly, but it's protected, IIRC so subclasses have flexibility about how they write bytes. writeByte for each byte isn't ideal and maybe wrecks the whole optimization. Similarly we could read one byte at a time, but I'm not sure it'd be better.

Anyway, just some thoughts. The PR is great as is and we can merge if you don't feel like making any changes.

@gdela
Copy link
Contributor Author

gdela commented Oct 12, 2023

Thanks for comments, I'll try to improve the code and fix whatever caused the build to fail, and will come back later.

@NathanSweet NathanSweet merged commit c0cc9f9 into EsotericSoftware:master Oct 13, 2023
1 check passed
@NathanSweet
Copy link
Member

It looks good, thanks for the contribution!

Maybe we could add writeBytes methods for 1, 2, 3, ..., 8 parameters (to avoid varargs). Then we wouldn't need new byte[8] and similar for just a few bytes.

Similarly we could have readBytes2, readBytes3, and readBytes4 that return an int and readBytes5 through readBytes8 that return a long.

@gdela
Copy link
Contributor Author

gdela commented Oct 14, 2023

@NathanSweet I played a little bit more with this serializer following your suggestions. I prepared two options that may further improve the serialization/deserialization throughput #1015 or #1016.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants