RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Claes Redestad-2
This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.

- Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
- Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
- Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.

Testing: tier1-3

-------------

Commit messages:
 - Safer publication of JavaLangAccess
 - Logic error
 - Move creation of JavaLangAccess up before creating System.in/out
 - Fix typo (caught by Test4625418.java)
 - Merge branch 'master' into streamdecoder
 - Let decoders use String intrinsics to inflate ASCII-only bytes
 - Remove trailing spaces
 - Add microbenchmark
 - Simplify and improve StreamDecoder/-Encoder

Changes: https://git.openjdk.java.net/jdk/pull/2574/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261744
  Stats: 706 lines in 13 files changed: 300 ins; 107 del; 299 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2574.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2574/head:pull/2574

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Claes Redestad-2
On Mon, 15 Feb 2021 11:22:10 GMT, Claes Redestad <[hidden email]> wrote:

> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>
> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>
> Testing: tier1-3

On the best-case microbenchmark (the byte stream is all ASCII), speed-ups are quite telling:

Before:
Benchmark                                        (charsetName)  (length)  Mode  Cnt       Score       Error  Units
ByteStreamDecoder.readStringReader                    US-ASCII       256  avgt   10    2085.399 ±    66.522  ns/op
ByteStreamDecoder.readStringReader                    US-ASCII      4096  avgt   10   12466.702 ±   747.116  ns/op
ByteStreamDecoder.readStringReader                    US-ASCII     25000  avgt   10  123508.987 ±  3583.345  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-1       256  avgt   10    1894.185 ±    51.772  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-1      4096  avgt   10    8117.404 ±   594.708  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-1     25000  avgt   10   99409.792 ± 28308.936  ns/op
ByteStreamDecoder.readStringReader                       UTF-8       256  avgt   10    2090.337 ±    56.500  ns/op
ByteStreamDecoder.readStringReader                       UTF-8      4096  avgt   10   11698.221 ±   898.910  ns/op
ByteStreamDecoder.readStringReader                       UTF-8     25000  avgt   10   66568.987 ±  4204.361  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-6       256  avgt   10    3061.130 ±   120.132  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-6      4096  avgt   10   24623.494 ±  1916.362  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-6     25000  avgt   10  139138.140 ±  7109.636  ns/op
ByteStreamDecoder.readStringReader                       MS932       256  avgt   10    2612.535 ±    98.638  ns/op
ByteStreamDecoder.readStringReader                       MS932      4096  avgt   10   18843.438 ±  1767.822  ns/op
ByteStreamDecoder.readStringReader                       MS932     25000  avgt   10  119923.997 ± 18560.065  ns/op

After:
Benchmark                                        (charsetName)  (length)  Mode  Cnt       Score       Error  Units
ByteStreamDecoder.readStringReader                    US-ASCII       256  avgt   10    1556.588 ±    37.083  ns/op
ByteStreamDecoder.readStringReader                    US-ASCII      4096  avgt   10    3290.627 ±   125.327  ns/op
ByteStreamDecoder.readStringReader                    US-ASCII     25000  avgt   10   13118.794 ±   597.086  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-1       256  avgt   10    1525.460 ±    36.510  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-1      4096  avgt   10    3051.887 ±   113.036  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-1     25000  avgt   10   11401.228 ±   563.124  ns/op
ByteStreamDecoder.readStringReader                       UTF-8       256  avgt   10    1596.878 ±    43.824  ns/op
ByteStreamDecoder.readStringReader                       UTF-8      4096  avgt   10    3349.961 ±   119.278  ns/op
ByteStreamDecoder.readStringReader                       UTF-8     25000  avgt   10   13273.403 ±   591.600  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-6       256  avgt   10    1602.328 ±    44.092  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-6      4096  avgt   10    3403.312 ±   107.516  ns/op
ByteStreamDecoder.readStringReader                  ISO-8859-6     25000  avgt   10   13163.468 ±   709.642  ns/op
ByteStreamDecoder.readStringReader                       MS932       256  avgt   10    1602.837 ±    32.021  ns/op
ByteStreamDecoder.readStringReader                       MS932      4096  avgt   10    3379.439 ±    87.716  ns/op
ByteStreamDecoder.readStringReader                       MS932     25000  avgt   10   13376.980 ±   669.983  ns/op

