Range performance fixes #9
Conversation
| @@ -189,13 +189,33 @@ class Range.NormalizedRange | |||
| # | |||
| # Returns updated self or null. | |||
| limit: (bounds) -> | |||
| nodes = $.grep this.textNodes(), (node) -> | |||
| node.parentNode == bounds or $.contains(bounds, node.parentNode) | |||
| if @commonAncestor == bounds or $.contains(bounds, @commonAncestor) | |||
There was a problem hiding this comment.
I would make this === for clarity.
There was a problem hiding this comment.
I've not written CoffeeScript before yesterday, but the docs suggest that == in CoffeeScript compiles to === in JS.
There was a problem hiding this comment.
|
To no-one's great surprise, using |
|
(Let me know if you would like the intermediate patches squashed away.)) |
|
That's excellent. Squash away and I'll be glad to merge it! |
|
If you could also remove the flatten function, which is no longer used now, that would be excellent. |
Previously, only the left-hand side was tested; and grandchildren of the
bounds were not tested at all. With [] indicating a text node:
<h1>[My Heading]</h1>
<p>
[My paragraph]
<span>
[ conti]
[nues]
</span>
<p>
<p>[Another paragraph begins]</p>
I wrote an implemetation where, given a range ending in the second
paragraph, limiting it to the first paragraph would incorrectly update
the `@end` to ` conti` not `nues`. The old test case did not catch it.
Traversing the whole tree is unnecessary -- and costly if the @CommonAncestor is <body> and you have many nodes in your document. All we actually need to do is update @start and @EnD to point at the first and last text nodes in the bounds, if they currently fall outside it. * master: 1.61s ±2.26% (6 runs sampled) -- but this is a little unfair since it was implemented in terms of a very slow getTextNodes call * this implementation: 0.01ms ±2.91% (85 runs sampled)
The old implementation was really expensive on a range covering many rows of table. I think it's because an array was being allocated at every level of the tree, then flattened out into the level above. Instead here we just do a depth-first search for text nodes and build up a single array as we go. In a little benchmark with a range covering every cell of a 5000×3 table: * master: 1.57s ±1.14% (6 runs sampled) * TreeWalker: 17.5ms ±0.74% (72 runs sampled)
Per openannotation/annotator#527 . We actually don't need to use global at all -- we can get the document from the element we're about to walk.
|
How's that? |
|
Looks great :) |
|
@mwidner there's a v1.2.x branch on annotator so we have a place to backport fixes and make maintenance releases. Do you think you could extract this into a PR there? |
|
It shouldn't be too hard because when this change happened the files were still coffee and still had the same names and paths, so it might be as simple as taking a copy of the annotator repo, branching off v1.2.x, adding this repo as a remote and cherry-picking these changes. |
|
Done! Thanks for the pointers. |
I'm using Annotator on pages with really large tables, highlighting potentially thousands of rows of text. Selecting any amount of text which spans more than one row was extremely expensive.
NormalizedRange#limitand#getTextNodeswere the main culprits according to Chrome's profiler. These patches make them an order of magnitude faster for me.Happy to write more tests if you're not convinced that this new logic is right! Also, the existing code is inconsistent in whether it uses
$.containsorutil.contains; shout if there's some hidden reason to use one or the other that I've not seen.Agh, while writing this pull request I've noticed that I introduced a use of
childNodesin#limit-- based on the comment in#getTextNodesI guess I need to remove that for IE9's sake.