Update Text class to use native java ByteBuffer #127666

jordan-powers · 2025-05-02T21:13:51Z

This PR is a precursor to #126492.

It does three things:

Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content.
Refactor the Text class to use a java-native ByteBuffer instead of the elasticsearch BytesReference.
Add the XContentString interface, with the Text class implement that interface.

elasticsearchmachine · 2025-05-02T21:14:15Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

ldematte

Looks good, thanks for separating this, made it easier to focus the review.
I left a couple of comments

server/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java

libs/x-content/src/main/java/org/elasticsearch/xcontent/Text.java

libs/x-content/src/main/java/org/elasticsearch/xcontent/XContentString.java

rjernst

I agree with Lorenzo's comments, and have one myself.

rjernst · 2025-05-06T13:45:43Z

server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java

            write(spare.bytes(), 0, spare.length());
        } else {
-            BytesReference bytes = text.bytes();
+            BytesReference bytes = BytesReference.fromByteBuffer(text.bytes());


nit: we could support reading/writing ByteBuffer directly in StreamInput/Output so that conversions are not necessary here. That could allow optimizing in the ByteArrayStreamInput case to create a ByteBuffer that wraps the underlying byte array with appropriate offset/length, without needing to copy bytes to a new array.

I'll look into that, thanks!

Ok, I'm thinking this might be a bigger change than we want to incorporate into this PR. It might be better to leave it as a follow-up, since the current implementation will still work, it'll just be less optimized.

I'm OK with leaving this in a follow-up, it will help with reviews too

ldematte

LGTM

elasticsearchmachine · 2025-05-08T15:20:53Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 127666

jordan-powers · 2025-05-08T16:20:34Z

💚 All backports created successfully

Status	Branch	Result
✅	8.19

Questions ?

Please refer to the Backport tool documentation

This PR is a precursor to #126492. It does three things: - Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. - Refactor the Text class to use a java-native ByteBuffer instead of the elasticsearch BytesReference. - Add the XContentString interface, with the Text class implementing that interface. (cherry picked from commit db0c3c7) # Conflicts: # server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/DefaultHighlighter.java # server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java # server/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightFieldTests.java # test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java

This PR is a precursor to elastic#126492. It does three things: - Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. - Refactor the Text class to use a java-native ByteBuffer instead of the elasticsearch BytesReference. - Add the XContentString interface, with the Text class implementing that interface.

After updating the Text class to use ByteBuffer in elastic#127666 we saw test failures where similar Text instances are shared between different threads and tested for equals(). The reason is that calling bytes() lazily materializes the internal ByteBuffer. That method is what the equals method calls on both instances it tests. Apparently this can leads to race conditions when instances are shared across threads. Making the internal `bytes` representation volatile fixes the problem. Closes elastic#128029

…)" This reverts commit db0c3c7.

* Revert "Fix the Text class package change in example plugins (#128316)" This reverts commit cc48648. * Revert "Update Text class to use native java ByteBuffer (#127666)" This reverts commit db0c3c7.

* Revert "Fix the Text class package change in example plugins (elastic#128316)" This reverts commit cc48648. * Revert "Update Text class to use native java ByteBuffer (elastic#127666)" This reverts commit db0c3c7.

