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

Jump to bottom

Allow AccessibleByteArrayOutputStream to shrink #346

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow AccessibleByteArrayOutputStream to shrink #346

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ea2de3
Allow AccessibleByteArrayOutputStream to shrink
olivergillespie Nov 3, 2023
d23cfe4
Allow different buffer shrink strategy implementations
olivergillespie Nov 9, 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
26 changes: 22 additions & 4 deletions src/com/amazon/corretto/crypto/provider/AccessibleByteArrayOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
import java.util.Arrays;

class AccessibleByteArrayOutputStream extends OutputStream implements Cloneable {
private static final int MAX_OVERSIZED_THRESHOLD = 1024;
private int timesOversized;
private int timesOversizedThreshold;
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the initial value of the threshold? These need to be initialized somewhere.

Copy link
Author

Choose a reason for hiding this comment

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

Here it would start at 0, which is fine by me, but also any small value would work (1, 2, 4).

Copy link
Author

Choose a reason for hiding this comment

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

Actually 0 is a mistake here, since I'm doubling it to grow it. It must have been set to 1 in my testing and somehow I messed it up when committing. I'll fix it if we decide to use it.


private final int limit;
private byte[] buf;
private int count;
Expand Down Expand Up @@ -83,11 +87,22 @@ byte[] getDataBuffer() {
}

void reset() {
Arrays.fill(buf, 0, count, (byte) 0);
int sizeUsed = count;
Arrays.fill(buf, 0, sizeUsed, (byte) 0);
geedo0 marked this conversation as resolved.
Show resolved Hide resolved
count = 0;
// TODO: Consider keeping track of length at reset.
// If it is consistently below the maximum value we may want to trim
// down to save on memory.

// Consider shrinking the buffer.
if (sizeUsed * 2 < buf.length) {
// The buffer was over-sized for this usage.
if (timesOversized++ > timesOversizedThreshold) {
// Shrink the buffer.
buf = new byte[buf.length / 2];
timesOversized = 0;
}
} else {
// Buffer was not over-sized, reset counter.
timesOversized = 0;
}
}

void write(final ByteBuffer bbuff) {
Expand Down Expand Up @@ -122,5 +137,8 @@ private void growCapacity(final int newCapacity, final boolean doNotAllocateMore
final byte[] toZeroize = buf;
buf = tmp;
Arrays.fill(toZeroize, 0, count, (byte) 0);

// Every time we need to grow, make it harder to shrink in the future (up to a limit).
timesOversizedThreshold = Math.min(MAX_OVERSIZED_THRESHOLD, timesOversized * 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following the logic here on how this threshold adapts to our workload. Seems like it only ever gets harder over time. That can make sense depending on the call pattern, but for something like ACCP we have to be extremely general purpose and assume nothing about those patterns.

Copy link
Author

Choose a reason for hiding this comment

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

It's true that it only gets harder over time. So really the only difference between workloads is how fast it tends to the limit, and how far it gets. My expectation is that some workloads will never reach it, some will reach it slowly and some will reach it so quickly that they might as well have started there. I agree it needs a bit more exploration, I'll add some thoughts to your top-level comment.

Copy link
Author

Choose a reason for hiding this comment

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

There's a bug here too - timesOversized should be timesOversizedThreshold. I had that fixed locally, I must have missed committing my last few changes, sorry.

}
}