8302877: Speed up latin1 case conversions #12623

eirbjo · 2023-02-17T17:31:09Z

This PR suggests we speed up Character.toUpperCase and Character.toLowerCase for latin1 code points by applying the 'oldest ASCII trick in the book'.

This takes advantage of the fact that latin1 uppercase code points are always 0x20 lower than their lowercase (with the exception of two code points which uppercase out of latin1).

To verify the correctness of the new implementation, the test Latin1CaseConversion is added with an exhaustive verification of toUpperCase/toLowerCase for all latin1 code points.

The implementation needs to balance the performance of the various ranges in latin1. An effort has been made to favour operations on ASCII code points, without causing excessive regression for higher code points.

Performance is benchmarked for 7 chosen sample code points, each representing a range or a special-case. Results in the first comment.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8302877: Speed up latin1 case conversions

Reviewers

Naoto Sato (@naotoj - Reviewer)
Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to 70c624d7

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12623/head:pull/12623
$ git checkout pull/12623

Update a local copy of the PR:
$ git checkout pull/12623
$ git pull https://git.openjdk.org/jdk pull/12623/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12623

View PR using the GUI difftool:
$ git pr show -t 12623

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12623.diff

…the 'oldest ASCII trick in the book'

eirbjo · 2023-02-17T17:31:48Z

Benchmark results:

Baseline:

Benchmark                                 (codePoint)  Mode  Cnt  Score   Error  Units
Characters.Latin1CaseConversion.toLowerCase          low  avgt   15  1.267 ± 0.013  ns/op
Characters.Latin1CaseConversion.toLowerCase            A  avgt   15  1.657 ± 0.011  ns/op
Characters.Latin1CaseConversion.toLowerCase            a  avgt   15  1.258 ± 0.005  ns/op
Characters.Latin1CaseConversion.toLowerCase      A-grave  avgt   15  1.656 ± 0.011  ns/op
Characters.Latin1CaseConversion.toLowerCase      a-grave  avgt   15  1.270 ± 0.023  ns/op
Characters.Latin1CaseConversion.toLowerCase           mu  avgt   15  1.261 ± 0.006  ns/op
Characters.Latin1CaseConversion.toLowerCase           yD  avgt   15  1.260 ± 0.005  ns/op
Characters.Latin1CaseConversion.toUpperCase          low  avgt   15  1.284 ± 0.043  ns/op
Characters.Latin1CaseConversion.toUpperCase            A  avgt   15  1.264 ± 0.008  ns/op
Characters.Latin1CaseConversion.toUpperCase            a  avgt   15  1.818 ± 0.016  ns/op
Characters.Latin1CaseConversion.toUpperCase      A-grave  avgt   15  1.261 ± 0.015  ns/op
Characters.Latin1CaseConversion.toUpperCase      a-grave  avgt   15  1.822 ± 0.013  ns/op
Characters.Latin1CaseConversion.toUpperCase           mu  avgt   15  1.823 ± 0.006  ns/op
Characters.Latin1CaseConversion.toUpperCase           yD  avgt   15  1.822 ± 0.008  ns/op

PR:

Benchmark                                 (codePoint)  Mode  Cnt  Score   Error  Units
Characters.Latin1CaseConversion.toLowerCase          low  avgt   15  0.878 ± 0.005  ns/op
Characters.Latin1CaseConversion.toLowerCase            A  avgt   15  1.038 ± 0.009  ns/op
Characters.Latin1CaseConversion.toLowerCase            a  avgt   15  1.036 ± 0.007  ns/op
Characters.Latin1CaseConversion.toLowerCase      A-grave  avgt   15  1.357 ± 0.015  ns/op
Characters.Latin1CaseConversion.toLowerCase      a-grave  avgt   15  1.352 ± 0.003  ns/op
Characters.Latin1CaseConversion.toLowerCase           mu  avgt   15  1.273 ± 0.002  ns/op
Characters.Latin1CaseConversion.toLowerCase           yD  avgt   15  1.352 ± 0.004  ns/op
Characters.Latin1CaseConversion.toUpperCase          low  avgt   15  0.880 ± 0.013  ns/op
Characters.Latin1CaseConversion.toUpperCase            A  avgt   15  0.920 ± 0.071  ns/op
Characters.Latin1CaseConversion.toUpperCase            a  avgt   15  1.055 ± 0.013  ns/op
Characters.Latin1CaseConversion.toUpperCase      A-grave  avgt   15  1.394 ± 0.010  ns/op
Characters.Latin1CaseConversion.toUpperCase      a-grave  avgt   15  1.391 ± 0.009  ns/op
Characters.Latin1CaseConversion.toUpperCase           mu  avgt   15  1.597 ± 0.021  ns/op
Characters.Latin1CaseConversion.toUpperCase           yD  avgt   15  1.354 ± 0.003  ns/op

