[Console] Fix performance bottleneck for large JSON payloads #57668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The legacy_core_editor implemenation was calculating the current editor line
count by .split('\n').length on the entire buffer which was very inefficient
in a tight loop. This caused a performance regression. Now we use the cached
line count provided by the underlying editor implementation.
Probably was not a performance issue, just taking unnecessary steps. Not sure that this function is even being used.
jloleysens
added
Feature:Console
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
sebelga
approved these changes
Feb 14, 2020
|
|
||
| function tokenTest(tokenList, prefix, data) { | ||
| if (data && typeof data !== 'string') { | ||
| data = JSON.stringify(data, null, 3); |
| position: { lineNumber: 1, column: 1 }, | ||
| }); | ||
| const ret = []; | ||
| let t = iter.getCurrentToken(); |
There was a problem hiding this comment.
Same here: let token = ...
| editor: coreEditor, | ||
| position: { lineNumber: 1, column: 1 }, | ||
| }); | ||
| const ret = []; |
There was a problem hiding this comment.
Wouldn't it be better to name it? what exactly do we expect to return?
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 14, 2020
…#57668) * Fix Console performance bug for large request bodies The legacy_core_editor implemenation was calculating the current editor line count by .split('\n').length on the entire buffer which was very inefficient in a tight loop. This caused a performance regression. Now we use the cached line count provided by the underlying editor implementation. * Fix performance regression inside of ace token_provider implementation * Clean up another unnecessary use of getValue().split(..).length. Probably was not a performance issue, just taking unnecessary steps. Not sure that this function is even being used.
This was referenced Feb 14, 2020
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 14, 2020
…#57668) * Fix Console performance bug for large request bodies The legacy_core_editor implemenation was calculating the current editor line count by .split('\n').length on the entire buffer which was very inefficient in a tight loop. This caused a performance regression. Now we use the cached line count provided by the underlying editor implementation. * Fix performance regression inside of ace token_provider implementation * Clean up another unnecessary use of getValue().split(..).length. Probably was not a performance issue, just taking unnecessary steps. Not sure that this function is even being used. # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/__tests__/input.test.js # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/__tests__/input_tokenization.test.js
jloleysens
added a commit
that referenced
this pull request
Feb 14, 2020
…#57683) * Fix Console performance bug for large request bodies The legacy_core_editor implemenation was calculating the current editor line count by .split('\n').length on the entire buffer which was very inefficient in a tight loop. This caused a performance regression. Now we use the cached line count provided by the underlying editor implementation. * Fix performance regression inside of ace token_provider implementation * Clean up another unnecessary use of getValue().split(..).length. Probably was not a performance issue, just taking unnecessary steps. Not sure that this function is even being used. # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/__tests__/input.test.js # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/__tests__/input_tokenization.test.js
jloleysens
added a commit
that referenced
this pull request
Feb 14, 2020
…#57682) * Fix Console performance bug for large request bodies The legacy_core_editor implemenation was calculating the current editor line count by .split('\n').length on the entire buffer which was very inefficient in a tight loop. This caused a performance regression. Now we use the cached line count provided by the underlying editor implementation. * Fix performance regression inside of ace token_provider implementation * Clean up another unnecessary use of getValue().split(..).length. Probably was not a performance issue, just taking unnecessary steps. Not sure that this function is even being used.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix #57431
Notes for reviewers
The bulk of changes are whitespace changes 🤦🏼♂️ for a test file that was updated to check line count number is reporting correctly.
The rest should be in
legacy_core_editor.ts,sense_editor.tsandtoken_provider.ts. Specifically we are now using.getLengththat returns a cached value of the line count.https://github.com/ajaxorg/ace/blob/d35142c47b5549ab5714848f373b98a4462d2bdd/lib/ace/edit_session.js#L1092
How to test
See the original issues for details. Thanks @LucaWintergerst for reporting!
Also attempt to edit text at the bottom of the large body. This will cause the autocomplete token iterator to traverse a large part of the body.
There should now be no noticeable delay when typing or moving the cursor around.
Release Note
We fixed a Console performance regression that caused a big slow down for large request bodies.
Checklist
Delete any items that are not applicable to this PR.
For maintainers