Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v15]
On Mon, 14 Mar 2022 20:30:51 GMT, Roger Riggs wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix copyright year in new test > > core libs String.java changes look fine. Thanks @RogerRiggs I intend to push this soon regardless, but would appreciate an explicit review of the aarch64 changes from an aarch64 maintainer. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v15]
On Wed, 9 Mar 2022 23:59:32 GMT, Claes Redestad wrote: >> I'm requesting comments and, hopefully, some help with this patch to replace >> `StringCoding.hasNegatives` with `countPositives`. The new method does a >> very similar pass, but alters the intrinsic to return the number of leading >> bytes in the `byte[]` range which only has positive bytes. This allows for >> dealing much more efficiently with those `byte[]`s that has a ASCII prefix, >> with no measurable cost on ASCII-only or latin1/UTF16-mostly input. >> >> Microbenchmark results: >> https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 >> >> - Only implemented on x86 for now, but I want to verify that implementations >> of `countPositives` can be implemented with similar efficiency on all >> platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) >> before moving ahead. This pretty much means holding up this until it's >> implemented on all platforms, which can either contributed to this PR or as >> dependent follow-ups. >> >> - An alternative to holding up until all platforms are on board is to allow >> the implementation of `StringCoding.hasNegatives` and `countPositives` to be >> implemented so that the non-intrinsified method calls into the intrinsified. >> This requires structuring the implementations differently based on which >> intrinsic - if any - is actually implemented. One way to do this could be to >> mimic how `java.nio` handles unaligned accesses and expose which intrinsic >> is available via `Unsafe` into a `static final` field. >> >> - There are a few minor regressions (~5%) in the x86 implementation on >> `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, >> for example `encode-/decodeShortMixed` even see a minor improvement, which >> makes me consider those corner case regressions with little real world >> implications (if you have latin1 Strings, you're likely to also have >> ASCII-only strings in your mix). > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copyright year in new test core libs String.java changes look fine. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v15]
On Wed, 9 Mar 2022 23:59:32 GMT, Claes Redestad wrote: >> I'm requesting comments and, hopefully, some help with this patch to replace >> `StringCoding.hasNegatives` with `countPositives`. The new method does a >> very similar pass, but alters the intrinsic to return the number of leading >> bytes in the `byte[]` range which only has positive bytes. This allows for >> dealing much more efficiently with those `byte[]`s that has a ASCII prefix, >> with no measurable cost on ASCII-only or latin1/UTF16-mostly input. >> >> Microbenchmark results: >> https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 >> >> - Only implemented on x86 for now, but I want to verify that implementations >> of `countPositives` can be implemented with similar efficiency on all >> platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) >> before moving ahead. This pretty much means holding up this until it's >> implemented on all platforms, which can either contributed to this PR or as >> dependent follow-ups. >> >> - An alternative to holding up until all platforms are on board is to allow >> the implementation of `StringCoding.hasNegatives` and `countPositives` to be >> implemented so that the non-intrinsified method calls into the intrinsified. >> This requires structuring the implementations differently based on which >> intrinsic - if any - is actually implemented. One way to do this could be to >> mimic how `java.nio` handles unaligned accesses and expose which intrinsic >> is available via `Unsafe` into a `static final` field. >> >> - There are a few minor regressions (~5%) in the x86 implementation on >> `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, >> for example `encode-/decodeShortMixed` even see a minor improvement, which >> makes me consider those corner case regressions with little real world >> implications (if you have latin1 Strings, you're likely to also have >> ASCII-only strings in your mix). > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix copyright year in new test Gentle reminder that I need a review of the aarch64 changes. - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v15]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fix copyright year in new test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7231/files - new: https://git.openjdk.java.net/jdk/pull/7231/files/58ee73bb..bc5a8c80 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7231&range=13-14 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