bridgekeeper · 2023-02-17T17:32:08Z

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-02-17T17:34:01Z

@eirbjo The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

naotoj

Looks good to me. I'd rather not use "case folding", as to me it implies "normalizing" but this is simply lowercasing/uppercasing.

naotoj · 2023-02-18T00:55:31Z

test/jdk/java/lang/Character/Latin1CaseFolding.java

+
+/**
+ * @test
+ * @summary Provides exchaustive verification of Character.toUpperCase and Character.toLowerCase


typo: "exhaustive"?

I did an 'exchaustive' search for 'exchaustive' across the code base and found two comments in LocaleData and LocaleData.cldr in jdk/test/jdk/sun/text/resources.

Would you like me to update these as well while we're here, or should we avoid getting out scope for this PR?

I'd appreciate it. I don't mind fixing it with this PR.

Thanks Naoto, I have fixed the spelling in these two unrelated files.

…ro Sign' and 'y with Diaeresis'

… as 'Micro Sign' ("Mu" is the Unicode code point it uppercases to)

eirbjo · 2023-02-18T06:44:31Z

I'd rather not use "case folding", as to me it implies "normalizing" but this is simply lowercasing/uppercasing.

I guess I was looking for a generic term for uppercase/lowercase. I picked "case conversion" instead.

eirbjo · 2023-02-20T14:41:35Z

/issue 8302877

openjdk · 2023-02-20T14:42:46Z

@eirbjo The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

mlbridge · 2023-02-20T14:47:32Z

Webrevs

eirbjo · 2023-02-21T09:33:07Z

A site note: Early and crude experiements using the Vector API indicate that the 'oldest ASCII trick in the book' vectorizes pretty well.

Here's a benchmark comparing the strings "helloworld" and "HelloWorld" repeated 1024 times, followed by either 'A' or 'B' (to force a an expensive mismatch):

Benchmark                    (size)  Mode  Cnt     Score    Error  Units
EqualsIgnoreCase.scalar        1024  avgt   15  6225.624 ± 89.182  ns/op
EqualsIgnoreCase.vectorized    1024  avgt   15  1246.110 ± 14.767  ns/op

I have the feeling that most case-insensitive comparisons are pretty short, so not sure how useful this is IRL.

eirbjo · 2023-02-21T10:00:59Z

I have the feeling that most case-insensitive comparisons are pretty short, so not sure how useful this is IRL.

There seems to be a win from strings of size 32 bytes upwards. (That's probably longer than most keys in TreeMaps using String.CASE_INSENSITIVE_ORDER, such as j.n.h.HttpHeaders)

Benchmark                    (size)  Mode  Cnt   Score   Error  Units
EqualsIgnoreCase.scalar          16  avgt    2  20.608          ns/op
EqualsIgnoreCase.scalar          32  avgt    2  36.510          ns/op
EqualsIgnoreCase.vectorized      16  avgt    2  18.601          ns/op
EqualsIgnoreCase.vectorized      32  avgt    2  12.795          ns/op

This is outside scope for this PR, I just wanted to leave a trace of this observation here for future record.

For reference, I suggested this benchmark for inclusion in the Vector API benchmark:

https://mail.openjdk.org/pipermail/panama-dev/2023-February/018709.html

cl4es

Looks good. Some nits inline

cl4es · 2023-02-21T10:29:24Z

src/java.base/share/classes/java/lang/CharacterDataLatin1.java.template

        }
-        return mapChar;
+        int l = ch | 0x20; // Lowercase using 'oldest ASCII trick in the book'
+        if ( l <= 'z' // In range a-z


Suggested change

if ( l <= 'z' // In range a-z

if (l <= 'z' // In range a-z

Fixed! (My IDE does not highlight this code, making it a bit harder to spot mistakes like this)

cl4es · 2023-02-21T10:36:08Z

test/micro/org/openjdk/bench/java/lang/Characters.java

+    @Warmup(iterations = 5, time = 1)
+    @Measurement(iterations = 5, time = 1)
+    @Fork(3)
+    public static class Latin1CaseConversions {


Not sure if qualifying this as "Latin1" is necessary, even though that's what you've focused on for this PR. We could easily add some codePoints outside of the latin1 range (now or later) without changing the test.

While having a switch with some readable names is a nice touch I think we should additionally allow integer codePoint as-is to keep it in line with the outer class, e.g. default -> Integer.parseInt(codePoint);

You are probably right that Latin1 is a bit narrow here, removing the prefix.

I added Integer.parseInt as the default, good idea!

openjdk · 2023-02-21T10:38:38Z

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8302877: Speed up latin1 case conversions

Reviewed-by: naoto, redestad

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 216 new commits pushed to the master branch:

1ea5f9f: 8302789: (fs) Files.copy should include unsupported copy option in exception message
dfce4e1: 8302800: Augment NaN handling tests of FDLIBM methods
5489c82: 8302603: Use Set.of in java.nio.charset.Charset
10b4cc9: 8301627: System.exit and Runtime.exit debug logging
db483a3: 8302320: AsyncGetCallTrace obtains too few frames in sanity test
02eb240: 8302846: IGV: Zoom stuck when zooming out on large graphs
92dfa11: 8302863: Speed up String::encodeASCII using countPositives
8b20aa9: 8302981: Fix a typo in the doc comment for java.lang.Record.equals
644fe0a: 8302837: Kernel32.cpp array memory release invokes undefined behaviour
60e6378: 8302977: ZGC: Doesn't support gc/TestVerifySubSet.java
... and 206 more: https://git.openjdk.org/jdk/compare/09b8a1959771213cb982d062f0a913285e4a0c6e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@naotoj, @cl4es) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

eirbjo · 2023-02-21T11:19:52Z

Thanks for your review and JBS juggling, Claes!

I'll wait for a final word from @naotoj before integrating.

naotoj

LGTM. Thanks for the fix!

eirbjo · 2023-02-21T20:47:35Z

/integrate

openjdk · 2023-02-21T20:48:35Z

@eirbjo
Your change (at version bff999c) is now ready to be sponsored by a Committer.

naotoj · 2023-02-21T20:53:34Z

/sponsor

openjdk · 2023-02-21T20:54:48Z

Going to push as commit ef1f7bd.
Since your change was applied there have been 216 commits pushed to the master branch:

1ea5f9f: 8302789: (fs) Files.copy should include unsupported copy option in exception message
dfce4e1: 8302800: Augment NaN handling tests of FDLIBM methods
5489c82: 8302603: Use Set.of in java.nio.charset.Charset
10b4cc9: 8301627: System.exit and Runtime.exit debug logging
db483a3: 8302320: AsyncGetCallTrace obtains too few frames in sanity test
02eb240: 8302846: IGV: Zoom stuck when zooming out on large graphs
92dfa11: 8302863: Speed up String::encodeASCII using countPositives
8b20aa9: 8302981: Fix a typo in the doc comment for java.lang.Record.equals
644fe0a: 8302837: Kernel32.cpp array memory release invokes undefined behaviour
60e6378: 8302977: ZGC: Doesn't support gc/TestVerifySubSet.java
... and 206 more: https://git.openjdk.org/jdk/compare/09b8a1959771213cb982d062f0a913285e4a0c6e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-21T20:55:06Z

@naotoj @eirbjo Pushed as commit ef1f7bd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

dholmes-ora · 2023-02-21T21:15:33Z

The testcase change is failing to compile:

open/test/micro/org/openjdk/bench/java/lang/Characters.java:115: error: case, default, or '}' expected

eirbjo · 2023-02-21T21:22:11Z

The testcase change is failing to compile:

open/test/micro/org/openjdk/bench/java/lang/Characters.java:115: error: case, default, or '}' expected

Darn, this was rather embarrassing!

Since this was just integrated, I guess I'll need to open a new PR for the fix?

naotoj · 2023-02-21T21:27:09Z

Yes please.

eirbjo · 2023-02-21T21:28:14Z

I created #12701 for this.

Could someone please file a JBS?

naotoj · 2023-02-21T21:29:30Z

https://bugs.openjdk.org/browse/JDK-8303033

eirbjo · 2023-02-21T21:33:38Z

Thanks, #12701 is ready for review.

Speed up Character.toUpperCase and Character.toLowerCase by applying …

4662482

…the 'oldest ASCII trick in the book'

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Feb 17, 2023

Adjust whitespace

a414a55

eirbjo marked this pull request as ready for review February 17, 2023 17:36

eirbjo mentioned this pull request Feb 17, 2023

(Replaced) Speed up case-insensitive string comparison #12586

Closed

3 tasks

naotoj reviewed Feb 18, 2023

View reviewed changes

eirbjo added 3 commits February 18, 2023 07:24

Improve comments for the two special-cased uppercase code points 'Mic…

62bd9f2

…ro Sign' and 'y with Diaeresis'

Prefer the term "case conversion" over ""case folding". Refer to 0xB5…

e280909

… as 'Micro Sign' ("Mu" is the Unicode code point it uppercases to)

Correct spelling for "exhaustive"

4e169c8

eirbjo changed the title ~~Speed up latin1 case folding~~ Speed up latin1 case transformation Feb 18, 2023

eirbjo changed the title ~~Speed up latin1 case transformation~~ Speed up latin1 case conversions Feb 18, 2023

eirbjo added 2 commits February 20, 2023 15:37

Improved whitespace alignment for Param and switch values

67c50a0

Add @bug tag to test

57a27d3

openjdk bot changed the title ~~Speed up latin1 case conversions~~ 8302877: Speed up latin1 case conversions Feb 20, 2023

openjdk bot added the rfr Pull request is ready for review label Feb 20, 2023

cl4es mentioned this pull request Feb 20, 2023

8302871: Speed up StringLatin1.regionMatchesCI #12632

Closed

3 tasks

Spell fix for 'exhaustive' in comments in sun/text/resources

70c624d

cl4es approved these changes Feb 21, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 21, 2023

eirbjo added 3 commits February 21, 2023 12:03

Remove a whitespace following 'if ('

7c31a49

Rename Latin1CaseConversions to just CaseConversions

c32fdb2

Allow any integer codePoint by defaulting to Integer.parseInt

bff999c

naotoj approved these changes Feb 21, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 21, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 21, 2023

openjdk bot closed this Feb 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 21, 2023

eirbjo mentioned this pull request Feb 21, 2023

8303033: Build failure with the micro bench mark #12701

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8302877: Speed up latin1 case conversions #12623

8302877: Speed up latin1 case conversions #12623

eirbjo commented Feb 17, 2023 •

edited by openjdk bot

Loading

eirbjo commented Feb 17, 2023 •

edited

Loading

bridgekeeper bot commented Feb 17, 2023

openjdk bot commented Feb 17, 2023

naotoj left a comment

naotoj Feb 18, 2023

eirbjo Feb 18, 2023 •

edited

Loading

naotoj Feb 21, 2023

eirbjo Feb 21, 2023

eirbjo commented Feb 18, 2023

eirbjo commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

mlbridge bot commented Feb 20, 2023 •

edited

Loading

eirbjo commented Feb 21, 2023 •

edited

Loading

eirbjo commented Feb 21, 2023 •

edited

Loading

cl4es left a comment

cl4es Feb 21, 2023

eirbjo Feb 21, 2023

cl4es Feb 21, 2023

eirbjo Feb 21, 2023

openjdk bot commented Feb 21, 2023 •

edited

Loading

eirbjo commented Feb 21, 2023

naotoj left a comment

eirbjo commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

naotoj commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

dholmes-ora commented Feb 21, 2023

eirbjo commented Feb 21, 2023

naotoj commented Feb 21, 2023

eirbjo commented Feb 21, 2023

naotoj commented Feb 21, 2023

eirbjo commented Feb 21, 2023

8302877: Speed up latin1 case conversions #12623

8302877: Speed up latin1 case conversions #12623

Conversation

eirbjo commented Feb 17, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

eirbjo commented Feb 17, 2023 • edited Loading

bridgekeeper bot commented Feb 17, 2023

openjdk bot commented Feb 17, 2023

naotoj left a comment

Choose a reason for hiding this comment

naotoj Feb 18, 2023

Choose a reason for hiding this comment

eirbjo Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

naotoj Feb 21, 2023

Choose a reason for hiding this comment

eirbjo Feb 21, 2023

Choose a reason for hiding this comment

eirbjo commented Feb 18, 2023

eirbjo commented Feb 20, 2023

openjdk bot commented Feb 20, 2023

mlbridge bot commented Feb 20, 2023 • edited Loading

Webrevs

eirbjo commented Feb 21, 2023 • edited Loading

eirbjo commented Feb 21, 2023 • edited Loading

cl4es left a comment

Choose a reason for hiding this comment

cl4es Feb 21, 2023

Choose a reason for hiding this comment

eirbjo Feb 21, 2023

Choose a reason for hiding this comment

cl4es Feb 21, 2023

Choose a reason for hiding this comment

eirbjo Feb 21, 2023

Choose a reason for hiding this comment

openjdk bot commented Feb 21, 2023 • edited Loading

eirbjo commented Feb 21, 2023

naotoj left a comment

Choose a reason for hiding this comment

eirbjo commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

naotoj commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

openjdk bot commented Feb 21, 2023

dholmes-ora commented Feb 21, 2023

eirbjo commented Feb 21, 2023

naotoj commented Feb 21, 2023

eirbjo commented Feb 21, 2023

naotoj commented Feb 21, 2023

eirbjo commented Feb 21, 2023

eirbjo commented Feb 17, 2023 •

edited by openjdk bot

Loading

eirbjo commented Feb 17, 2023 •

edited

Loading

eirbjo Feb 18, 2023 •

edited

Loading

mlbridge bot commented Feb 20, 2023 •

edited

Loading

eirbjo commented Feb 21, 2023 •

edited

Loading

eirbjo commented Feb 21, 2023 •

edited

Loading

openjdk bot commented Feb 21, 2023 •

edited

Loading