Improving I/O Efficience and Performance using NIO API instead of IO API #5341
Conversation
| public static void writeTo(InputStream in, OutputStream out) throws IOException { | ||
| ReaderWriter.writeTo(in, out); | ||
| in.transferTo(out); |
There was a problem hiding this comment.
In general, I am not in favor of replacing ReaderWriter.writeTo. While transferTo seems to do exactly the same as writeTo, Jersey has a mechanism to extend the buffer size used.
There was a problem hiding this comment.
In a JMH test campaign at OpenJDK we have empirically proven that it makes no sense for any application to use any other buffer sizes than the JRE's default size. In particular we have proven that it makes no sense to use buffers smaller than 8K (JRE's previous default) / larger than 16K (JRE's current default). Keeping that configuration option will in virtually all cases result in (very) worse efficience and performance compared to using transferTo. Hence I do not see what your target is by not using transferTo. Using transferTo is the sole key point of this PR, and is the sole proven driver for optimum efficience and performance. So if your decision is final, we can drop this PR -- but this means that Jersey will always have bad efficency and performance.
There was a problem hiding this comment.
BTW, transferTo is definitively not doing the same as Jersey's custom code in recent JREs. In fact, modern JREs offload the data transfer to the OS, which leads to dramatic performance and efficiency gains. My PR targets on moden JREs, not on JRE 11. That's why I opened it for 3.1, not for 3.x / master. Please check the actual source code of JRE 21-SNAPSHOT.
Edit: To understand where this happens, you need to see that each implementation of InputStream has its own implementation of transferTo (this is what we added in the past years since JRE 11). In particular, the optimization happens inside of channel-based input streams, e. g. the one returned by Files.newInputStream, which is working completely different to Jersey's.
There was a problem hiding this comment.
A compromise could be that we keep calling ReaderWriter.transferTo but inside of that method we add an option that is calling the old code if explicitly wanted. For example, we could replace the default buffer size 8192 by -1. For -1 we call transferTo. For any other number (hence explicitly set 8192 or any other value) we call the original code. @jansupol WDYT?
There was a problem hiding this comment.
@jansupol I think this is what we could agree upon: c118b65. It preserves the option to explicitly set the buffer size, but unless one does so, it let's the JRE do its job. So user who don't care get JRE optimizations out-of-the-box, and users who want to get the old code, can explicity set 8192. This is the best of all worlds.
There was a problem hiding this comment.
@mkarg ReaderWriter's BUFFER_SIZE is a public property with a default value of 8192. This must not be changed, think about compatibility.
I do not see much difference in my early copy of JDK 21. But I agree, the JDK internals can change. This is seen by extending the buffer values in JDK 21. While the tests with JDK may show the large buffer brings a slight performance boost, the customer may judge otherwise, based on their use case.
You are right, however, that by default the buffer size is not changed by the majority of customers. Hence, what we can do is forward to JDK's transfer method for cases where the customer does not change the BUFFER_SIZE to a value different from 8192.
There was a problem hiding this comment.
@jansupol I see we come nearer to a final solution. :-)
What we measured at OpenJDK is that the performance / efficiency benefit strongly depends greatly on the actual platform stack, less of the actual use case, which is why we still have decided for a one size fits it all buffer size even in JDK 21. I do not see any evidence that this experience should not fit to particularly Jersey. Anyways I have no objection against keeping the IO_BUFFER_SIZE option available for those rare cases that I might not see but possibly exist (do you actually know some?).
The actual code changes BTW can be found here: https://github.com/openjdk/jdk/pulls?q=is%3Apr+is%3Aclosed+author%3Amkarg. They (mostly) are effective solely when using transferTo, in particular when using transferTo in conjunction with JRE (non-custom) streams (hence the lots of changed implementations) - as you can see, over the past years, we changed and discussed a lot of JRE code, and that for a good reason (to make the most of modern server hardware / to not stand in the way of OS-internal optimizations).
Regarding the default size: I do not think that anybody really relies on particularly the value 8192 being the default for all times, but people instead rely on "BUFFER_SIZE == IO_DEFAULT_BUFFER_SIZE", and they rely on IO_DEFAULT_BUFFER_SIZE telling them the actual value at any time they can trust upon to be the default, so I think it would be OK if we tell them that IO_DEFAULT_BUFFER_SIZE == -1 from the next minor or major release. IMHO existing applications that did not check IO_DEFAULT_BUFFER_SIZE are simply wrong because that is the reason why the docs do not say "8192" but say "IO_DEFAULT_BUFFER_SIZE", right?
Only enabling the fallback code in case the application is setting a value other than 8192 sounds rather odd and ambiguous. What if a user wants to really use just 8192 bytes, because he does not want to squander the new JRE default of 16384 (or possibly even more bytes in JRE 22 or 23)? So I really would recommend to go with IO_DEFAULT_BUFFER_SIZE == -1 instead, to make the actual behavior change visible (if we trust our users to read the release notes), or (for maximum backwards compatibility) to actually detect any change by using a Jersey-internal flag which is set to true as soon as IO_BUFFER_SIZE >= 0 (i. e. Jersey internally ignores BUFFER_SIZE, but externally the BUFFER_SIZE backward-compatibly reflects IO_DEFAULT_BUFFER_SIZE == 8192).
Having said that, in what direction shall I effectively migrate this PR now?
There was a problem hiding this comment.
@jansupol "ping" - I need your decision regarding above proposal. Thanks.
There was a problem hiding this comment.
Really it is not a good practice to remove or change public properties, and values.
You have a point that people might require a lower buffer than the one in JDK on purpose. However, since the change comes in JDK 21 which is not released yet, it is not very likely people will want to override that value, and it makes more sense to me to live with overriding this value than changing public constants.
If you want to be precise, you can check for JDK version (using the Jersey JdkVersion class) and if JDK is 21, keep Jersey routine for BUFFER_SIZE 16384. But then, is it not the goal to use JDK code instead of Jersey? While I can imagine the poor guy spending days on testing various BUFFER_SIZE with JDK 11, I can hardly imagine that guy doing so with beta JDK 21.
There was a problem hiding this comment.
I actually think it is a pretty good practice to update default values from time to time, as users gain benefits without getting active themselfs.
Actually I do see a benefit of explicitly setting the BUFFER_SIZE property even on Java 11: For example setting it to 16k, as Java 11's default is 8k, so you gain performance even on old-but-widespread JREs. So it makes not much sense to keep the default value at 8k (with or without JRE 21), or speculate around which improvement might be in which JRE version from which vendor.
IMHO Jersey users prefer automatic performance improvements over buffer size being statically 8192, so my clear advice to Team Jersey would be to adopt my proposed compromise and let the default buffer size be -1 from now on, or at least modify it to 16k in Jersey 3.x and -1 in Jersey 4x. Anyways, I am neither the project lead nor a committer, so it is up to you @jansupol to decide.
Please tell me clearly the solution that you want me to add to this PR, so we get NIO-offloading ready for merge into Jersey 3.x ASAP. Thanks.
core-common/src/main/java/org/glassfish/jersey/message/internal/ReaderWriter.java
Outdated
Show resolved
Hide resolved
mkarg
force-pushed
the
nio
branch
2 times, most recently
from
d11ab23 to
c118b65
Compare
|
Please update the copyright year in modified files, the CI tests fail otherwise. |
Thank you, it seems I missed to update two files. Now all files how Copyright 2023. |
mkarg
force-pushed
the
nio
branch
2 times, most recently
from
c1846ce to
42078bd
Compare
|
I know - this is because I did not finish work, because the discussion whether we go with -1 or not is still open. (Edit: This is the reason why it is marked as draft) (Edit: Should be fixed in fa2cbd2, but the solution is temporary unless @jansupol finally decided if we go with -1 or if we stick with 8192) |
|
@jansupol How to proceed? Agreed with -1, or want me to go with 8192? |
|
I believe we need to keep all the properties (including IO_DEFAULT_BUFFER_SIZE) and not make any troubling changes. Since the changes are JDK-related, I would suggest opening the PR against the master branch so that each version of Jersey can benefit from your updates. |
I will do as you say.
The master branch uses Java 8 while my changes rely on Java 11, so I do not see a way to make that work. |
mkarg
force-pushed
the
nio
branch
4 times, most recently
from
540bab9 to
1f96cbe
Compare
...n/src/test/java/org/glassfish/jersey/tests/e2e/common/message/internal/ReaderWriterTest.java
Outdated
Show resolved
Hide resolved
|
Have you thought about putting the changes to 2.x? Most of it would work there (but the NullOutputStream) |
As I wrote earlier, this PR will not build unchanged on Jersey 2.x, as 2.x is based on Java 8. Besides NullOutputStream, InputStream::transferTo did not exist prior to Java 9, and Reader::transferTo did not exist prior to Java 10. So what I can do (with acceptable effort) is: Once this PR is merged into the 3.1 branch, I can open a second PR for the 2.x branch with backports of just those (few) parts that actually will work in Java 8. |
Thank you, Jan. I have switch this PR from draft to ready status, so Github allows to merge it. I propose you apply Github's own squash button (so I abstain from squash-and-force-push), so we do not lose the conversion history in Github. |
|
This PR exists since May 20. Today it is November 12. It would be nice to get told why not integrating it for so long. Thank you. |
Target
The aim of this PR is to improve…
Justification
Jersey is a key component of tens of thousands of servers operated world-wide, hence it should perform data transfers as quickly as possible, with least resource consumption as possible.
Description
This PR replaces usage of IO API by NIO API in some parts of Jersey.
Non-Targets
Background Information
Since Java 7 (and later, Java 9 and 11) there is a new NIO API which allows the JRE to do internal "tricks":
transferTorecently got hand-optimized, to work faster than typical custom application code and/or skip execution if possible and/or offload to wrapped streams, etc.).Note: I deliberately did not squash the commits, so reviewers get a better understanding of the particular reason for each change.
Note: I am the original author of several of the mentioned JRE-internal optimizations, and my target is to help downstream projects to adopt them, hence to make most of them. If there are any questions regarding these internals, feel free to directly ask me. NB: One of the projects I already helped to optimize was Maven, which benefits from the changes I made since several months.