Faster filtering with fragmented annotations#526
Open
wjt wants to merge 3 commits intoopenannotation:masterfrom
Open
Faster filtering with fragmented annotations#526wjt wants to merge 3 commits intoopenannotation:masterfrom
wjt wants to merge 3 commits intoopenannotation:masterfrom
Conversation
Contributor
Author
|
A nicer approach would be for Filter to track the set of annotations directly, rather than reconstructing it from the highlights. This would have other benefits as welll – for example, it wouldn't have to refilter all annotations just because a single annotation was modified – but it would be a more invasive change. |
Member
|
By the way, I think my latest commit on master will eliminate this suprious travis failures on pull requests. See what happens if you rebase. |
Before, it depended on pretty specific knowledge of how updateFilter is implemented, which seems quite questionable!
If there are many spans of text for an annotation (many table cells,
say), previously the filter would be applied once for each span, and
matching annotations would appear n times in filter.annotations. In and
of itself this is relatively harmless, but filterHighlights then maps
back from annotations to highlights, and:
for (var i = 0, len = filtered.length; i < len; i++) {
highlights = highlights.not(filtered[i]._local.highlights);
}
is ruinous. This change dramatically improves filtering performance on
my pet test case of big, overlapping highlights on a 5000×3 table.
I have mixed feelings about using a new identifier in _local for this,
but we don't have any guarantee that the id provided by the server is
unique, or even present.
I also have mixed feelings about setting the id in highlighter.js and
using it in filter.js, but the same coupling exists for
.data('annotation') already...
49f6dec to
b097905
Compare
Contributor
Author
|
Nope, the rebase didn't seem to help. |
Member
|
It would be nice to do this without coupling the highlighter and the filter with a private property. Any other ideas? |
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
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.
Let me know if you'd prefer monolithic pull requests! The test data I'm looking at for this PR is here – you'll need to open it from
tools/serve. The key commit message here:If there are many spans of text for an annotation (many table cells,
say), previously the filter would be applied once for each span, and
matching annotations would appear n times in
filter.annotations. In andof itself this is relatively harmless, but
filterHighlightsthen mapsback from annotations to highlights, and:
is ruinous. This change dramatically improves filtering performance on
my pet test case of big, overlapping highlights on a 5000×3 table.
I have mixed feelings about using a new identifier in
_localfor this,but we don't have any guarantee that the id provided by the server is
unique, or even present.
I also have mixed feelings about setting the id in
highlighter.jsandusing it in
filter.js, but the same coupling exists for.data('annotation')already...