8289908: Skip bounds check for cases when String is constructed from entirely used byte[] #9407

stsypanov · 2022-07-07T10:21:06Z

We can skip bounds check and null check for Charset in case we use the array entirely and the Charset is either default one or proven to be non-null.

Benchmark results:

before

Benchmark                                                  Mode  Cnt  Score   Error  Units
StringConstructor.newStringFromArray                       avgt   50  4,815 ± 0,154  ns/op
StringConstructor.newStringFromArrayWithCharset            avgt   50  4,462 ± 0,068  ns/op
StringConstructor.newStringFromArrayWithCharsetName        avgt   50  8,653 ± 0,040  ns/op
StringConstructor.newStringFromRangedArray                 avgt   50  5,090 ± 0,066  ns/op
StringConstructor.newStringFromRangedArrayWithCharset      avgt   50  4,550 ± 0,041  ns/op
StringConstructor.newStringFromRangedArrayWithCharsetName  avgt   50  8,080 ± 0,055  ns/op

after

Benchmark                                                  Mode  Cnt  Score   Error  Units
StringConstructor.newStringFromArray                       avgt   50  4,595 ± 0,053  ns/op
StringConstructor.newStringFromArrayWithCharset            avgt   50  4,038 ± 0,062  ns/op
StringConstructor.newStringFromArrayWithCharsetName        avgt   50  8,035 ± 0,031  ns/op
StringConstructor.newStringFromRangedArray                 avgt   50  4,084 ± 0,007  ns/op
StringConstructor.newStringFromRangedArrayWithCharset      avgt   50  4,014 ± 0,008  ns/op
StringConstructor.newStringFromRangedArrayWithCharsetName  avgt   50  7,466 ± 0,071  ns/op

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-8289908: Skip bounds check for cases when String is constructed from entirely used byte[]

Reviewers

Phil Race (@prrace - Reviewer) ⚠️ Review applies to b7375cd3
Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to f653a67b
Andrey Turbanov (@turbanoff - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9407

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

Using diff file

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

…entirely used byte[]

bridgekeeper · 2022-07-07T10:22:59Z

👋 Welcome back stsypanov! 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 · 2022-07-07T10:27:46Z

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

client
core-libs
security

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.

mlbridge · 2022-07-07T10:32:15Z

Webrevs

RogerRiggs · 2022-07-07T18:45:03Z

src/java.base/share/classes/java/lang/String.java

@@ -1424,7 +1426,7 @@ public String(byte[] bytes, Charset charset) {
     * @since  1.1
     */
    public String(byte[] bytes, int offset, int length) {
-        this(bytes, offset, length, Charset.defaultCharset());
+        this(bytes, offset, length, Charset.defaultCharset(), checkBoundsOffCount(offset, length, bytes.length));


Can you avoid the extra constructor by applying checkBoundOffCount to the offset argument; it returns the offset.

this(bytes, checkBoundsOffCount(offset, length, bytes.length), length, Charset.defaultCharset());

or call Preconditions.checkFromIndexSize directly.

But if I roll back the added constructor I'll go through existing public one public String(byte[] bytes, int offset, int length, Charset charset) doing bounds check twice, won't I?

The new constructor looks very odd, especially when it does not have an explanation and doesn't describe the required preconditions for calling it. Is there a better way than adding a non-functional argument?
The "unused" name is going to draw a warning from IntelliJ and some enterprising developer is going to try to remove it, not understanding why its there. And there is a bit of overhead pushing the extra arg.

The constructor should be private, it has a very narrow scope of use given the lack of checking its args.

It would be nice if the Hotspot compiler would recognize the llmits on the range and optimize away the checks;
it would have broader applicability then this point fix.
I would be interesting to ask the compiler folks if that's a future optimization.
These source changes make it harder to understand what's happening where; though that is sometimes work it for a noticeable performance improvement.

Well, we already have e.g. String(char[], int, int, Void) and String(AbstractStringBuilder asb, Void sig) where trailing argument is for disambiguation against private constructors. I did the same for mine and applied the same naming as in other trailing Void params.

Benchmark results after:

Benchmark Mode Cnt Score Error Units StringConstructor.newStringFromArray avgt 50 4,354 ± 0,195 ns/op StringConstructor.newStringFromArrayWithCharset avgt 50 4,035 ± 0,088 ns/op StringConstructor.newStringFromArrayWithCharsetName avgt 50 8,166 ± 0,062 ns/op StringConstructor.newStringFromRangedArray avgt 50 4,132 ± 0,054 ns/op StringConstructor.newStringFromRangedArrayWithCharset avgt 50 4,416 ± 0,206 ns/op StringConstructor.newStringFromRangedArrayWithCharsetName avgt 50 7,421 ± 0,041 ns/op

Matching the existing Void argument looks better.
The new private method should have a comment saying that it does not do any precondition checks on the arguments.

(Reordering the arguments is an alternative to adding an argument, for example, (Charset, byte[], int, int).
But it is less readable and can raise questions due to the different order of arguments.)

Updated PR. With newer version I get these results:

Benchmark Mode Cnt Score Error Units StringConstructor.newStringFromArray avgt 50 4,831 ± 0,205 ns/op StringConstructor.newStringFromArrayWithCharset avgt 50 3,940 ± 0,008 ns/op StringConstructor.newStringFromArrayWithCharsetName avgt 50 7,662 ± 0,112 ns/op StringConstructor.newStringFromRangedArray avgt 50 4,175 ± 0,065 ns/op StringConstructor.newStringFromRangedArrayWithCharset avgt 50 3,970 ± 0,037 ns/op StringConstructor.newStringFromRangedArrayWithCharsetName avgt 50 7,480 ± 0,014 ns/op

openjdk · 2022-07-12T22:29:11Z

⚠️ @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8289908
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2022-07-12T22:29:13Z

@stsypanov 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:

8289908: Skip bounds check for cases when String is constructed from entirely used byte[]

Reviewed-by: prr, rriggs, aturbanov

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 26 new commits pushed to the master branch:

c7c2066: 8290221: G1: Merge multiple calls of get_next_marked_addr in HeapRegion::oops_on_memregion_iterate_in_unparsable
be58cbc: 8290269: gc/shenandoah/TestVerifyJCStress.java fails due to invalid tag: required after JDK-8290023
109e21a: 8290080: G1: Remove unnecessary is-obj-dead check in HeapRegion::do_oops_on_memregion_in_humongous
adf40d2: 8290149: java/nio/file/Files/probeContentType/Basic.java fails on Windows Server 2019/2022
292d909: 8290178: failure_handler: run netstat without name lookups
a7f8358: Merge
ce36f6e: 8290203: ProblemList vmTestbase/nsk/jvmti/scenarios/capability/CM03/cm03t001/TestDescription.java on linux-all
fff7f35: 8290201: ProblemList com/sun/jdi/InvokeHangTest.java on macosx-x64 in vthread mode
128c6c6: 8290095: java/nio/channels/FileChannel/largeMemory/LargeGatheringWrite.java timed out
59d0c73: 8289930: Improve Thread description of inherited AccessControlContext
... and 16 more: https://git.openjdk.org/jdk/compare/6e18883d8ffd9a7b7d495da05e9859dc1d1a2677...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 (@prrace, @RogerRiggs, @turbanoff) 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).

RogerRiggs

Looks good, thanks for the updates.

src/java.base/share/classes/java/lang/String.java

stsypanov · 2022-07-14T07:19:26Z

/integrate

openjdk · 2022-07-14T07:21:12Z

@stsypanov
Your change (at version 72146f7) is now ready to be sponsored by a Committer.

prrace · 2022-07-18T17:13:25Z

/sponsor

openjdk · 2022-07-18T17:15:56Z

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

b2010a7: 8287805: Shenandoah: consolidate evacuate-update-root closures
ea8b75c: 8290334: Update FreeType to 2.12.1
6882f0e: 8290013: serviceability/jvmti/GetLocalVariable/GetLocalWithoutSuspendTest.java failed "assert(!in_vm) failed: Undersized StackShadowPages"
92067e2: 8290137: riscv: small refactoring for add_memory_int32/64
87340fd: 8288883: C2: assert(allow_address || t != T_ADDRESS) failed after JDK-8283091
bc7a1ea: 8288948: Few J2DBench tests indicate lower primitive drawing performance with metal rendering pipeline
84f2314: 8286030: Avoid JVM crash when containers share the same /tmp dir
4dd236b: 8290280: riscv: Clean up stack and register handling in interpreter
522b657: Merge
15d3329: 8281969: Bad result for the snippet @link tag if substring/regex consists of whitespace
... and 44 more: https://git.openjdk.org/jdk/compare/6e18883d8ffd9a7b7d495da05e9859dc1d1a2677...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-18T17:16:17Z

@prrace @stsypanov Pushed as commit efed7a7.

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

8289908: Skip bounds check for cases when String is constructed from …

567727e

…entirely used byte[]

stsypanov changed the title ~~Skip bounds check for cases when String is constructed from entirely used byte[]~~ 8289908: Skip bounds check for cases when String is constructed from entirely used byte[] Jul 7, 2022

openjdk bot added the rfr Pull request is ready for review label Jul 7, 2022

openjdk bot added security security-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jul 7, 2022

RogerRiggs reviewed Jul 7, 2022

View reviewed changes

stsypanov added 3 commits July 11, 2022 10:57

8289908: Make constructor private and use trailing Void instead of int

d41ceec

Merge branch 'master' into 8289908

294e91f

8289908: Fixed tests

b7375cd

prrace approved these changes Jul 12, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 12, 2022

stsypanov added 2 commits July 13, 2022 16:44

Merge branch 'master' into 8289908

4b1531e

8289908: Rework constructor

f653a67

RogerRiggs approved these changes Jul 13, 2022

View reviewed changes

src/java.base/share/classes/java/lang/String.java Outdated Show resolved Hide resolved

stsypanov added 2 commits July 13, 2022 20:36

8289908: Fix typo

ea2bbe8

8289908: Fix typo

72146f7

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 14, 2022

turbanoff approved these changes Jul 14, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 18, 2022

openjdk bot closed this Jul 18, 2022

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 Jul 18, 2022

stsypanov deleted the 8289908 branch July 18, 2022 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8289908: Skip bounds check for cases when String is constructed from entirely used byte[] #9407

8289908: Skip bounds check for cases when String is constructed from entirely used byte[] #9407

stsypanov commented Jul 7, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

mlbridge bot commented Jul 7, 2022 •

edited

RogerRiggs Jul 7, 2022

stsypanov Jul 8, 2022

RogerRiggs Jul 8, 2022

stsypanov Jul 11, 2022

stsypanov Jul 11, 2022

RogerRiggs Jul 12, 2022

stsypanov Jul 13, 2022

openjdk bot commented Jul 12, 2022

openjdk bot commented Jul 12, 2022 •

edited

RogerRiggs left a comment

stsypanov commented Jul 14, 2022

openjdk bot commented Jul 14, 2022

prrace commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

8289908: Skip bounds check for cases when String is constructed from entirely used byte[] #9407

8289908: Skip bounds check for cases when String is constructed from entirely used byte[] #9407

Conversation

stsypanov commented Jul 7, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 7, 2022

openjdk bot commented Jul 7, 2022

mlbridge bot commented Jul 7, 2022 • edited

Webrevs

RogerRiggs Jul 7, 2022

Choose a reason for hiding this comment

stsypanov Jul 8, 2022

Choose a reason for hiding this comment

RogerRiggs Jul 8, 2022

Choose a reason for hiding this comment

stsypanov Jul 11, 2022

Choose a reason for hiding this comment

stsypanov Jul 11, 2022

Choose a reason for hiding this comment

RogerRiggs Jul 12, 2022

Choose a reason for hiding this comment

stsypanov Jul 13, 2022

Choose a reason for hiding this comment

openjdk bot commented Jul 12, 2022

openjdk bot commented Jul 12, 2022 • edited

RogerRiggs left a comment

Choose a reason for hiding this comment

stsypanov commented Jul 14, 2022

openjdk bot commented Jul 14, 2022

prrace commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

stsypanov commented Jul 7, 2022 •

edited by openjdk bot

mlbridge bot commented Jul 7, 2022 •

edited

openjdk bot commented Jul 12, 2022 •

edited