Re: RFR: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying

2021-08-01 Thread Sergey Bylokhov
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov 
 wrote:

> I found few places, where code initially perform `Object[] 
> Colleciton.toArray()` call and then manually copy array into another array 
> with required type.
> This PR cleanups such places to more shorter call `T[] 
> Collection.toArray(T[])`.

Changes in the desktop module looks fine.

-

Marked as reviewed by serb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4487


Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]

2021-08-01 Thread Jie Fu
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev  wrote:

>> Hi all,
>> 
>> could you please review this big tedious and trivial(-ish) patch which moves 
>> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package?
>> 
>> the majority of the patch is the following substitutions:
>>  - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g`
>>  - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g`
>>  - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g`
>>  - `s/sun.hotspot.code/jdk.test.whitebox.code/g`
>>  - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g`
>>  - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g`
>> 
>> testing: tier1-4
>> 
>> Thanks,
>> -- Igor
>
> Igor Ignatyev has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains 12 new 
> commits since the last revision:
> 
>  - fixed ctw build
>  - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class
>  - updated requires.VMProps
>  - updated TEST.ROOT
>  - adjusted hotspot source
>  - added test
>  - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox)
>  - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class
>  - removed sun/hotspot/parser/DiagnosticCommand
>  - deprecated sun/hotspot classes
>disallowed s.h.WhiteBox w/ security manager
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860

Shall we also update the copyright year like 
test/lib/sun/hotspot/cpuinfo/CPUInfo.java?
Thanks.

-

PR: https://git.openjdk.java.net/jdk17/pull/290


Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]

2021-08-01 Thread David Holmes
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev  wrote:

>> Hi all,
>> 
>> could you please review this big tedious and trivial(-ish) patch which moves 
>> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package?
>> 
>> the majority of the patch is the following substitutions:
>>  - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g`
>>  - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g`
>>  - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g`
>>  - `s/sun.hotspot.code/jdk.test.whitebox.code/g`
>>  - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g`
>>  - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g`
>> 
>> testing: tier1-4
>> 
>> Thanks,
>> -- Igor
>
> Igor Ignatyev has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

Hi Igor,

This set of changes seems much more manageable to me.

Not sure about the new deprecation warnings for the old WB classes .. might 
that not itself trigger some failures? If not then I don't see how the 
deprecation warnings help with transitioning to the new WB class?

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/290


Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

2021-08-01 Thread Markus KARG
On Sun, 1 Aug 2021 08:24:13 GMT, Markus KARG 
 wrote:

>>> The modified code found in 
>>> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20)
>>>  should be safe from infinite loops, as it checks the actual file length in 
>>> each iteration (even if this costs us one more `synchronized` per 
>>> iteration).
>> 
>> I need to look at it closely but I suspect this introduces a potential 
>> overflow. Also if output stream is backed by a SocketChannel configured 
>> non-blocking then FC::transferTo may return 0 so I assume there is a 
>> potential infinite loop there too. I suspect the eventually patch will need 
>> have to make use of the blockingLock to prevent the underlying channels from 
>> being changed to non-blocking during the transfer.
>
> @AlanBateman Did I recap the sum your comments correctly, is the above 
> conclusion what you wanted to tell me? Shall I proceed with one of the two 
> solutions outlined in the "...2:2 matrix..." section of my answer *or* shall 
> I wait until you had a deeper look?

Asserting blocking mode for *both* sides (source and target) in 
https://github.com/openjdk/jdk/pull/4263/commits/fc38eae44de9e16468d33bd2ebab6502c92b4860.

Eliminated the resulting duplicate code in 
https://github.com/openjdk/jdk/pull/4263/commits/4a0d7cf74ee7e35aa0448df4b5ea4c5e3113ece6.

Do you see more problems we need to solve?

-

PR: https://git.openjdk.java.net/jdk/pull/4263


Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v13]

2021-08-01 Thread Markus KARG
> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with two additional 
commits since the last revision:

 - Draft: Eliminated duplicate code using lambda expressions
 - Draft: Use blocking mode also for target channel

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/55c96880..4a0d7cf7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=12
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=11-12

  Stats: 43 lines in 1 file changed: 21 ins; 8 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263


Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v12]

