-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Optimize ContentPath#pathAsText #98244
Optimize ContentPath#pathAsText #98244
Conversation
💚 CLA has been signed |
Pinging @elastic/es-search (Team:Search) |
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.
Hi @dorukguner, sorry for the delay in getting to this and thanks for picking it up.
I like the solution and the tests are great. Since we have tests now I would like to go a step further and optimize your solution even a bit more. I left a few suggestions for that, let me know what you think.
server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/ContentPathPathAsTextTests.java
Outdated
Show resolved
Hide resolved
Hi @cbuescher, thanks for the feedback. I've merged the extra test cases I had into the new main test class and I've also optimised ContentPath a little further. I realised we didn't need both an array of strings for the path and a string builder. We can have just a string builder while keeping track of the indexes in which the delimiters were added, and using the add and remove methods to modify the string builder directly. This has removed the need for any loops and any extra indexes. Please let me know what you think about the new optimisations, I'm open to any other suggestions you may have. |
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.
@dorukguner thanks a lot, I think the idea of just keeping one datastructure for the whole path and keeping around the pointers to the parts is great and the tests are also covering this nicely. I left two small comments but will prepare this PR for testing and merging while you address those.
server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java
Outdated
Show resolved
Hide resolved
Just running a check before the final changes to see if this breaks anything else. @elasticsmachine test this please |
@dorukguner could you also run ./gradlew :server:spotlessApply to apply our formatting rules to the modified classes? |
…ve existing ContentPath tests
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.
Thanks for the update, LGTM. Will need to run full test suite before merging.
@elasticmachine test this please |
@elasticmachine test this please |
@elasticmachine update branch |
@elasticmachine test this please |
@dorukguner huzzah, all passed. Thanks for the change and the iterations, merged to main branch now. I think this is a great change that makes using this class a lot less wasteful. |
This reverts commit db22afa.
) Some benchmarks indicate that parts of the change made in elastic#98244 is negatively affecting document parsing. This change reverts it until we have better benchmarks that support it or a solution that avoids the Stack datastructure most likely contributing to this. This reverts commit db22afa.
This pr optimizes calls to ContentPath#pathAsText through the following changes:
The case in which fields that have been previously added to the path are removed is handled in ContentPath#remove by clearing the existing StringBuilder.
Tests for ContentPath#pathAsText have been added in a separate commit to the optimization to allow for easy validation that the tests are correct.
Addresses #94544