-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8355177: Speed up StringBuilder::append(char[]) via Unsafe::copyMemory #24773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cd51370
8c82366
3b16473
ee356d3
ec6482b
db11089
3218260
834acc4
a9af8c3
d555a1d
7e5b6de
2a45cfc
1846371
4378fdf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||
| /* | ||||||||||||||
| * Copyright (c) 2015, 2025, Oracle and/or its affiliates. All rights reserved. | ||||||||||||||
| * Copyright (c) 2025, Alibaba Group Holding Limited. All Rights Reserved. | ||||||||||||||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||||||||||||||
| * | ||||||||||||||
| * This code is free software; you can redistribute it and/or modify it | ||||||||||||||
|
|
@@ -1312,12 +1313,6 @@ static Stream<String> lines(byte[] value) { | |||||||||||||
| return StreamSupport.stream(LinesSpliterator.spliterator(value), false); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private static void putChars(byte[] val, int index, char[] str, int off, int end) { | ||||||||||||||
| while (off < end) { | ||||||||||||||
| putChar(val, index++, str[off++]); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static String newString(byte[] val, int index, int len) { | ||||||||||||||
| if (len == 0) { | ||||||||||||||
| return ""; | ||||||||||||||
|
|
@@ -1486,7 +1481,13 @@ public static void putCharSB(byte[] val, int index, int c) { | |||||||||||||
|
|
||||||||||||||
| public static void putCharsSB(byte[] val, int index, char[] ca, int off, int end) { | ||||||||||||||
| checkBoundsBeginEnd(index, index + end - off, val); | ||||||||||||||
| putChars(val, index, ca, off, end); | ||||||||||||||
| String.checkBoundsBeginEnd(off, end, ca.length); | ||||||||||||||
| Unsafe.getUnsafe().copyMemory( | ||||||||||||||
| ca, | ||||||||||||||
| Unsafe.ARRAY_CHAR_BASE_OFFSET + ((long) off << 1), | ||||||||||||||
| val, | ||||||||||||||
| Unsafe.ARRAY_BYTE_BASE_OFFSET + ((long) index << 1), | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to use ARRAY_CHAR_INDEX_SCALE, it should be used as follows Unsafe.getUnsafe().copyMemory(
ca,
Unsafe.ARRAY_CHAR_BASE_OFFSET + (long) off * Unsafe.ARRAY_CHAR_INDEX_SCALE,
val,
Unsafe.ARRAY_CHAR_BASE_OFFSET + (long) index * Unsafe.ARRAY_CHAR_INDEX_SCALE,
(long) (end - off) * Unsafe.ARRAY_CHAR_INDEX_SCALE);There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to calculate an ARRAY_CHAR_SHIFT in the constant like ShortVector does, like this static final int ARRAY_CHAR_SHIFT
= 31 - Integer.numberOfLeadingZeros(Unsafe.ARRAY_CHAR_INDEX_SCALE);
Unsafe.getUnsafe().copyMemory(
ca,
Unsafe.ARRAY_CHAR_BASE_OFFSET + (long) off << ARRAY_CHAR_SHIFT,
val,
Unsafe.ARRAY_CHAR_BASE_OFFSET + (long) index << ARRAY_CHAR_SHIFT,
(long) (end - off) << ARRAY_CHAR_SHIFT);There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. String uses Unsafe.getUnsafe().copyMemory(
ca,
Unsafe.ARRAY_CHAR_BASE_OFFSET + (long) off << String.UTF16,
val,
Unsafe.ARRAY_CHAR_BASE_OFFSET + (long) index << String.UTF16,
(long) (end - off) << String.UTF16);There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that we can expect arrays to be laid out as a contiguous chunk of memory with the intuitively expected element size. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are many places in the String class that use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, I prefer the explicit constant shift |
||||||||||||||
| (long) (end - off) << 1); | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation of The invocation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recall runtime requires UTF16 byte array and char array have exactly the same layout - would be nice if we keep this in the design notes for the string implementation classes, such as on the class header. (Useful notes could include that indices are char-based, UTF16 byte[] and char[] has identical layout, etc.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The StringUTF16.getChar and putChar methods are carefully written to use the platform endianness to compose and decompose char values from and to byte[] in terms of shifts of the lower and upper bytes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've found the code that imposes this requirement: jdk/src/hotspot/share/opto/library_call.cpp Lines 1713 to 1718 in 7bf4c60
|
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static void putCharsSB(byte[] val, int index, CharSequence s, int off, int end) { | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.