Only reindex documents if declarations are being modified #2942
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
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vinistock
added
enhancement
Graphite App
vinistock
commented
Nov 29, 2024
| @@ -64,7 +64,6 @@ def initialize(index, dispatcher, parse_result, uri, collect_comments: false) | |||
| :on_constant_path_or_write_node_enter, | |||
| :on_constant_path_operator_write_node_enter, | |||
| :on_constant_path_and_write_node_enter, | |||
| :on_constant_or_write_node_enter, | |||
There was a problem hiding this comment.
This event was duplicated.
There was a problem hiding this comment.
Should we change Prism detect this and emit an error/warning?
There was a problem hiding this comment.
I'd rather make our cop smarter if we can avoid paying the price on every single listener instantiation.
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
from
41cc250 to
826bd1a
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
2 times, most recently
from
c103d7c to
ea92bc7
Compare
andyw8
reviewed
Dec 3, 2024
| @@ -64,7 +64,6 @@ def initialize(index, dispatcher, parse_result, uri, collect_comments: false) | |||
| :on_constant_path_or_write_node_enter, | |||
| :on_constant_path_operator_write_node_enter, | |||
| :on_constant_path_and_write_node_enter, | |||
| :on_constant_or_write_node_enter, | |||
There was a problem hiding this comment.
Should we change Prism detect this and emit an error/warning?
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
from
826bd1a to
2251c9d
Compare
vinistock
force-pushed
the
11-29-index_documents_on_modification
branch
from
2251c9d to
e5b8c20
Compare
vinistock
force-pushed
the
11-29-only_reindex_documents_if_declarations_are_being_modified
branch
from
ea92bc7 to
f299a65
Compare
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.
Motivation
This PR tries to limit the amount of times we pay the price of reindexing the current document being modified by analyzing the context behind the last edit made to it.
Implementation
The idea of this approach is to remember the position of the last edit made to a document and the nature of the edit (insert, replace or delete).
Then, depending on the type of edit and what nodes exist under the position where the edit was made, we decide if reindexing is relevant or not.
To determine that, we look at the node under the cursor and, if it matches a node we use for indexing in the
DeclarationListener, then we consider that we need to reindex.It's not perfect, so let me know if you have other ideas on how to determine this in a way that is:
Necessarily in this order because the whole point is to reduce the performance impact of having to reindex. If detecting whether we need to do it or not is equally as slow, then there's no point in doing it.
Automated Tests
Added some tests, but we should probably add more.