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

[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

https://github.com/apache/ozone/blob/b3c84845926fb79e4b3b4ae20ded2be1c97ada06/hadoop-ozone/dist/src/shell/ozone/ozone#L89-L95

[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.