Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > moving nr declaration from the beginning of the method to where it's > actually used # JMH version: 1.34 # VM version: JDK 19-internal, OpenJDK 64-Bit Server VM, 19-internal-adhoc..jdk # VM invoker: F:\workspace\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.exe # VM options: -Djava.library.path=f:\workspace\jdk\build\windows-x86_64-server-release\images\test\micro\native -Xms1g -Xmx1g # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable) # Warmup: 10 iterations, 500 ms each # Measurement: 10 iterations, 500 ms each # Timeout: 10 min per iteration # Threads: 1 thread, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: org.openjdk.bench.java.io.InputStreamSkipBenchmark.testSkip0 # Parameters: (inputStreamSize = 100, skipLength = 1) # Run progress: 0.00% complete, ETA 00:14:00 # Fork: 1 of 3 # Warmup Iteration 1: 3188201.250 ns/op # Warmup Iteration 2: 2445386.603 ns/op # Warmup Iteration 3: 2395206.481 ns/op # Warmup Iteration 4: 2391797.196 ns/op # Warmup Iteration 5: 2518698.039 ns/op # Warmup Iteration 6: 2414542.254 ns/op # Warmup Iteration 7: 2419808.019 ns/op # Warmup Iteration 8: 2430526.540 ns/op # Warmup Iteration 9: 2430806.635 ns/op # Warmup Iteration 10: 2414558.173 ns/op Iteration 1: 2426910.427 ns/op Iteration 2: 2429144.811 ns/op Iteration 3: 2574322.500 ns/op Iteration 4: 2396079.907 ns/op Iteration 5: 2425966.509 ns/op Iteration 6: 2461450.239 ns/op Iteration 7: 2527480.392 ns/op Iteration 8: 2523890.640 ns/op Iteration 9: 2511460.488 ns/op Iteration 10: 2532332.843 ns/op # Run progress: 1.19% complete, ETA 00:14:36 # Fork: 2 of 3 # Warmup Iteration 1: 3174534.969 ns/op # Warmup Iteration 2: 2499138.235 ns/op # Warmup Iteration 3: 2421553.521 ns/op # Warmup Iteration 4: 2416571.698 ns/op # Warmup Iteration 5: 2384638.889 ns/op # Warmup Iteration 6: 2377448.387 ns/op # Warmup Iteration 7: 2395746.512 ns/op # Warmup Iteration 8: 2384657.209 ns/op # Warmup Iteration 9: 2403249.302 ns/op # Warmup Iteration 10: 2397511.163 ns/op Iteration 1: 2386731.481 ns/op Iteration 2: 2418383.491 ns/op Iteration 3: 2394599.535 ns/op Iteration 4: 2436117.536 ns/op Iteration 5: 2399048.357 ns/op Iteration 6: 2483871.498 ns/op Iteration 7: 2496279.612 ns/op Iteration 8: 2496743.204 ns/op Iteration 9: 2491237.811 ns/op Iteration 10: 2493390.338 ns/op # Run progress: 2.38% complete, ETA 00:14:25 # Fork: 3 of 3 # Warmup Iteration 1: 3677115.714 ns/op # Warmup Iteration 2: 2423365.877 ns/op # Warmup Iteration 3: 2480975.962 ns/op # Warmup Iteration 4: 2412990.610 ns/op # Warmup Iteration 5: 2433951.185 ns/op # Warmup Iteration 6: 2389266.190 ns/op # Warmup Iteration 7: 2407245.933 ns/op # Warmup Iteration 8: 2375563.721 ns/op # Warmup Iteration 9: 2388543.981 ns/op # Warmup Iteration 10: 2393794.419 ns/op Iteration 1: 2403104.306 ns/op Iteration 2: 2414766.355 ns/op Iteration 3: 2408296.667 ns/op Iteration 4: 2509554.412 ns/op Iteration 5: 2402212.440 ns/op Iteration 6: 2436674.272 ns/op Iteration 7: 2466500.000 ns/op Iteration 8: 2472919.712 ns/op Iteration 9: 2462019.139 ns/op Iteration 10: 2453551.905 ns/op Result "org.openjdk.bench.java.io.InputStreamSkipBenchmark.testSkip0": 2457834.694 ±(99.9%) 33354.477 ns/op [Average] (min, avg, max) = (2386731.481, 2457834.694, 2574322.500), stdev = 49923.415 CI (99.9%): [2424480.217, 2491189.172] (assumes normal distribution) # JMH version: 1.34 # VM version: JDK 19-internal, OpenJDK 64-Bit Server VM, 19-internal-adhoc..jdk # VM invoker: F:\workspace\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.exe # VM options: -Djava.library.path=f:\workspace\jdk\build\windows-x86_64-server-release\images\test\micro\native -Xms1g -Xmx1g # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable) # Warmup: 10 iterations, 500 ms each # Measurement: 10 iterations, 500 ms each # Timeout: 10 min per iteration # Threads: 1 thread, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: org.openjdk.bench.java.io.InputStreamSkipBenchmark.testSkip0 # Parameters: (inputStreamSize = 100, skipLength = 8) # Run progress: 3.57% complete, ETA 00:14:13 # Fork: 1 of 3 # Warmup Iteration 1: 432960.511 ns/op # Warmup Iteration 2: 336604.734 ns/op # Warmup Iteration 3: 330185.843 ns/op # Warmup Iteration 4: 325184.675 ns/op # Warmup Iteration 5: 328446.641 ns/op # Warmup Iteration 6: 327902.621 ns/op # Warmup Iteration 7: 330808.786 ns/op # Warmup Iteration 8: 347259.824 ns/op # Warmup Iteration 9: 325685.010 ns/op # Warmu
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > moving nr declaration from the beginning of the method to where it's > actually used I'd be a quite cautious about the slippery slope of incremental change with very little measurable benefit. Most applications reading InputStreams or Readers are using the buffered versions that already handle skip. These are the fallback versions. Except for the buffered versions, the lower level InputStreams can do a better job of skip then at the higher level built on read(). FileInputStream directly uses seek, other direct streams can probably do a more efficient skip() by reading into private buffers. For example low level I/O for networking already reads into private buffers. But the dominate case is still the buffered streams and readers. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> moving nr declaration from the beginning of the method to where it's
> actually used
> Yeah forgot about that part. Thus I guess using soft reference would be a
> better way to deal with this extra memory use issue. So something like
>
> ```java
> private SoftReference skipBuffer;
>
> private byte[] skipBuffer(long remaining) {
> int size = (int) Math.min(MAX_SKIP_BUFFER_SIZE, remaining);
> SoftReference ref = this.skipBuffer;
> byte[] buffer;
> if (ref == null || (buffer = ref.get()) == null || buffer.length < size) {
> buffer = new byte[size];
> this.skipBuffer = new SoftReference(buffer);
> }
> return buffer;
> }
> ```
>
> This should be thread-safe, and we can just call `byte[] skipBuffer =
> skipBuffer(remaining);`
LGTM
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> moving nr declaration from the beginning of the method to where it's
> actually used
Yeah forgot about that part. Thus I guess using soft reference would be a
better way to deal with this extra memory use issue. So something like
private SoftReference skipBuffer;
private byte[] skipBuffer(long remaining) {
int size = (int) Math.min(MAX_SKIP_BUFFER_SIZE, remaining);
SoftReference ref = this.skipBuffer;
byte[] buffer;
if (ref == null || (buffer = ref.get()) == null || buffer.length < size) {
buffer = new byte[size];
this.skipBuffer = new SoftReference(buffer);
}
return buffer;
}
This should be thread-safe, and we can just call `byte[] skipBuffer =
skipBuffer(remaining);`
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 15:23:16 GMT, XenoAmess wrote: > > On a side note for unifying the skip buffer implementation of reader vs > > input stream: For the input stream subclasses in the JDK that have their > > own skip with buffering logic (as described in > > https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they > > almost always have only local-variable skip buffers (not kept as fields for > > reuse), and their buffers' max sizes are smaller that provided by the > > InputStream class. > > > > Imo we should check the usage of `skip` in other projects; in JDK it's like > > skipping 2 bytes in certain image formats, and I would expect usages like > > reading class file attribute name and size then skip by the read size. > > > > > This change may be problematic for servers with a large number > > > connections and an input stream for each connection. It could add up to > > > 2k to the footprint of each connection when skip is used. > > > > If per-object allocation is a problem, would it be feasible to allocate a > > static soft reference to a max-sized skip buffer? It can be potentially > > shared with the `Reader` class, too. > > No as security reason. Any subclass can read this buffer using read function， > thus might cause secure data leak. see https://github.com/openjdk/jdk/pull/5855 - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 15:20:06 GMT, liach wrote: > On a side note for unifying the skip buffer implementation of reader vs input > stream: For the input stream subclasses in the JDK that have their own skip > with buffering logic (as described in > https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they almost > always have only local-variable skip buffers (not kept as fields for reuse), > and their buffers' max sizes are smaller that provided by the InputStream > class. > > Imo we should check the usage of `skip` in other projects; in JDK it's like > skipping 2 bytes in certain image formats, and I would expect usages like > reading class file attribute name and size then skip by the read size. > > > This change may be problematic for servers with a large number connections > > and an input stream for each connection. It could add up to 2k to the > > footprint of each connection when skip is used. > > If per-object allocation is a problem, would it be feasible to allocate a > static soft reference to a max-sized skip buffer? It can be potentially > shared with the `Reader` class, too. No as security reason. Any subclass can read this buffer using read function， thus might cause secure data leak. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > moving nr declaration from the beginning of the method to where it's > actually used On a side note for unifying the skip buffer implementation of reader vs input stream: For the input stream subclasses in the JDK that have their own skip with buffering logic (as described in https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they almost always have only local-variable skip buffers (not kept as fields for reuse), and their buffers' max sizes are smaller that provided by the InputStream class. Imo we should check the usage of `skip` in other projects; in JDK it's like skipping 2 bytes in certain image formats, and I would expect usages like reading class file attribute name and size then skip by the read size. > This change may be problematic for servers with a large number connections > and an input stream for each connection. It could add up to 2k to the > footprint of each connection when skip is used. If per-object allocation is a problem, would it be feasible to allocate a static soft reference to a max-sized skip buffer? It can be potentially shared with the `Reader` class, too. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
> @jmehrens what about this then? > I think it safe now(actually this mechanism is learned from Reader) XenoAmess has updated the pull request incrementally with one additional commit since the last revision: moving nr declaration from the beginning of the method to where it's actually used - Changes: - all: https://git.openjdk.java.net/jdk/pull/5872/files - new: https://git.openjdk.java.net/jdk/pull/5872/files/81e9ca49..85cec668 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5872/head:pull/5872 PR: https://git.openjdk.java.net/jdk/pull/5872