* Revert changes to Text class (#128483) * Revert "Fix the Text class package change in example plugins (#128316)" This reverts commit cc48648. * Revert "Update Text class to use native java ByteBuffer (#127666)" This reverts commit db0c3c7. * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>

* Revert "Fix the Text class package change in example plugins (#128316)" This reverts commit cc48648. * Revert "Update Text class to use native java ByteBuffer (#127666)" This reverts commit db0c3c7. Co-authored-by: Lorenzo Dematté <[email protected]>

This PR is a precursor to #126492. It does three things: 1. Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. 2. Refactor the Text class to use a new EncodedBytes record instead of the elasticsearch BytesReference. 3. Add the XContentString interface, with the Text class implementing that interface. These changes were originally implemented in #127666 and #128316, however they were reverted in #128484 due to problems caused by the mutable nature of java ByteBuffers. This is resolved by instead using a new immutable EncodedBytes record.

This PR is a precursor to #126492. It does three things: 1. Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. 2. Refactor the Text class to use a new EncodedBytes record instead of the elasticsearch BytesReference. 3. Add the XContentString interface, with the Text class implementing that interface. These changes were originally implemented in #127666 and #128316, however they were reverted in #128484 due to problems caused by the mutable nature of java ByteBuffers. This is resolved by instead using a new immutable EncodedBytes record. (cherry picked from commit de40ac4) # Conflicts: # server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/DefaultHighlighter.java # server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java # server/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightFieldTests.java # test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java

jordan-powers added 2 commits May 2, 2025 13:55

Move Text to :libs:x-content

d119ca4

Add XContentString interface

346e85e

jordan-powers added >non-issue :Core/Infra/Core Core issues without another label v9.1.0 labels May 2, 2025

jordan-powers requested a review from ldematte May 2, 2025 21:13

jordan-powers self-assigned this May 2, 2025

jordan-powers requested a review from a team as a code owner May 2, 2025 21:13

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label May 2, 2025

jordan-powers mentioned this pull request May 2, 2025

Skip UTF8 to UTF16 conversion during document indexing #126492

Merged

jordan-powers added 2 commits May 5, 2025 11:33

Fix CI

c55a4c7

Merge remote-tracking branch 'upstream/main' into move-text-class

87afc97

ldematte reviewed May 6, 2025

View reviewed changes

rjernst reviewed May 6, 2025

View reviewed changes

jordan-powers added 4 commits May 6, 2025 08:29

Update readOptionalText() to match readText()

ae8aa86

Rename XContentString#length() to stringLength()

7114fa9

Rename Text#length to Text#stringLength

4a0ac35

Merge remote-tracking branch 'upstream/main' into move-text-class

e20208e

jordan-powers added auto-backport Automatically create backport pull requests when merged v8.19.0 labels May 6, 2025

ldematte approved these changes May 8, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into move-text-class

4e2e588

jordan-powers merged commit db0c3c7 into elastic:main May 8, 2025
17 checks passed

jordan-powers deleted the move-text-class branch May 8, 2025 15:19

elasticsearchmachine added the backport pending label May 8, 2025

jordan-powers mentioned this pull request May 8, 2025

[8.19] Update Text class to use native java ByteBuffer (#127666) #127925

Merged

jordan-powers removed the backport pending label May 8, 2025

ldematte mentioned this pull request May 22, 2025

Fix the Text class package change in example plugins #128316

Merged

cbuescher mentioned this pull request May 23, 2025

[CI] SearchHitsTests testConcurrentEquals failing #127971

Closed

cbuescher mentioned this pull request May 23, 2025

Fix concurrency issue in Text class #128403

Closed

prdoyle added a commit to prdoyle/elasticsearch that referenced this pull request May 26, 2025

Revert "Update Text class to use native java ByteBuffer (elastic#127666…

db5f66b

…)" This reverts commit db0c3c7.

prdoyle added a commit to prdoyle/elasticsearch that referenced this pull request May 26, 2025

Revert "Update Text class to use native java ByteBuffer (elastic#127666…

f431c7d

…)" This reverts commit db0c3c7.

This was referenced May 26, 2025

Revert changes to Text class #128479

Closed

Revert changes to Text class #128480

Closed

Revert changes to Text class #128482

Closed

Revert changes to Text class #128483

Merged

jordan-powers mentioned this pull request Jun 2, 2025

Move Text class to libs/xcontent #128780

Merged

Update Text class to use native java ByteBuffer #127666

Update Text class to use native java ByteBuffer #127666

Uh oh!

Conversation

jordan-powers commented May 2, 2025

Uh oh!

elasticsearchmachine commented May 2, 2025

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst May 6, 2025

Choose a reason for hiding this comment

Uh oh!

jordan-powers May 6, 2025

Choose a reason for hiding this comment

Uh oh!

jordan-powers May 7, 2025

Choose a reason for hiding this comment

Uh oh!

ldematte May 8, 2025

Choose a reason for hiding this comment

Uh oh!

ldematte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented May 8, 2025

💔 Backport failed

Uh oh!

jordan-powers commented May 8, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants