Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v7]
On Tue, 28 Sep 2021 11:49:28 GMT, Volker Simonis wrote: >> Does the changes in 9800a99 resolve your concerns? > > In principle yes, but shouldn't the condition read: > > if (!Matcher::match_rule_supported(Op_EncodeISOArray) || > !Matcher::supports_encode_ascii_array) return false; > > I.e. the intrinisc is supported if both conditions are true and not supported > if either one of them is false? Resolved in 47633cb - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v7]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Clean up some stale test comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/4ad9c08a..ebe509ef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=05-06 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v6]
On Wed, 29 Sep 2021 12:36:23 GMT, Claes Redestad wrote: >> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to >> work also for ASCII encoding, which makes for example the `UTF_8$Encoder` >> perform on par with (or outperform) similarly getting charset encoded bytes >> from a String. The former took a small performance hit in JDK 9, and the >> latter improved greatly in the same release. >> >> Extending the `EncodeIsoArray` intrinsics on other platforms should be >> possible, but I'm unfamiliar with the macro assembler in general and unlike >> the x86 intrinsic they don't use a simple vectorized mask to implement the >> latin-1 check. For example aarch64 seem to filter out the low bytes and then >> check if there's any bits set in the high bytes. Clever, but very different >> to the 0xFF80 2-byte mask that an ASCII test wants. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Clean up and make TestEncodeIntrinsics fail on the particular scenario > where the ISO intrinsic was used in place of the ASCII-only intrinsic The incremental change looks good and trivial to me. - Marked as reviewed by thartmann (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v6]
On Mon, 27 Sep 2021 12:27:32 GMT, Tobias Hartmann wrote: >>> Should we remove the "iso" part from the method/class names? >> >> I'm open to suggestions, but I've not been able to think of anything better. >> `encodeISOOrASCII` doesn't seem helpful and since ASCII is a subset of the >> ISO-8859-1 encoding referred to by the "iso" moniker then the ASCII-only >> variant is technically encoding chars to valid ISO-8859-1. > >> > Should we remove the "iso" part from the method/class names? >> >> I'm open to suggestions, but I've not been able to think of anything better. >> `encodeISOOrASCII` doesn't seem helpful and since ASCII is a subset of the >> ISO-8859-1 encoding referred to by the "iso" moniker then the ASCII-only >> variant is technically encoding chars to valid ISO-8859-1. > > Okay, that's fine with me. Thanks for reviewing, @TobiHartmann I also cleaned up the test and made sure it fails when there's a logic bug like the one I introduced in 9800a99 where the ISO array intrinsic would be matched for a case requiring the ASCII-only array intrinsic. The test was (in)conveniently never testing out-of-range characters in the 0x80-0xFF range, which is precisely where the two intrinsics would produce different results. I hope this doesn't require another 24 hour grace period. - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v6]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Clean up and make TestEncodeIntrinsics fail on the particular scenario where the ISO intrinsic was used in place of the ASCII-only intrinsic - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/b4a5d105..4ad9c08a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=04-05 Stats: 8 lines in 1 file changed: 1 ins; 1 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
On Tue, 28 Sep 2021 11:49:29 GMT, Claes Redestad wrote: >> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to >> work also for ASCII encoding, which makes for example the `UTF_8$Encoder` >> perform on par with (or outperform) similarly getting charset encoded bytes >> from a String. The former took a small performance hit in JDK 9, and the >> latter improved greatly in the same release. >> >> Extending the `EncodeIsoArray` intrinsics on other platforms should be >> possible, but I'm unfamiliar with the macro assembler in general and unlike >> the x86 intrinsic they don't use a simple vectorized mask to implement the >> latin-1 check. For example aarch64 seem to filter out the low bytes and then >> check if there's any bits set in the high bytes. Clever, but very different >> to the 0xFF80 2-byte mask that an ASCII test wants. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add fail-safe predicate to all encode_iso_array to only match non-ASCII > EncodeISOArrayNodes The latest version looks good to me. - Marked as reviewed by thartmann (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
Yes, this was spotted and fixed already. Annoyingly the test I added didn't detect this so GHA was green, but it failed some tier2 tests on aarch64. I added extra safeguards by predicating matching the encode_iso_array instructions on the node being !ascii, which will cause C2 to report an error rather than silently using the iso variant for ascii-only nodes. Hämta Outlook för Android<https://aka.ms/AAb9ysg> From: core-libs-dev on behalf of Volker Simonis Sent: Tuesday, September 28, 2021 1:53:22 PM To: core-libs-dev@openjdk.java.net ; hotspot-compiler-...@openjdk.java.net ; nio-...@openjdk.java.net Subject: Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5] On Tue, 28 Sep 2021 10:01:43 GMT, Claes Redestad wrote: >> Not too much work. I recently introduced platform-specific `matcher_*.hpp` >> files, so since then adding a boolean constant is easy (no need to muck with >> the .ad files). > > Does the changes in 9800a99 resolve your concerns? In principle yes, but shouldn't the condition read: if (!Matcher::match_rule_supported(Op_EncodeISOArray) || !Matcher::supports_encode_ascii_array) return false; I.e. the intrinisc is supported if both conditions are true and not supported if either one of them is false? - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
On Tue, 28 Sep 2021 10:01:43 GMT, Claes Redestad wrote: >> Not too much work. I recently introduced platform-specific `matcher_*.hpp` >> files, so since then adding a boolean constant is easy (no need to muck with >> the .ad files). > > Does the changes in 9800a99 resolve your concerns? In principle yes, but shouldn't the condition read: if (!Matcher::match_rule_supported(Op_EncodeISOArray) || !Matcher::supports_encode_ascii_array) return false; I.e. the intrinisc is supported if both conditions are true and not supported if either one of them is false? - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v5]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add fail-safe predicate to all encode_iso_array to only match non-ASCII EncodeISOArrayNodes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/47633cba..b4a5d105 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=03-04 Stats: 3 lines in 3 files changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v4]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fix logic error - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/9800a99a..47633cba Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
On Mon, 27 Sep 2021 12:11:22 GMT, Claes Redestad wrote: >> src/hotspot/share/opto/c2compiler.cpp line 222: >> >>> 220: #if !defined(X86) >>> 221: return false; // not yet implemented >>> 222: #endif >> >> It might be a little more work, but I think it's cleaner to move the >> decision whether the intrinisc is supported into the Matcher like for most >> other intrinsics and keep this code here platform independent. Otherwise we >> will get an increasing cascade of ifdefs as people start implementing this >> for other platforms. > > Not too much work. I recently introduced platform-specific `matcher_*.hpp` > files, so since then adding a boolean constant is easy (no need to muck with > the .ad files). Does the changes in 9800a99 resolve your concerns? - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add Matcher predicate to avoid changing shared code as non-x86 platforms implements support for the _encodeAsciiArray intrinsic - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/12ab6ff5..9800a99a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=01-02 Stats: 17 lines in 6 files changed: 14 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
On Mon, 27 Sep 2021 11:59:54 GMT, Claes Redestad wrote: > > Should we remove the "iso" part from the method/class names? > > I'm open to suggestions, but I've not been able to think of anything better. > `encodeISOOrASCII` doesn't seem helpful and since ASCII is a subset of the > ISO-8859-1 encoding referred to by the "iso" moniker then the ASCII-only > variant is technically encoding chars to valid ISO-8859-1. Okay, that's fine with me. - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
On Mon, 27 Sep 2021 09:34:21 GMT, Volker Simonis wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add Matcher predicate to avoid changing shared code as non-x86 platforms >> implements support for the _encodeAsciiArray intrinsic > > src/hotspot/share/opto/c2compiler.cpp line 222: > >> 220: #if !defined(X86) >> 221: return false; // not yet implemented >> 222: #endif > > It might be a little more work, but I think it's cleaner to move the decision > whether the intrinisc is supported into the Matcher like for most other > intrinsics and keep this code here platform independent. Otherwise we will > get an increasing cascade of ifdefs as people start implementing this for > other platforms. Not too much work. I recently introduced platform-specific `matcher_*.hpp` files, so since then adding a boolean constant is easy (no need to muck with the .ad files). - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v2]
On Mon, 27 Sep 2021 06:36:50 GMT, Tobias Hartmann wrote: > Should we remove the "iso" part from the method/class names? I'm open to suggestions, but I've not been able to think of anything better. `encodeISOOrASCII` doesn't seem helpful and since ASCII is a subset of the ISO-8859-1 encoding referred to by the "iso" moniker then the ASCII-only variant is technically encoding chars to valid ISO-8859-1. - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v2]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Tobias review - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/8edc228f..12ab6ff5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=00-01 Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote: > This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. src/hotspot/share/opto/c2compiler.cpp line 222: > 220: #if !defined(X86) > 221: return false; // not yet implemented > 222: #endif It might be a little more work, but I think it's cleaner to move the decision whether the intrinisc is supported into the Matcher like for most other intrinsics and keep this code here platform independent. Otherwise we will get an increasing cascade of ifdefs as people start implementing this for other platforms. - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote:
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to
> work also for ASCII encoding, which makes for example the `UTF_8$Encoder`
> perform on par with (or outperform) similarly getting charset encoded bytes
> from a String. The former took a small performance hit in JDK 9, and the
> latter improved greatly in the same release.
>
> Extending the `EncodeIsoArray` intrinsics on other platforms should be
> possible, but I'm unfamiliar with the macro assembler in general and unlike
> the x86 intrinsic they don't use a simple vectorized mask to implement the
> latin-1 check. For example aarch64 seem to filter out the low bytes and then
> check if there's any bits set in the high bytes. Clever, but very different
> to the 0xFF80 2-byte mask that an ASCII test wants.
Very nice. The changes look good to me, just added some minor comments.
Should we remove the "iso" part from the method/class names?
src/hotspot/cpu/x86/x86_32.ad line 12218:
> 12216: instruct encode_ascii_array(eSIRegP src, eDIRegP dst, eDXRegI len,
> 12217: regD tmp1, regD tmp2, regD tmp3, regD tmp4,
> 12218: eCXRegI tmp5, eAXRegI result, eFlagsReg cr)
> %{
Indentation is wrong.
src/hotspot/cpu/x86/x86_32.ad line 12223:
> 12221: effect(TEMP tmp1, TEMP tmp2, TEMP tmp3, TEMP tmp4, USE_KILL src,
> USE_KILL dst, USE_KILL len, KILL tmp5, KILL cr);
> 1:
> 12223: format %{ "Encode array $src,$dst,$len -> $result// KILL ECX,
> EDX, $tmp1, $tmp2, $tmp3, $tmp4, ESI, EDI " %}
You might want to change the opto assembly comment to "Encode ascii array" (and
to "Encode iso array" above). Same on 64-bit.
src/hotspot/share/opto/intrinsicnode.hpp line 171:
> 169:
> 170:
> //--EncodeISOArray
> 171: // encode char[] to byte[] in ISO_8859_1
Comment should be adjusted to `... in ISO_8859_1 or ASCII`.
-
Marked as reviewed by thartmann (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote: > This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. x86 part of changes look good. - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote: > This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. core library part of the changes looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Thu, 23 Sep 2021 00:03:55 GMT, Claes Redestad wrote: > This can probably be simplified further, say by adding a flag to the > intrinsic of whether we're encoding ASCII only or ISO-8859-1. Done: Removed the addition of a new C2 Node, merged the macro assembler encode_iso_array and encode_ascii_array and added a predicate to select the behavior. > It also needs to be implemented and tested on all architectures. Implementing this on other hardware is Future Work. The non-x86 intrinsics for implEncodeISOArray all seem to use clever tricks rather than a simple mask that can be easily switched from detecting non-latin-1(0xFF00) to detecting ASCII (0xFF80). Clever tricks make it rather challenging to extend this like I could easily do in the x86 code (most all assembler is foreign to me) - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote: > This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. The current version (cef05f4) copies the ISO_8859_1.implEncodeISOArray intrinsic and adapts it to work on ASCII encoding, which makes the UTF_8$Encoder perform on par with (or outperform) encoding from a String. Using microbenchmarks provided by @carterkozak here: https://github.com/carterkozak/stringbuilder-encoding-performance Baseline: Benchmark (charsetName) (message) (timesToAppend) Mode Cnt ScoreError Units EncoderBenchmarks.charsetEncoder UTF-8 This is a simple ASCII message3 avgt8 270.237 ± 10.504 ns/op EncoderBenchmarks.charsetEncoder UTF-8 This is a message with unicode 3 avgt8 568.353 ± 2.331 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a simple ASCII message3 avgt8 324.889 ± 17.466 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a message with unicode 3 avgt8 633.720 ± 22.703 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a simple ASCII message3 avgt8 1132.436 ± 30.661 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a message with unicode 3 avgt8 1379.207 ± 66.982 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a simple ASCII message3 avgt891.253 ± 3.848 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a message with unicode 3 avgt8 519.489 ± 12.516 ns/op Patch: Benchmark (charsetName) (message) (timesToAppend) Mode Cnt Score Error Units EncoderBenchmarks.charsetEncoder UTF-8 This is a simple ASCII message3 avgt482.535 ± 20.310 ns/op EncoderBenchmarks.charsetEncoder UTF-8 This is a message with unicode 3 avgt4 522.679 ± 13.456 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a simple ASCII message3 avgt4 127.831 ± 32.612 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a message with unicode 3 avgt4 549.343 ± 59.899 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a simple ASCII message3 avgt4 1182.835 ± 153.735 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a message with unicode 3 avgt4 1416.407 ± 130.551 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a simple ASCII message3 avgt497.770 ± 15.742 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a message with unicode 3 avgt4 516.351 ± 58.580 ns/op This can probably be simplified further, say by adding a flag to the intrinsic of whether we're encoding ASCII only or ISO-8859-1. It also needs to be implemented and tested on all architectures. (edit: accidentally edit rather than quote-reply, restored original comment) On the JDK-included `CharsetEncodeDecode.encode` microbenchmark, I get these numbers in the baseline (18-b09): Benchmark (size) (type) Mode CntScore Error Units CharsetEncodeDecode.encode 16384UTF-8 avgt 30 39.962 ± 1.703 us/op CharsetEncodeDecode.encode 16384 BIG5 avgt 30 153.282 ± 4.521 us/op CharsetEncodeDecode.encode 16384 ISO-8859-15 avgt 30 192.040 ± 4.543 us/op
