Rewrite rendering algorithm+ improve performance + fix memory leaks #581
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
aminya
changed the title
aminya
force-pushed
the
fixRemoval
branch
5 times, most recently
from
43ad0e8 to
be2d5ed
Compare
aminya
force-pushed
the
fixRemoval
branch
8 times, most recently
from
4abdbd5 to
1206a55
Compare
|
Haha! Found the reason! JavaScript cannot delete/add messages objects from We should use |
aminya
force-pushed
the
fixRemoval
branch
3 times, most recently
from
f990669 to
3a2eee0
Compare
JavaScript has problems comparing different complex object. In contrast, using a simple string as the key removes this problem https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map
JavaScript cannot delete/add messages objects from `this.messages`! Even though we call, `this.messages.delete(message)`, it cannot find the message we are asking it to delete. Because of that `this.messages` keeps growing!! We should use `message.key` to delete things instead of using a `Set`.
Because we use Map for this.markers and this.messages, we need to update this code which had remained untouched!
This comment has been minimized.
This comment has been minimized.
aminya
force-pushed
the
fixRemoval
branch
2 times, most recently
from
2027e95 to
d5778ae
Compare
|
I am happy to announce that this PR and linter PR are ready! 😃 |
aminya
changed the title
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.
Description of the change
By merging steelbrain/linter#1706 the removal now works correctly. However, that reveals some bugs in the linter-ui code which is supposed to be fixed in this pull request. The previous logic of the linter mostly rerendered all the messages from scratch. But now some are needed to be removed and that is why these bugs got revealed.
In this PR, the rendering algorithm is rewritten so only the "added" messages are rendered from scratch, and the "removed" messages are removed. It does not render all the messages every time. This greatly improves performance.
Several memory leaks were fixed in this PR.
should be merged with steelbrain/linter#1706