Virtual table improvements/fixes #11
Merged
Conversation
This file contains hidden or 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
This isn't ideal, but it is a big performance win if the list is big. Since we don't make assumptions about the data outside of the rebuild method I think it's pretty safe.
Possibly I merged/rebased badly - this code was originally moved, but we've ended up with two copies. This meant every event was being logged twice.
Hmmmm 🤔 |
devoncarew
approved these changes
Sep 18, 2018
| // of items from the old data. This is faster than insert(0, log). | ||
| // | ||
| // If this turns out to be too slow, we can make it faster (saving around | ||
| // 30-40% of the time) by just .add()ing to the list and having a flag on |
There was a problem hiding this comment.
This seems worth doing - appending to the list and reversing the way we index through it.
There was a problem hiding this comment.
Okey, I'll do in a separate PR 👍
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.
setRowsin tables (this isn't ideal, but for big tables just copying the list can take a long time - I think the risk is low and if a consumer of the table wants, it can provide a cloned list to setRows)_logand remove TODOsThe last one - I had previously mentioned adding a small buffer, but it turned out to be slightly complicated and I came up with a better idea - just reusing the same list and appending using
.add()- this gets fast because the table capacity doubles so once the table gets big (or near its max size). The downside is that logs are added to the bottom - but we can easily fix that with a flag on the table that just reverses the index as it reads data.That said, it's performing well enough locally that I didn't think it was worth that complexity. If you feel that we do need to make it faster, I think this should be the first thing to try - just lmk.