Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ private AccumulatingRead(
super(rangeSpec, retryContext, onCloseCallback);
this.readId = readId;
this.hasher =
(rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher))
(rangeSpec.begin() == 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

By removing the !(hasher instanceof Hasher.NoOpHasher) check, a CumulativeHasher is now always allocated and wrapped around the delegate hasher when rangeSpec.begin() == 0, even if checksum validation is disabled (i.e., when hasher is a NoOpHasher). Keeping this check avoids unnecessary object allocation and overhead when checksumming is disabled.

Suggested change
(rangeSpec.begin() == 0)
(rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher))
References
  1. Prefer lazy initialization over eager initialization for resource-intensive objects if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.

? new CumulativeHasher(hasher, 0, rangeSpec.maxLength())
: hasher;
this.complete = SettableApiFuture.create();
Expand Down Expand Up @@ -284,7 +284,7 @@ static class StreamingRead extends BaseObjectReadSessionStreamRead<ScatteringByt
super(rangeSpec, retryContext, onCloseCallback);
this.readId = new AtomicLong(readId);
this.hasher =
(rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher))
(rangeSpec.begin() == 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

By removing the !(hasher instanceof Hasher.NoOpHasher) check, a CumulativeHasher is now always allocated and wrapped around the delegate hasher when rangeSpec.begin() == 0, even if checksum validation is disabled (i.e., when hasher is a NoOpHasher). Keeping this check avoids unnecessary object allocation and overhead when checksumming is disabled.

Suggested change
(rangeSpec.begin() == 0)
(rangeSpec.begin() == 0 && !(hasher instanceof Hasher.NoOpHasher))
References
  1. Prefer lazy initialization over eager initialization for resource-intensive objects if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.

? new CumulativeHasher(hasher, 0, rangeSpec.maxLength())
: hasher;
this.closed = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ final class CumulativeHasher implements Hasher {
this.delegate = delegate;
this.startOffset = startOffset;
this.limit = limit;
this.cumulativeHash = Crc32cValue.zero();
this.cumulativeHash = delegate.initialValue();
}

@Override
Expand Down Expand Up @@ -123,16 +123,19 @@ boolean qualifiesForVerification(Object metadata) {
void validateCumulativeChecksum(Object metadata)
throws UncheckedCumulativeChecksumMismatchException {
if (qualifiesForVerification(metadata)) {
Crc32cValue<?> expected = Crc32cValue.of(metadata.getChecksums().getCrc32C());
Crc32cLengthKnown actual = getCumulativeHash();
if (actual == null) {
return;
}
Crc32cValue<?> expected = Crc32cValue.of(metadata.getChecksums().getCrc32C());
if (!actual.eqValue(expected)) {
throw new UncheckedCumulativeChecksumMismatchException(expected, actual);
}
}
}

private void accumulate(Crc32cLengthKnown actual) {
cumulativeHash = cumulativeHash.concat(actual);
cumulativeHash = nullSafeConcat(cumulativeHash, actual);
}

Crc32cLengthKnown getCumulativeHash() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ private ReadableByteChannelSessionBuilder(
this.read = read;
this.retrier = retrier;
this.resultRetryAlgorithm = resultRetryAlgorithm;
this.hasher = Hasher.defaultHasher();
this.hasher = Hasher.readHasher();
this.autoGzipDecompression = false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ final class GapicUnbufferedReadableByteChannel
this.read = read;
this.req = req;
this.hasher =
(req.getReadOffset() == 0 && !(hasher instanceof Hasher.NoOpHasher))
(req.getReadOffset() == 0)
? new CumulativeHasher(
hasher,
0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ protected LazyReadChannel<?, Object> newLazyReadChannel() {
ResumableMedia.gapic()
.read()
.byteChannel(read, retrier, resultRetryAlgorithm)
.setHasher(Hasher.defaultHasher())
.setHasher(Hasher.readHasher())
.setAutoGzipDecompression(autoGzipDecompression);
BufferHandle bufferHandle = getBufferHandle();
// because we're erasing the specific type of channel, we need to declare it here.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,27 @@ final class DefaultInstanceHolder {
}
}

final class ReadInstanceHolder {
private static final Logger LOGGER = Logger.getLogger(Hasher.class.getName());
private static final String PROPERTY_NAME = "com.google.cloud.storage.Hasher.read";
private static final String PROPERTY_VALUE =
System.getProperty(PROPERTY_NAME, DefaultInstanceHolder.PROPERTY_VALUE);
static final Hasher READ_HASHER;

static {
LOGGER.fine(String.format(Locale.US, "-D%s=%s", PROPERTY_NAME, PROPERTY_VALUE));
if ("disabled".equalsIgnoreCase(PROPERTY_VALUE)) {
READ_HASHER = noop();
} else {
READ_HASHER = enabled();
}
}
}

static Hasher readHasher() {
return ReadInstanceHolder.READ_HASHER;
}

@Nullable
default Crc32cLengthKnown hash(Supplier<ByteBuffer> b) {
return hash(b.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
@BetaApi
@Immutable
public final class ReadAsChannel extends BaseConfig<ScatteringByteChannel, StreamingRead> {
static final ReadAsChannel INSTANCE = new ReadAsChannel(RangeSpec.all(), Hasher.enabled());
static final ReadAsChannel INSTANCE = new ReadAsChannel(RangeSpec.all(), Hasher.readHasher());

private final RangeSpec range;
private final Hasher hasher;
Expand Down Expand Up @@ -97,7 +97,7 @@ public ReadAsChannel withRangeSpec(RangeSpec range) {
*/
@BetaApi
boolean getCrc32cValidationEnabled() {
return Hasher.enabled().equals(hasher);
return !Hasher.noop().equals(hasher);
}

/**
Expand All @@ -111,7 +111,7 @@ boolean getCrc32cValidationEnabled() {
*/
@BetaApi
ReadAsChannel withCrc32cValidationEnabled(boolean enabled) {
if (enabled && Hasher.enabled().equals(hasher)) {
if (enabled && !Hasher.noop().equals(hasher)) {
return this;
} else if (!enabled && Hasher.noop().equals(hasher)) {
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public final class ReadAsFutureByteString
extends BaseConfig<ApiFuture<DisposableByteString>, AccumulatingRead<DisposableByteString>> {

static final ReadAsFutureByteString INSTANCE =
new ReadAsFutureByteString(RangeSpec.all(), Hasher.enabled());
new ReadAsFutureByteString(RangeSpec.all(), Hasher.readHasher());

private final RangeSpec range;
private final Hasher hasher;
Expand Down Expand Up @@ -97,7 +97,7 @@ public ReadAsFutureByteString withRangeSpec(RangeSpec range) {
*/
@BetaApi
boolean getCrc32cValidationEnabled() {
return Hasher.enabled().equals(hasher);
return !Hasher.noop().equals(hasher);
}

/**
Expand All @@ -111,7 +111,7 @@ boolean getCrc32cValidationEnabled() {
*/
@BetaApi
ReadAsFutureByteString withCrc32cValidationEnabled(boolean enabled) {
if (enabled && Hasher.enabled().equals(hasher)) {
if (enabled && !Hasher.noop().equals(hasher)) {
return this;
} else if (!enabled && Hasher.noop().equals(hasher)) {
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public final class ReadAsFutureBytes
extends BaseConfig<ApiFuture<byte[]>, AccumulatingRead<byte[]>> {

static final ReadAsFutureBytes INSTANCE =
new ReadAsFutureBytes(RangeSpec.all(), Hasher.enabled());
new ReadAsFutureBytes(RangeSpec.all(), Hasher.readHasher());

private final RangeSpec range;
private final Hasher hasher;
Expand Down Expand Up @@ -92,7 +92,7 @@ public ReadAsFutureBytes withRangeSpec(RangeSpec range) {
*/
@BetaApi
boolean getCrc32cValidationEnabled() {
return Hasher.enabled().equals(hasher);
return !Hasher.noop().equals(hasher);
}

/**
Expand All @@ -106,7 +106,7 @@ boolean getCrc32cValidationEnabled() {
*/
@BetaApi
ReadAsFutureBytes withCrc32cValidationEnabled(boolean enabled) {
if (enabled && Hasher.enabled().equals(hasher)) {
if (enabled && !Hasher.noop().equals(hasher)) {
return this;
} else if (!enabled && Hasher.noop().equals(hasher)) {
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
public final class ReadAsSeekableChannel extends ReadProjectionConfig<SeekableByteChannel> {

static final ReadAsSeekableChannel INSTANCE =
new ReadAsSeekableChannel(Hasher.enabled(), LinearExponentialRangeSpecFunction.INSTANCE);
new ReadAsSeekableChannel(
Hasher.readHasher(), LinearExponentialRangeSpecFunction.INSTANCE);

private final Hasher hasher;
private final RangeSpecFunction rangeSpecFunction;
Expand Down Expand Up @@ -94,7 +95,7 @@ public ReadAsSeekableChannel withRangeSpecFunction(RangeSpecFunction rangeSpecFu
*/
@BetaApi
boolean getCrc32cValidationEnabled() {
return Hasher.enabled().equals(hasher);
return !Hasher.noop().equals(hasher);
}

/**
Expand All @@ -108,7 +109,7 @@ boolean getCrc32cValidationEnabled() {
*/
@BetaApi
ReadAsSeekableChannel withCrc32cValidationEnabled(boolean enabled) {
if (enabled && Hasher.enabled().equals(hasher)) {
if (enabled && !Hasher.noop().equals(hasher)) {
return this;
} else if (!enabled && Hasher.noop().equals(hasher)) {
return this;
Expand Down
Loading