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

Improves MessageBuffer support for Java 11 #514

Merged
merged 9 commits into from
Nov 18, 2019
Merged

Improves MessageBuffer support for Java 11 #514

merged 9 commits into from
Nov 18, 2019

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Sep 13, 2019 •
edited
Loading

Switches the MessageBuffer implementation to MessageBufferU for Java versions at least 9, which solves a couple of problems:

  1. On Java 11 (OpenJDK) jdk.internal.ref.Cleaner#clean is not open to reflection and the static initializer of DirectBufferAccess throws an IllegalAccessException.
  2. Access to java.nio.DirectByteBuffer may suffer the same problem in the future. For now we just get a warning.

Since MessageBufferU overrides almost all methods of MessageBuffer, while direct ByteBuffers register a cleaner by themself, I don't see why MessageBufferU(ByteBuffer) shouldn't accept direct buffers.

This should also solve bug number #482.

Piotr P. Karwasz and others added 2 commits September 13, 2019 21:09
Improves MessageBuffer support of Java 11
40fea89 
Switches the MessageBuffer implementation to MessageBufferU for Java
versions at least 9.

Since this implementation overrides almost all MessageBuffer's method it
is safe to allow direct buffers as argument to
MessageBuffer.wrap(ByteBuffer)
Corrects code style
89d6964
@ppkarwasz ppkarwasz changed the title Improves MessageBuffer support of Java 11 Improves MessageBuffer support for Java 11 Sep 14, 2019
@xerial
Copy link
Member

xerial commented Nov 8, 2019

@ppkarwasz Thanks for the suggestions. Unfortunately, however, using MessageBufferU for Java 9 or later cannot be our option as it will have a significant impact on the performance. We need to find a way to make DirectBufferAccess work on jdk9 or later.

Piotr P. Karwasz added 2 commits November 8, 2019 23:55
Do not switch to MessageBufferU on Java 9+
77a76a2 
Disables the automatic switch to MessageBufferU on Java 9+, falling back
to a manual switch through Java properties.
Run Java 11 tests on universal buffer only
f3b14bb 
Java 11 tests without the "msgpack.universal-buffer" property set where
using the universal buffer anyway:  Java 11's
"java.specification.version" does not contain a dot, so MessageBuffer
misidentified it as Java less than 7 and switched to MessageBufferU.
@ppkarwasz
Copy link
Contributor Author

I removed the automatic switch to MessageBufferU on Java 9+ and disabled Travis tests without the msgpack.universal-buffer property. Those tests where using MessageBufferU anyway, since the original code gave isJavaAtLeast7 == false on Java 11.

With these last changes this PR may still serve a purpose: the original code throws an exception, when we instantiate a MessageBufferU using a direct ByteBuffer. This behavior is, IMHO, too cautious. At least on Java 8+ direct ByteBuffers can clean up themselves so using direct buffers with MessageBufferU would not cause memory leaks.

In our use case the support for direct buffers is essential: we feed MessageUnpacker using netty (which almost always gives us direct buffers).

@ppkarwasz
Copy link
Contributor Author

We need to find a way to make DirectBufferAccess work on jdk9 or later.

I might adapt netty-common's solution to the Cleaner problem and provide a PR.

@xerial
Copy link
Member

xerial commented Nov 8, 2019

@ppkarwasz Yes. We also use netty or jetty-provided ByteBuffer with msgpack. What is netty-common's approach for supporting jdk9? If it's reasonable, I'd like to use it in this PR instead of ignoring jdk11 test for MessageBuffer.

@ppkarwasz
Copy link
Contributor Author

In Java 9+ sun.misc.Unsafe gets an invokeCleaner method which is accessible through reflection.

Piotr P. Karwasz added 5 commits November 10, 2019 14:20
Fixes DirectBufferAccess#clean on Java 11
55d8dd3 
For Java 9+ we switch from a DirectByteBuffer.cleaner().clean() call
to Unsafe.invokeCleaner(buffer).
Corrects style
293d928
Corrects whitespace
42ef575
Restores Java8 tests.
bd2e6f9
Fixes IllegalAccessExceptions
2860e9c 
Adds missing setAccessible calls.
@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Nov 10, 2019 •
edited
Loading

Done. Now it should work on both Java 8 and Java 11.

Summarizing, this PR:

  • it correctly identifies Java 9+ runtimes (they were identified as Java 6 before),
  • it extends MessageBufferU to support direct buffers,
  • adds direct buffer access on Java 9+ runtimes.

@xerial
Copy link
Member

xerial commented Nov 12, 2019

@ppkarwasz Thanks for your work. We will take over the remaining task (if necessary) as we also need to support Java11. (cc: @miniway)

@miniway
Copy link
Contributor

miniway commented Nov 14, 2019

It looks very good to me. Also we could encapsulate the Cleaner in a separate class in the future

@xerial
Copy link
Member

xerial commented Nov 18, 2019

It also looks good to me. Let me merge it and release a new version.

@xerial xerial merged commit bef0ccd into msgpack:develop Nov 18, 2019
@dbrgn dbrgn mentioned this pull request Nov 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xerial xerial xerial left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ppkarwasz @xerial @miniway