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.

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

Avoid allocation in BigDecimal serializer with alternative way to write bytes. #1016

Closed
wants to merge 4 commits into from
Closed

Avoid allocation in BigDecimal serializer with alternative way to write bytes. #1016

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62bcd2d
Avoid allocation in `BigDecimal` serializer, new method for bytes in …
Oct 14, 2023
029190b
Improve names, tests, checks for writing and reading bytes of a long.
Oct 17, 2023
9698899
Add to Input/Output methods for writing and reading bytes of an int.
Oct 17, 2023
813ae4b
Optimize writing and reading bytes of a long/int by unrolling loop.
Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/com/esotericsoftware/kryo/io/ByteBufferInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,16 @@ public void readBytes (byte[] bytes, int offset, int count) throws KryoException
}
}

public long readBytesAsLong (int count) {
Copy link
Member

@NathanSweet NathanSweet Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is better than my initial idea of readBytes2, readBytes3, etc. Nice!

I don't hate the name readBytesAsLong, but what if it was readLong(int count)? Should we have a readInt(int count) for 2-3 bytes?

The first line of the method should be:

if (count < 0 || count > 8) throw new IllegalArgumentException("count must be >= 0 and <= 8: " + count);

Same with other methods taking a count parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 029190b, and the readInt(int count) in 9698899.

require(count);
position += count;
long bytes = byteBuffer.get();
for (int i = 1; i < count; i++) {
bytes = (bytes << 8) | (byteBuffer.get() & 0xFF);
}
return bytes;
}

// int:

public int readInt () throws KryoException {
Expand Down
8 changes: 8 additions & 0 deletions src/com/esotericsoftware/kryo/io/ByteBufferOutput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,14 @@ public void writeBytes (byte[] bytes, int offset, int count) throws KryoExceptio
}
}

public void writeBytesFromLong (long bytes, int count) {
Copy link
Member

Choose a reason for hiding this comment

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

writeLong(long bytes, int count)? Should we also have writeInt(int bytes, int count)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed to writeLong(long bytes, int count) and added writeInt(int bytes, int count). I wonder if those methods should stay where they are in the Output class, i.e. in the section marked as // byte:, or should I move them to sections // long: and // int: correspondingly (and to the same in Input class)?

require(count);
position += count;
for (int i = count - 1; i >= 0; i--) {
byteBuffer.put((byte) (bytes >> (i << 3)));
}
}

// int:

public void writeInt (int value) throws KryoException {
Expand Down
12 changes: 12 additions & 0 deletions src/com/esotericsoftware/kryo/io/Input.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,18 @@ public void readBytes (byte[] bytes, int offset, int count) throws KryoException
}
}

/** Reads count bytes and returns them as long, the last byte read will be the lowest byte in the long. */
public long readBytesAsLong (int count) {
require(count);
int p = position;
position = p + count;
long bytes = buffer[p++];
for (int i = 1; i < count; i++) {
bytes = (bytes << 8) | (buffer[p++] & 0xFF);
}
return bytes;
}

// int:

/** Reads a 4 byte int. */
Expand Down
12 changes: 11 additions & 1 deletion src/com/esotericsoftware/kryo/io/Output.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
package com.esotericsoftware.kryo.io;

import com.esotericsoftware.kryo.KryoException;
import com.esotericsoftware.kryo.io.KryoBufferOverflowException;
import com.esotericsoftware.kryo.util.Pool.Poolable;
import com.esotericsoftware.kryo.util.Util;

Expand Down Expand Up @@ -274,6 +273,17 @@ public void writeBytes (byte[] bytes, int offset, int count) throws KryoExceptio
}
}

/** Writes count bytes from long, the last byte written is the lowest byte from the long.
* Note the number of bytes is not written. */
public void writeBytesFromLong (long bytes, int count) {
require(count);
int p = position;
position = p + count;
for (int i = count - 1; i >= 0; i--) {
buffer[p++] = (byte) (bytes >> (i << 3));
Copy link
Member

Choose a reason for hiding this comment

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

Writing could be one less shift per iteration (untested):

for (int i = (count - 1) << 3; i >= 0; i -= 8)
	buffer[p++] = (byte)(bytes >> i);

Same for ByteBufferOutput.

Reading could match, like:

for (int i = (count - 1) << 3; i > 0; i -= 8)
	bytes |= (buffer[p++] & 0xFF) << i;

However, for reading what you have may be better: simpler initializer and similar operations per iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do that, but the read side got a bit more complex:

long bytes = buffer[p] >= 0 ? 0 : ~(-1L >>> ((8-count) << 3));
for (int i = (count - 1) << 3; i >= 0; i -= 8) {
	bytes |= (buffer[p++] & (long) 0xFF) << i;
}

And also much slower (while the write side stayed the same when it comes to speed):
new-loops

So in 813ae4b I did something different - I unrolled the loops manually replacing it with some switches and repeated code. This got the reading/writing a little bit faster:

unrolled-loops

}
}

// int:

/** Writes a 4 byte int. */
Expand Down
37 changes: 11 additions & 26 deletions src/com/esotericsoftware/kryo/serializers/DefaultSerializers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import static com.esotericsoftware.kryo.Kryo.*;
import static com.esotericsoftware.kryo.util.Util.*;
import static java.lang.Long.numberOfLeadingZeros;

import com.esotericsoftware.kryo.Kryo;
import com.esotericsoftware.kryo.KryoException;
Expand Down Expand Up @@ -282,27 +283,15 @@ public void write (Kryo kryo, Output output, BigDecimal object) {
}

// compatible with writing unscaled value represented as BigInteger's bytes
private static void writeUnscaledLong(Output output, long unscaledLong) {
if (unscaledLong >>> 7 == 0) { // optimize for tiny values
output.writeVarInt(2, true);
output.writeByte((byte) unscaledLong);
} else {
byte[] bytes = new byte[8];
int pos = 8;
do {
bytes[--pos] = (byte) (unscaledLong & 0xFF);
unscaledLong >>= 8;
} while (unscaledLong != 0 && unscaledLong != -1); // out of bits

if (((bytes[pos] ^ unscaledLong) & 0x80) != 0) {
// sign bit didn't fit in previous byte, need to add another byte
bytes[--pos] = (byte) unscaledLong;
}
private static void writeUnscaledLong (Output output, long unscaledLong) {
int insignificantBits = unscaledLong >= 0
? numberOfLeadingZeros(unscaledLong)
: numberOfLeadingZeros(~unscaledLong);
int significantBits = (64 - insignificantBits) + 1; // one more bit is for the sign
int length = (significantBits + (8 - 1)) >> 3; // how many bytes are needed (rounded up)

int length = 8 - pos;
output.writeVarInt(length + 1, true);
output.writeBytes(bytes, pos, length);
}
output.writeByte(length + 1);
output.writeBytesFromLong(unscaledLong, length);
}

public BigDecimal read (Kryo kryo, Input input, Class<? extends BigDecimal> type) {
Expand All @@ -313,15 +302,11 @@ public BigDecimal read (Kryo kryo, Input input, Class<? extends BigDecimal> type
if (length == NULL) return null;
length--;

byte[] bytes = input.readBytes(length);
if (length > 8) {
byte[] bytes = input.readBytes(length);
unscaledBig = new BigInteger(bytes);
} else {
unscaledLong = bytes[0];
for (int i = 1; i < bytes.length; i++) {
unscaledLong <<= 8;
unscaledLong |= (bytes[i] & 0xFF);
}
unscaledLong = input.readBytesAsLong(length);
}

int scale = input.readInt(false);
Expand Down