Performance degrades when you mix in non-ASCII characters, but so does the `new String(byte[], Charset)` baseline. While there might be algorithmic refinements possible to improve on some of the non-ASCII variants I'm happy to leave that to a follow-up and keep this RFE reasonably straightforward.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Claes Redestad-2
In reply to this post by Claes Redestad-2
> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>
> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>
> Testing: tier1-3

Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:

  Revert rem assertions

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2574/files
  - new: https://git.openjdk.java.net/jdk/pull/2574/files/8da49e16..b0bf8cfb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=00-01

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

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Philippe Marschall-2
In reply to this post by Claes Redestad-2
On Mon, 15 Feb 2021 11:30:54 GMT, Claes Redestad <[hidden email]> wrote:

>> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>>
>> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
>> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
>> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>>
>> Testing: tier1-3
>
> On the best-case microbenchmark (the byte stream is all ASCII), speed-ups are quite telling:
>
> Before:
> Benchmark                                        (charsetName)  (length)  Mode  Cnt       Score       Error  Units
> ByteStreamDecoder.readStringReader                    US-ASCII       256  avgt   10    2085.399 ±    66.522  ns/op
> ByteStreamDecoder.readStringReader                    US-ASCII      4096  avgt   10   12466.702 ±   747.116  ns/op
> ByteStreamDecoder.readStringReader                    US-ASCII     25000  avgt   10  123508.987 ±  3583.345  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-1       256  avgt   10    1894.185 ±    51.772  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-1      4096  avgt   10    8117.404 ±   594.708  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-1     25000  avgt   10   99409.792 ± 28308.936  ns/op
> ByteStreamDecoder.readStringReader                       UTF-8       256  avgt   10    2090.337 ±    56.500  ns/op
> ByteStreamDecoder.readStringReader                       UTF-8      4096  avgt   10   11698.221 ±   898.910  ns/op
> ByteStreamDecoder.readStringReader                       UTF-8     25000  avgt   10   66568.987 ±  4204.361  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-6       256  avgt   10    3061.130 ±   120.132  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-6      4096  avgt   10   24623.494 ±  1916.362  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-6     25000  avgt   10  139138.140 ±  7109.636  ns/op
> ByteStreamDecoder.readStringReader                       MS932       256  avgt   10    2612.535 ±    98.638  ns/op
> ByteStreamDecoder.readStringReader                       MS932      4096  avgt   10   18843.438 ±  1767.822  ns/op
> ByteStreamDecoder.readStringReader                       MS932     25000  avgt   10  119923.997 ± 18560.065  ns/op
>
> After:
> Benchmark                                        (charsetName)  (length)  Mode  Cnt       Score       Error  Units
> ByteStreamDecoder.readStringReader                    US-ASCII       256  avgt   10    1556.588 ±    37.083  ns/op
> ByteStreamDecoder.readStringReader                    US-ASCII      4096  avgt   10    3290.627 ±   125.327  ns/op
> ByteStreamDecoder.readStringReader                    US-ASCII     25000  avgt   10   13118.794 ±   597.086  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-1       256  avgt   10    1525.460 ±    36.510  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-1      4096  avgt   10    3051.887 ±   113.036  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-1     25000  avgt   10   11401.228 ±   563.124  ns/op
> ByteStreamDecoder.readStringReader                       UTF-8       256  avgt   10    1596.878 ±    43.824  ns/op
> ByteStreamDecoder.readStringReader                       UTF-8      4096  avgt   10    3349.961 ±   119.278  ns/op
> ByteStreamDecoder.readStringReader                       UTF-8     25000  avgt   10   13273.403 ±   591.600  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-6       256  avgt   10    1602.328 ±    44.092  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-6      4096  avgt   10    3403.312 ±   107.516  ns/op
> ByteStreamDecoder.readStringReader                  ISO-8859-6     25000  avgt   10   13163.468 ±   709.642  ns/op
> ByteStreamDecoder.readStringReader                       MS932       256  avgt   10    1602.837 ±    32.021  ns/op
> ByteStreamDecoder.readStringReader                       MS932      4096  avgt   10    3379.439 ±    87.716  ns/op
> ByteStreamDecoder.readStringReader                       MS932     25000  avgt   10   13376.980 ±   669.983  ns/op
>
> Performance degrades when you mix in non-ASCII characters, but so does the `new String(byte[], Charset)` baseline. While there might be algorithmic refinements possible to improve on some of the non-ASCII variants I'm happy to leave that to a follow-up and keep this RFE reasonably straightforward.

Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Claes Redestad-2
On Mon, 15 Feb 2021 19:56:32 GMT, Philippe Marschall <[hidden email]> wrote:

> Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?

Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem very reasonable, which I was thinking of exploring next as a separate RFE.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Claes Redestad-2
On Mon, 15 Feb 2021 20:34:53 GMT, Claes Redestad <[hidden email]> wrote:

> > Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?
>
> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem very reasonable, which I was thinking of exploring next as a separate RFE.

Maybe I misunderstood. The intrinsified method you point out here pre-dates the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617

It might be worthwhile cleaning this up. Not having to route via SharedSecrets -> JavaLangAccess does speed things up during startup/interpretation, at the cost of some code duplication.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Alan Bateman-2
On Tue, 16 Feb 2021 13:45:41 GMT, Claes Redestad <[hidden email]> wrote:

>>> Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?
>>
>> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem very reasonable, which I was thinking of exploring next as a separate RFE.
>
>> > Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?
>>
>> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem very reasonable, which I was thinking of exploring next as a separate RFE.
>
> Maybe I misunderstood. The intrinsified method you point out here pre-dates the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617
>
> It might be worthwhile cleaning this up. Not having to route via SharedSecrets -> JavaLangAccess does speed things up during startup/interpretation, at the cost of some code duplication.

This looks a really good change and it's great can these decoders get the benefit of the instrinics work.

ISO_8869_1.decodeArrayLoop using JLA.inflate looks a bit strange and maybe we can rename the method in the shared secret to inflatedCopy or just copy. or just add a comment. The reason is that code outside of java.lang shouldn't know anything about String's internals and inflation.

The reformatting changes to StreamEncoder/StreamDecoder make it hard to see if there are any actual changes. Is there anything we should look at? It's okay to include this cleanup, I can't tell what happened with this source file.

I want to study the change System.setJavaLangAccess a bit further to understand why this is needed (I know there is a awkward bootstrapping issue here).

I expect Naoto will want to review this PR too.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Claes Redestad-2
On Tue, 16 Feb 2021 15:05:33 GMT, Alan Bateman <[hidden email]> wrote:

>>> > Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?
>>>
>>> Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem very reasonable, which I was thinking of exploring next as a separate RFE.
>>
>> Maybe I misunderstood. The intrinsified method you point out here pre-dates the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617
>>
>> It might be worthwhile cleaning this up. Not having to route via SharedSecrets -> JavaLangAccess does speed things up during startup/interpretation, at the cost of some code duplication.
>
> This looks a really good change and it's great can these decoders get the benefit of the instrinics work.
>
> ISO_8869_1.decodeArrayLoop using JLA.inflate looks a bit strange and maybe we can rename the method in the shared secret to inflatedCopy or just copy. or just add a comment. The reason is that code outside of java.lang shouldn't know anything about String's internals and inflation.
>
> The reformatting changes to StreamEncoder/StreamDecoder make it hard to see if there are any actual changes. Is there anything we should look at? It's okay to include this cleanup, I can't tell what happened with this source file.
>
> I want to study the change System.setJavaLangAccess a bit further to understand why this is needed (I know there is a awkward bootstrapping issue here).
>
> I expect Naoto will want to review this PR too.

I added the `Inflated copy from byte[] to char[], as defined by StringLatin1` comment to the `inflate` method in `JavaLangAccess`, but you're probably right and it would be good to make the name more explicit when exporting it outside of the package internal use. How about `inflateBytesToChars`?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths

Claes Redestad-2
In reply to this post by Alan Bateman-2
On Tue, 16 Feb 2021 15:05:33 GMT, Alan Bateman <[hidden email]> wrote:

> The reformatting changes to StreamEncoder/StreamDecoder make it hard to see if there are any actual changes. Is there anything we should look at? It's okay to include this cleanup, I can't tell what happened with this source file.
>

Apart from a few things being made final that weren't, and a flattening of a back-to-back Charset lookup in one of the constructors its mostly formatting cleanups.

> I want to study the change System.setJavaLangAccess a bit further to understand why this is needed (I know there is a awkward bootstrapping issue here).

Right, I'm not exactly sure why the more limited changes I attempted in 5f4e87f failed. In that change I simply changed the initialization order, which made the failing (closed) tests pass locally - but not in our CI. Since this is in initPhase1 there should be no concurrency possible.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Naoto Sato-2
In reply to this post by Claes Redestad-2
On Mon, 15 Feb 2021 15:19:01 GMT, Claes Redestad <[hidden email]> wrote:

>> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>>
>> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
>> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
>> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>>
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>
>   Revert rem assertions

Great improvement! Looks good to me overall. I wondered about the changes in JLA as Alan already mentioned.

src/java.base/share/classes/java/lang/String.java line 1017:

> 1015:      * @return the number of bytes successfully decoded, at most len
> 1016:      */
> 1017:     /* package-private */

Some more explanation would be helpful here, as it is accessed from System for shared secrets.

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 48:

> 46: import jdk.internal.module.ServicesCatalog;
> 47: import jdk.internal.reflect.ConstantPool;
> 48: import jdk.internal.vm.annotation.IntrinsicCandidate;

Not needed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v3]

Claes Redestad-2
In reply to this post by Claes Redestad-2
> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>
> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>
> Testing: tier1-3

Claes Redestad has updated the pull request incrementally with two additional commits since the last revision:

 - Copyrights
 - Review fixes

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2574/files
  - new: https://git.openjdk.java.net/jdk/pull/2574/files/b0bf8cfb..367be2a5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=01-02

  Stats: 15 lines in 10 files changed: 1 ins; 1 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2574.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2574/head:pull/2574

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Claes Redestad-2
In reply to this post by Naoto Sato-2
On Tue, 16 Feb 2021 19:48:09 GMT, Naoto Sato <[hidden email]> wrote:

>> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revert rem assertions
>
> src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 48:
>
>> 46: import jdk.internal.module.ServicesCatalog;
>> 47: import jdk.internal.reflect.ConstantPool;
>> 48: import jdk.internal.vm.annotation.IntrinsicCandidate;
>
> Not needed.

Fixed

> src/java.base/share/classes/java/lang/String.java line 1017:
>
>> 1015:      * @return the number of bytes successfully decoded, at most len
>> 1016:      */
>> 1017:     /* package-private */
>
> Some more explanation would be helpful here, as it is accessed from System for shared secrets.

Ok, added a short comment

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Alan Bateman-2
In reply to this post by Naoto Sato-2
On Tue, 16 Feb 2021 19:51:21 GMT, Naoto Sato <[hidden email]> wrote:

>> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revert rem assertions
>
> Great improvement! Looks good to me overall. I wondered about the changes in JLA as Alan already mentioned.

> Right, I'm not exactly sure why the more limited changes I attempted in [5f4e87f](https://github.com/openjdk/jdk/commit/5f4e87f50f49e64b8616063c176ea35632b0347e) failed. In that change I simply changed the initialization order, which made the failing (closed) tests pass locally - but not in our CI. Since this is in initPhase1 there should be no concurrency possible.

The Reference Handler thread is started by the initializer in jl.ref.Reference so could be a candidate. The Finalizer thread is another but this should VM.awaitInitLevel(1) and not touch JLA until initPhase1 is done.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Alan Bateman-2
On Wed, 17 Feb 2021 17:19:58 GMT, Alan Bateman <[hidden email]> wrote:

>> Great improvement! Looks good to me overall. I wondered about the changes in JLA as Alan already mentioned.
>
>> Right, I'm not exactly sure why the more limited changes I attempted in [5f4e87f](https://github.com/openjdk/jdk/commit/5f4e87f50f49e64b8616063c176ea35632b0347e) failed. In that change I simply changed the initialization order, which made the failing (closed) tests pass locally - but not in our CI. Since this is in initPhase1 there should be no concurrency possible.
>
> The Reference Handler thread is started by the initializer in jl.ref.Reference so could be a candidate. The Finalizer thread is another but this should VM.awaitInitLevel(1) and not touch JLA until initPhase1 is done.

> but you're probably right and it would be good to make the name more explicit when exporting it outside of the package internal use. How about `inflateBytesToChars`?

That should be okay.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Philippe Marschall-2
On Wed, 17 Feb 2021 17:22:21 GMT, Alan Bateman <[hidden email]> wrote:

>>> Right, I'm not exactly sure why the more limited changes I attempted in [5f4e87f](https://github.com/openjdk/jdk/commit/5f4e87f50f49e64b8616063c176ea35632b0347e) failed. In that change I simply changed the initialization order, which made the failing (closed) tests pass locally - but not in our CI. Since this is in initPhase1 there should be no concurrency possible.
>>
>> The Reference Handler thread is started by the initializer in jl.ref.Reference so could be a candidate. The Finalizer thread is another but this should VM.awaitInitLevel(1) and not touch JLA until initPhase1 is done.
>
>> but you're probably right and it would be good to make the name more explicit when exporting it outside of the package internal use. How about `inflateBytesToChars`?
>
> That should be okay.

