HDDS-7228. ChecksumByteBufferImpl.update() is expensive

[jojochuang]

ChecksumByteBufferImpl.update() copies the bytebuffer before checksum calculation because the isReadOnly field of ByteBuffer is false.

This is a quick&dirty hack, which uses inflection to reset the isReadOnly field of ByteBuffer to true, so that ChecksumByteBufferImpl.update performs checksum calculation directly without memory copy.

Posting this as a draft. It's a dirty hack and I like it, and it does work without any visible overhead.

What changes were proposed in this pull request?

HDDS-7228

What is the link to the Apache JIRA

(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)

[kerneltime]

@duongkame

[dineshchitlangia]

@jojochuang Pretty hack! LGTM.

[jojochuang]

I'll still trying to come up with a better implementation. This is hacky.
And while this hack looked okay on a spindle disk cluster, unfortunately this hack still shows some overhead on a NVME cluster I tested.

[sodonnel]

I've marked this ready for review - any reason not to commit this? It may not be perfect, but its a lot better than what is there at the moment without this change.

[sodonnel]

Huum, some acceptance tests are failing with:

Put                                                                   | FAIL |
'WARNING: An illegal reflective access operation has occurred
--
Put with Streaming                                                    | FAIL |
'WARNING: An illegal reflective access operation has occurred
--
ozonefs-o3fs-link :: Ozone FS tests                                   | FAIL |
23 tests, 21 passed, 2 failed

Which I suspect is related

[sodonnel]

Various suggestions to suppress the warning - https://stackoverflow.com/questions/46454995/how-to-hide-warning-illegal-reflective-access-in-java-9-without-jvm-argument

I suspect it is the warning message that is breaking the acceptance tests, as they are likely expecting a certain output on stdout / stderr and this warning is breaking that expectation.

[adoroszlai]

I suspect it is the warning message that is breaking the acceptance tests, as they are likely expecting a certain output on stdout / stderr and this warning is breaking that expectation.

Yes, the test expects empty output for these commands.

This can be suppressed by:

• add --add-opens java.base/java.nio=ALL-UNNAMED to OZONE_MODULE_ACCESS_ARGS for Java9+
• add $OZONE_MODULE_ACCESS_ARGS to OZONE_SH_OPTS, OZONE_FS_OPTS, maybe some others, too

ozone/hadoop-ozone/dist/src/shell/ozone/ozone

Lines 89 to 95 in b3c8484

	# Extract the major version number
	JAVA_MAJOR_VERSION=$(echo "$JAVA_VERSION_STRING" | sed -E -n 's/.* version "([^.-]*).*"/\1/p' | cut -d' ' -f1)
	# populate JVM args based on java version
	if [[ "${JAVA_MAJOR_VERSION}" == "17" ]]; then
	OZONE_MODULE_ACCESS_ARGS="--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.management/com.sun.jmx.mbeanserver=ALL-UNNAMED --add-exports java.management/com.sun.jmx.mbeanserver=ALL-UNNAMED"
	fi

[adoroszlai]

I must be missing something here, but isn't buffer defined as readonly by us here:

ozone/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/ChecksumByteBuffer.java

Lines 42 to 43 in b3c8484

	default void update(byte[] b, int off, int len) {
	update(ByteBuffer.wrap(b, off, len).asReadOnlyBuffer());

Instead of using reflection, can't we simply remove the .asReadOnlyBuffer() call?

[adoroszlai]

I guess there are other places that are not so simple to change, e.g. this function used by computeChecksum / verifyChecksum:

ozone/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java

Lines 68 to 75 in b3c8484

	public static List<ByteBuffer> getReadOnlyByteBuffers(
	List<ByteString> byteStrings) {
	List<ByteBuffer> buffers = new ArrayList<>();
	for (ByteString byteString : byteStrings) {
	buffers.add(byteString.asReadOnlyByteBuffer());
	}
	return buffers;
	}

[sodonnel]

Its the protobuf that originates the buffer as readOnly. When the protobuf bytes are received, the data goes into a protobuf ByteString, which is backed by a byteArray. ByteString has some methods to return a ByteBuffer, one of which is "asReadOnlyByteBuffer". This wraps the original byte array and returns a readonly buffer. At that stage the Java checksum interface only accepts a byte or byteArray. Even the java.util.zip.CRC32 implementation has an implementation to take a ByteBuffer, but this is what it does:

public void update(ByteBuffer buffer) {
        int pos = buffer.position();
        int limit = buffer.limit();
        assert (pos <= limit);
        int rem = limit - pos;
        if (rem <= 0)
            return;
        if (buffer instanceof DirectBuffer) {
            crc = updateByteBuffer(crc, ((DirectBuffer)buffer).address(), pos, rem);
        } else if (buffer.hasArray()) {
            crc = updateBytes(crc, buffer.array(), pos + buffer.arrayOffset(), rem);
        } else {
            byte[] b = new byte[rem];
            buffer.get(b);
            crc = updateBytes(crc, b, 0, b.length);
        }
        buffer.position(limit);
    }

buffer.hasArray() checks for readOnly and returns false if it is readonly, meaning we cannot get at the array, and it will end up copying it.

[Cyrill]

@jojochuang Is there any estimation when this can be fixed. This change adds a significant boost to the Ozone performance and I wanted to continue investigating the performance further having this issue resolved.
Thanks.

[adoroszlai]

@jojochuang I've added the JVM arg to avoid the warning, plus changed initialization of the Field to static, please take a look if you're OK with that.

I think we should refine the printStackTrace() statements, too.

[sodonnel]

@jojochuang I've added the JVM arg to avoid the warning, plus changed initialization of the Field to static, please take a look if you're OK with that.

I think we should refine the printStackTrace() statements, too.

We should probably move these to a log message, rather than printing directly to stdout / stderr, which is what I assume e.printStackTrace() does. Otherwise I think we should commit this, as it greatly improves what is there performance wise.

[adoroszlai]

We should probably move these to a log message, rather than printing directly to stdout / stderr, which is what I assume e.printStackTrace() does.

Done.

[sodonnel]

LGTM