2021-08-01 Thread Markus KARG
> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  [Draft] Using blocking lock to prevent concurrently switching into 
non-blocking mode

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/f4485d5b..55c96880

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=11
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=10-11

  Stats: 16 lines in 1 file changed: 8 ins; 6 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263


Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

2021-08-01 Thread Markus KARG
On Sat, 31 Jul 2021 17:33:50 GMT, Alan Bateman  wrote:

>>> Also can you can check that IllegalBlockingModeException is thrown for the 
>>> case ch is a SelectableChannel configured non-blocking and the destination 
>>> is a FileChannel?
>> 
>> Done in 
>> https://github.com/openjdk/jdk/pull/4263/commits/8e2889fd6138d8aa8e308a1cd2aea3546a7c43a7,
>>  but honestly I'd kindly like to ask for a brief explanation why this has to 
>> be done. What actual bad effect would happen if I do not throw?
>
>> The modified code found in 
>> [4b501b2](https://github.com/openjdk/jdk/commit/4b501b205c6f1c48bbc82d7a154aed3fc41b1a20)
>>  should be safe from infinite loops, as it checks the actual file length in 
>> each iteration (even if this costs us one more `synchronized` per iteration).
> 
> I need to look at it closely but I suspect this introduces a potential 
> overflow. Also if output stream is backed by a SocketChannel configured 
> non-blocking then FC::transferTo may return 0 so I assume there is a 
> potential infinite loop there too. I suspect the eventually patch will need 
> have to make use of the blockingLock to prevent the underlying channels from 
> being changed to non-blocking during the transfer.

@AlanBateman Did I recap the sum your comments correctly, is the above 
conclusion what you wanted to tell me? Shall I proceed with one of the two 
solutions outlined in the "...2:2 matrix..." section of my answer *or* shall I 
wait until you had a deeper look?

-

PR: https://git.openjdk.java.net/jdk/pull/4263


Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

2021-08-01 Thread Thomas Stuefe
On Sun, 1 Aug 2021 08:17:08 GMT, Thomas Stuefe  wrote:

>> Short: this patch makes NMT available in custom-launcher scenarios and 
>> during gtests. It simplifies NMT initialization. It adds a lot of 
>> NMT-specific testing, cleans them up and makes them sideeffect-free.
>> 
>> -
>> 
>> NMT continues to be an extremely useful tool for SAP to tackle memory 
>> problems in the JVM.
>> 
>> However, NMT is of limited use due to the following restrictions:
>> 
>> - NMT cannot be used if the hotspot is embedded into a custom launcher 
>> unless the launcher actively cooperates. Just creating and invoking the JVM 
>> is not enough, it needs to do some steps prior to loading the hotspot. This 
>> limitation is not well known (nor, do I believe, documented). Many products 
>> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this 
>> problem limits NMT usefulness greatly since our VMs are often embedded into 
>> custom launchers and modifying every launcher is impossible.
>> - Worse, if that custom launcher links the libjvm *statically* there is just 
>> no way to activate NMT at all. This is the reason NMT cannot be used in the 
>> `gtestlauncher`.
>> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` 
>> and `-XX:Flags=`.
>> - The fact that NMT cannot be used in gtests is really a pity since it would 
>> allow us to both test NMT itself more rigorously and check for memory leaks 
>> while testing other stuff.
>> 
>> The reason for all this is that NMT initialization happens very early, on 
>> the first call to `os::malloc()`. And those calls happen already during 
>> dynamic C++ initialization - a long time before the VM gets around parsing 
>> arguments. So, regular VM argument parsing is too late to parse NMT 
>> arguments.
>> 
>> The current solution is to pass NMT arguments via a specially prepared 
>> environment variable: `NMT_LEVEL_=`. That environment 
>> variable has to be set by the embedding launcher, before it loads the 
>> libjvm. Since its name contains the PID, we cannot even set that variable in 
>> the shell before starting the launcher.
>> 
>> All that means that every launcher needs to especially parse and process the 
>> NMT arguments given at the command line (or via whatever method) and prepare 
>> the environment variable. `java` itself does this. This only works before 
>> the libjvm.so is loaded, before its dynamic C++ initialization. For that 
>> reason, it does not work if the launcher links statically against the 
>> hotspot, since in that case C++ initialization of the launcher and hotspot 
>> are folded into one phase with no possibility of executing code beforehand.
>> 
>> And since it bypasses argument handling in the VM, it bypasses a number of 
>> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`.
>> 
>> --
>> 
>> This patch fixes these shortcomings by making NMT late-initializable: it can 
>> now be initialized after normal VM argument parsing, like all other parts of 
>> the VM. This greatly simplifies NMT initialization and makes it work 
>> automagically for every third party launcher, as well as within our gtests.
>> 
>> The glaring problem with late-initializing NMT is the NMT malloc headers. If 
>> we rule out just always having them (unacceptable in terms of memory 
>> overhead), there is no safe way to determine, in os::free(), if an 
>> allocation came from before or after NMT initialization ran, and therefore 
>> what to do with its malloc headers. For a more extensive explanation, please 
>> see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett 
>> and @zhengyu123 in the JBS comment section.
>> 
>> The heart of this patch is a new way to track early, pre-NMT-init 
>> allocations. These are tracked via a lookup table. This was a suggestion by 
>> Kim and it worked out well.
>> 
>> Changes in detail:
>> 
>> - pre-NMT-init handling:
>>  - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init 
>> handling. They contain a small global lookup table managing C-heap blocks 
>> allocated in the pre-NMT-init phase.
>>  - `os::malloc()/os::realloc()/os::free()` defer to this code before 
>> doing anything else.
>>  - Please see the extensive comment block at the start of 
>> `nmtPreinit.hpp` explaining the details.
>> 
>> - Changes to NMT:
>>  - Before, NMT initialization was spread over two phases, `initialize()` 
>> and `late_initialize()`. Those were merged into one and simplified - there 
>> is only one initialization now which happens after argument parsing.
>>  - Minor changes were needed for the `NMT_TrackingLevel` enum - to 
>> simplify code, I changed NMT_unknown to be numerically 0. A new comment 
>> block in `nmtCommon.hpp` now clearly specifies what's what, including 
>> allowed level state transitions.
>>  - New utility functions to translate tracking level from/to strings 
>> added to `NMTUtil`
>>  - NMT has never been able to handle virtual memory al

Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

2021-08-01 Thread Thomas Stuefe
On Fri, 30 Jul 2021 20:14:04 GMT, Zhengyu Gu  wrote:

>> Thomas Stuefe has updated the pull request incrementally with six additional 
>> commits since the last revision:
>> 
>>  - feedback zhengyu
>>  - feeback Coleen/Kim (constness fix)
>>  - Feedback David
>>  - Add test to test for non-java launcher support of NMT
>>  - move all test code to gtest
>>  - actually free blocks freed in pre-init phase
>
> src/hotspot/share/services/nmtPreInit.hpp line 153:
> 
>> 151: 
>> 152:   static unsigned calculate_hash(const void* p) {
>> 153: uintptr_t tmp = p2i(p);
> 
> malloc memory usually is 2-machine word aligned, maybe tmp = tmp >> 
> LP64_ONLY(4) NOT_LP64(3) can result better hash distribution?

I thought so too at first, but it is not necessary. The hash function uses all 
input bits:

uintptr_t tmp = p2i(p);
unsigned hash = (unsigned)tmp
 LP64_ONLY( ^ (unsigned)(tmp >> 32));

p2i is lossless since input and result have the same size.
Then,
- for 32-bit, it does not matter since unsigned is the same size as uintptr_t 
and we lose no bits
- for 64-bit, we xor the upper half of the 64-bit input value into the lower 
half; again, all input bits count toward the result. We don't gain anything by 
shifting, we would just exchange the lower 2 bits always being zero against the 
upper 2 bits always being zero.

---

BTW, I experimented with different hash functions, e.g. 
http://www.concentric.net/~Ttwang/tech/inthash.htm, but did not really get a 
better distribution. This somewhat mirrors my experiences when I tried to 
optimize your hash function in the MallocSiteTable. Seems malloc return value 
is already "random enough". I coupled it with a crooked table size though, made 
it a prime.

-

PR: https://git.openjdk.java.net/jdk/pull/4874


Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

2021-08-01 Thread Thomas Stuefe
> Short: this patch makes NMT available in custom-launcher scenarios and during 
> gtests. It simplifies NMT initialization. It adds a lot of NMT-specific 
> testing, cleans them up and makes them sideeffect-free.
> 
> -
> 
> NMT continues to be an extremely useful tool for SAP to tackle memory 
> problems in the JVM.
> 
> However, NMT is of limited use due to the following restrictions:
> 
> - NMT cannot be used if the hotspot is embedded into a custom launcher unless 
> the launcher actively cooperates. Just creating and invoking the JVM is not 
> enough, it needs to do some steps prior to loading the hotspot. This 
> limitation is not well known (nor, do I believe, documented). Many products 
> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this 
> problem limits NMT usefulness greatly since our VMs are often embedded into 
> custom launchers and modifying every launcher is impossible.
> - Worse, if that custom launcher links the libjvm *statically* there is just 
> no way to activate NMT at all. This is the reason NMT cannot be used in the 
> `gtestlauncher`.
> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` 
> and `-XX:Flags=`.
> - The fact that NMT cannot be used in gtests is really a pity since it would 
> allow us to both test NMT itself more rigorously and check for memory leaks 
> while testing other stuff.
> 
> The reason for all this is that NMT initialization happens very early, on the 
> first call to `os::malloc()`. And those calls happen already during dynamic 
> C++ initialization - a long time before the VM gets around parsing arguments. 
> So, regular VM argument parsing is too late to parse NMT arguments.
> 
> The current solution is to pass NMT arguments via a specially prepared 
> environment variable: `NMT_LEVEL_=`. That environment 
> variable has to be set by the embedding launcher, before it loads the libjvm. 
> Since its name contains the PID, we cannot even set that variable in the 
> shell before starting the launcher.
> 
> All that means that every launcher needs to especially parse and process the 
> NMT arguments given at the command line (or via whatever method) and prepare 
> the environment variable. `java` itself does this. This only works before the 
> libjvm.so is loaded, before its dynamic C++ initialization. For that reason, 
> it does not work if the launcher links statically against the hotspot, since 
> in that case C++ initialization of the launcher and hotspot are folded into 
> one phase with no possibility of executing code beforehand.
> 
> And since it bypasses argument handling in the VM, it bypasses a number of 
> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`.
> 
> --
> 
> This patch fixes these shortcomings by making NMT late-initializable: it can 
> now be initialized after normal VM argument parsing, like all other parts of 
> the VM. This greatly simplifies NMT initialization and makes it work 
> automagically for every third party launcher, as well as within our gtests.
> 
> The glaring problem with late-initializing NMT is the NMT malloc headers. If 
> we rule out just always having them (unacceptable in terms of memory 
> overhead), there is no safe way to determine, in os::free(), if an allocation 
> came from before or after NMT initialization ran, and therefore what to do 
> with its malloc headers. For a more extensive explanation, please see the 
> comment block `nmtPreInit.hpp` and the discussion with @kimbarrett and 
> @zhengyu123 in the JBS comment section.
> 
> The heart of this patch is a new way to track early, pre-NMT-init 
> allocations. These are tracked via a lookup table. This was a suggestion by 
> Kim and it worked out well.
> 
> Changes in detail:
> 
> - pre-NMT-init handling:
>   - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init 
> handling. They contain a small global lookup table managing C-heap blocks 
> allocated in the pre-NMT-init phase.
>   - `os::malloc()/os::realloc()/os::free()` defer to this code before 
> doing anything else.
>   - Please see the extensive comment block at the start of 
> `nmtPreinit.hpp` explaining the details.
> 
> - Changes to NMT:
>   - Before, NMT initialization was spread over two phases, `initialize()` 
> and `late_initialize()`. Those were merged into one and simplified - there is 
> only one initialization now which happens after argument parsing.
>   - Minor changes were needed for the `NMT_TrackingLevel` enum - to 
> simplify code, I changed NMT_unknown to be numerically 0. A new comment block 
> in `nmtCommon.hpp` now clearly specifies what's what, including allowed level 
> state transitions.
>   - New utility functions to translate tracking level from/to strings 
> added to `NMTUtil`
>   - NMT has never been able to handle virtual memory allocations before 
> initialization, which is fine since os::reserve_memory() is not called before 
> VM parses arguments. We now assert that.
>

Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

2021-08-01 Thread Markus KARG
On Sun, 1 Aug 2021 07:46:36 GMT, Markus KARG 
 wrote:

>>> I need to look at it closely but I suspect this introduces a potential 
>>> overflow. Also if output stream is backed by a SocketChannel configured 
>>> non-blocking then FC::transferTo may return 0 so I assume there is a 
>>> potential infinite loop there too. I suspect the eventually patch will need 
>>> have to make use of the blockingLock to prevent the underlying channels 
>>> from being changed to non-blocking during the transfer.
>> 
>> I need to confess that my NIO knowledge is not deep enough to follow you 
>> closely, so I trust on your advice how to go on from here.
>
>> I suspect the eventually patch will need have to make use of the 
>> blockingLock to prevent the underlying channels from being changed to 
>> non-blocking during the transfer.
> 
> The blocking lock now is used since 
> https://github.com/openjdk/jdk/pull/4263/commits/f4485d5b6a3b5c471feff5642dfef0fc747d3fac
>  to prevent this.

> Also can you can check that IllegalBlockingModeException is thrown for the 
> case ch is a SelectableChannel configured non-blocking and the destination is 
> a FileChannel?

Do we really only have to check this in the particular case of `FileChannel` 
destinations?

And don't we have to assert blocking mode for *destination* channels, too (just 
like `ChannelOutputStream::writeFully` does)?

As this results in an 2:2 matrix of possibilities (src is selectable nor not, 
dst is selectable or not) it might be easier to do *only the blocking checks* 
in `transferTo` but let it call something like 
`transferFromSelectableToNonSelectable` in turn *or* to wrap *each* 
implementation of `transferTo` in a wrapper like `executeInBlockingMode((src, 
dst) -> transferTo(src, dst))`...?

-

PR: https://git.openjdk.java.net/jdk/pull/4263


Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v6]