> > > Is there a reason `sun.nio.cs.ISO_8859_1.Encoder#implEncodeISOArray(char[], int, byte[], int, int)` wasn't moved to `JavaLangAccess` as well?
> >
> >
> > Exposing StringUTF16.compress for Latin-1 and ASCII-compatible encoders seem very reasonable, which I was thinking of exploring next as a separate RFE.
>
> Maybe I misunderstood. The intrinsified method you point out here pre-dates the work in JDK 9 to similarly intrinsify char[]->byte[] compaction in StringUTF16, see https://bugs.openjdk.java.net/browse/JDK-6896617
>
> It might be worthwhile cleaning this up. Not having to route via SharedSecrets -> JavaLangAccess does speed things up during startup/interpretation, at the cost of some code duplication.

My understanding was ISO_8859_1$Encoder.implEncodeISOArray and StringUTF16.compress are ultimately hooked up to the same intrinsic. I find it inconsistent that ISO_8859_1$Encoder access an encoding intrinsitc directly while ISO_8859_1$Decoder and others access a decoding intrinsic indirectly through JavaLangAccess. I realize this RFE is about decoding so keeping encoding to a different RFE may indeed be better.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v4]

Claes Redestad-2
In reply to this post by Claes Redestad-2
> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>
> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>
> Testing: tier1-3

Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:

  Revert JavaLangAccessImpl changes

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2574/files
  - new: https://git.openjdk.java.net/jdk/pull/2574/files/367be2a5..e2316007

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2574&range=02-03

  Stats: 176 lines in 2 files changed: 6 ins; 10 del; 160 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2574.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2574/head:pull/2574

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Claes Redestad-2
In reply to this post by Alan Bateman-2
On Wed, 17 Feb 2021 17:19:58 GMT, Alan Bateman <[hidden email]> wrote:

> > Right, I'm not exactly sure why the more limited changes I attempted in [5f4e87f](https://github.com/openjdk/jdk/commit/5f4e87f50f49e64b8616063c176ea35632b0347e) failed. In that change I simply changed the initialization order, which made the failing (closed) tests pass locally - but not in our CI. Since this is in initPhase1 there should be no concurrency possible.
>
> The Reference Handler thread is started by the initializer in jl.ref.Reference so could be a candidate. The Finalizer thread is another but this should VM.awaitInitLevel(1) and not touch JLA until initPhase1 is done.

Turns out the native code called by `SystemProps.initProperties();` can initialize a `CharsetDecoder` if `sun.jnu.encoding` is set to certain values, and this tripped up the initialization to cause `getJavaLangAccess` to be called before `setJavaLangAccess`. By moving `setJavaLangAccess` to the very start of `initPhase1` we avoid this possibility, and limit churn.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]

Alan Bateman-2
On Thu, 18 Feb 2021 23:16:54 GMT, Claes Redestad <[hidden email]> wrote:

> Turns out the native code called by `SystemProps.initProperties();` can initialize a `CharsetDecoder` if `sun.jnu.encoding` is set to certain values, and this tripped up the initialization to cause `getJavaLangAccess` to be called before `setJavaLangAccess`. By moving `setJavaLangAccess` to the very start of `initPhase1` we avoid this possibility, and limit churn.

I assume this is tests running with -Dfile.encoding. I can go along with moving setJavaLangAccess to the start of initPhase1. I'll do another pass over the updated patch today. Good work!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v4]

Alan Bateman-2
In reply to this post by Claes Redestad-2
On Thu, 18 Feb 2021 23:16:00 GMT, Claes Redestad <[hidden email]> wrote:

>> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>>
>> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
>> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
>> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>>
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>
>   Revert JavaLangAccessImpl changes

Marked as reviewed by alanb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v4]

Naoto Sato-2
In reply to this post by Claes Redestad-2
On Thu, 18 Feb 2021 23:16:00 GMT, Claes Redestad <[hidden email]> wrote:

>> This patch exposes a couple of intrinsics used by String to speed up ASCII checking and byte[] -> char[] inflation, which can be used by latin1 and ASCII-compatible CharsetDecoders to speed up decoding operations.
>>
>> - Fast-path implemented for all standard charsets, with up to 10x performance improvements in microbenchmarks reading Strings from ByteArrayInputStream.
>> - Cleanup of StreamDecoder/-Encoder with some minor improvements when interpreting
>> - Reworked creation of JavaLangAccess to be safely published for CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and these encoders are created during System.initPhase1 the current sequence caused the initialization to became unstable and a few tests were consistently getting an NPE.
>>
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>
>   Revert JavaLangAccessImpl changes

Marked as reviewed by naoto (Reviewer).

src/java.base/share/classes/java/lang/System.java line 1988:

> 1986:         // might initialize CharsetDecoders that rely on it
> 1987:         setJavaLangAccess();
> 1988:

Good to see the init issue has been resolved with a simple reordering.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2574
12