On Wed, 23 Feb 2022 14:19:20 GMT, Claes Redestad <redes...@openjdk.org> 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 with a new target base due to a > merge or a rebase. The pull request now contains 29 commits: > > - Resolve merge conflict > - Fix TestCountPositives to correctly allow 0 return when expected != len > (for now) > - aarch64: fix issue with short inputs divisible by wordSize > - Switch aarch64 intrinsic to a variant of countPositives returning len or > zero as a first step. > - Revert micro changes, split out to #7516 > - Merge branch 'master' of https://github.com/cl4es/jdk into count_positives > - Merge branch 'master' into count_positives > - Restore partial vector checks in AVX2 and SSE intrinsic variants > - Let countPositives use hasNegatives to allow ports not implementing the > countPositives intrinsic to stay neutral > - Simplify changes to encodeUTF8 > - ... and 19 more: > https://git.openjdk.java.net/jdk/compare/5035bf5e...685795ce > Making the `bottom_type()` of `CountPositivesNode` more precise (`TypeInt::INT` -> `TypeInt::POS`) might help, then. Seems like something we want to do regardless. ------------- PR: https://git.openjdk.java.net/jdk/pull/7231