2021-08-01 Thread Markus KARG
On Sat, 31 Jul 2021 21:20:28 GMT, Markus KARG 
 wrote:

> I suspect the eventually patch will need have to make use of the blockingLock 
> to prevent the underlying channels from being changed to non-blocking during 
> the transfer.

The blocking lock now is used since 
https://github.com/openjdk/jdk/pull/4263/commits/f4485d5b6a3b5c471feff5642dfef0fc747d3fac
 to prevent this.

-

PR: https://git.openjdk.java.net/jdk/pull/4263


Re: RFR: 8265891: (ch) InputStream returned by Channels.newInputStream should override transferTo [v11]

2021-08-01 Thread Markus KARG
> This PR-*draft* is **work in progress** and an invitation to discuss a 
> possible solution for issue 
> [JDK-8265891](https://bugs.openjdk.java.net/browse/JDK-8265891). It is *not 
> yet* intended for a final review.
> 
> As proposed in JDK-8265891, this PR provides an implementation for 
> `Channels.newInputStream().transferTo()` which provide superior performance 
> compared to the current implementation. The changes are:
> * Prevents transfers through the JVM heap as much as possibly by offloading 
> to deeper levels via NIO, hence allowing the operating system to optimize the 
> transfer.
> * Using more JRE heap in the fallback case when no NIO is possible (still 
> only KiBs, hence mostl ynegligible even on SBCs) to better perform on modern 
> hardware / fast I/O devides.
> 
> Using JMH I have benchmarked both, the original implementation and this 
> implementation, and (depending on the used hardware and use case) performance 
> change was approx. doubled performance. So this PoC proofs that it makes 
> sense to finalize this work and turn it into an actual OpenJDK contribution. 
> 
> I encourage everybody to discuss this draft:
> * Are there valid arguments for *not* doing this change?
> * Is there a *better* way to improve performance of 
> `Channels.newInputStream().transferTo()`?
> * How to go on from here: What is missing to get this ready for an actual 
> review?

Markus KARG has updated the pull request incrementally with one additional 
commit since the last revision:

  [Draft] Using blocking lock to prevent concurrently switching into 
non-blocking mode

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4263/files
  - new: https://git.openjdk.java.net/jdk/pull/4263/files/8e2889fd..f4485d5b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=10
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4263&range=09-10

  Stats: 16 lines in 1 file changed: 8 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4263.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4263/head:pull/4263

PR: https://git.openjdk.java.net/jdk/pull/4263