Issue 29714601: Issue 6437 - Skip elements not affected by DOM mutations (Closed)
|
Created:
March 5, 2018, 6:05 p.m. by Manish Jethani Modified:
June 26, 2018, 7:04 p.m. CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionThis builds on https://codereview.adblockplus.org/29712655/
Let's take the following document:
<div id="n1">
<p id="n1_1"></p>
<p id="n1_2"></p>
<p id="n1_4">Hello</p>
</div>
<div id="n2">
<p id="n2_1"></p>
<p id="n2_2"></p>
<p id="n2_4">Hello</p>
</div>
<div id="n3">
<p id="n3_1"></p>
<p id="n3_2"></p>
<p id="n3_4">Hello</p>
</div>
<div id="n4">
<p id="n4_1"></p>
<p id="n4_2"></p>
<p id="n4_4">Hello</p>
</div>
Suppose we have the filter p:-abp-contains(Hello)
When a new node n2_3 is added to the document, we check every element that matches the selector "p". This is wasteful and unnecessary. MutationObserver already tells us which nodes have been added, then we only need to check any of the new nodes' ancestors or descendants that match the selector. When we come across n1_1, for example, we can skip it because it obviously is not affected by the given set of mutations.
The problem is even worse when you consider -abp-has and multiple levels of nesting.
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 1
Patch Set 3 : Rebase #Patch Set 4 : Disable optimization for patterns containing sibling combinators #
Total comments: 1
Patch Set 5 : Remove comment about removed nodes #Patch Set 6 : Ignore mutation targets when using style sheets #Patch Set 7 : Add tests #
Total comments: 1
Patch Set 8 : Ignore own DOM modifications in debug mode #Patch Set 9 : Rename debug mode to test mode #
Total comments: 4
MessagesTotal messages: 26
Patch Set 1
Patch Set 2: Rebase
https://codereview.adblockplus.org/29714601/diff/29728577/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29728577/lib/content/elemHid... lib/content/elemHideEmulation.js:349: if (targets && !targets.some(target => element.contains(target) || By the way this may not seem like such a great idea until you realize how slow Element.textContent is. The element has to collect text from all of its children.
I just realized that this patch does not take into account sibling selectors (~ and +), always assumes that only ancestors and descendants are affected by a mutation. I'll try to update it to take sibling selectors into account as well.
Patch Set 4: Disable optimization for patterns containing sibling combinators
The pattern "#?#div.bar ~ div:-abp-has(>p:-abp-contains(Hello))" would not work
on the following DOM:
<div id="foo"></div><div><p>Hello</p></div>
Of course because the first div doesn't have a class. But if we set the class in
JavaScript:
document.getElementById("foo").className = "bar";
It would still not work, because of this optimization. This optimization would
ignore the second div (candidate element) because it neither contains nor is
contained by the first div (mutation target).
The easiest way to deal with this for now is to disable the optimization for
patterns that appear to contain (even if they don't necessarily contain) any
sibling combinators.
I have more ideas about this:
1. We can check if the candidate element or one of its ancestors is a sibling
of the mutation target
2. Better detection of actual sibling combinators (fewer false positives)
3. The sibling combinator matters only when it's a prefix, not when it's a
suffix; we can take this into account
4. Check if it's a "~" or a "+" and narrow down the criteria accordingly
All of this be done later, let's keep it simple for now. This optimization
should still be useful for most of the cases.
https://codereview.adblockplus.org/29714601/diff/29778597/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29778597/lib/content/elemHid... lib/content/elemHideEmulation.js:542: // Ideally we would also be interested in removed nodes, but since we Well we're already using CSS selectors for element hiding emulation by now, I'll have to update this. See also: https://codereview.adblockplus.org/29728690/
Patch Set 5: Remove comment about removed nodes
Note that this patch will break the following case on Firefox:
<!DOCTYPE html>
<article>
Hello
</article>
<script>
setTimeout(() =>
{
let [oldArticle] = document.getElementsByTagName("article");
let newArticle = document.createElement("article");
newArticle.innerText = "Hi";
document.body.insertBefore(newArticle, oldArticle);
}, 5000);
</script>
For this to work, we need to land https://codereview.adblockplus.org/29728690/
first. Let me try to address the concerns with that one.
Patch Set 6: Ignore mutation targets when using style sheets
On 2018/05/13 14:42:35, Manish Jethani wrote:
> Note that this patch will break the following case on Firefox:
>
> <!DOCTYPE html>
> <article>
> Hello
> </article>
> <script>
> setTimeout(() =>
> {
> let [oldArticle] = document.getElementsByTagName("article");
> let newArticle = document.createElement("article");
> newArticle.innerText = "Hi";
> document.body.insertBefore(newArticle, oldArticle);
> }, 5000);
> </script>
>
> For this to work, we need to land https://codereview.adblockplus.org/29728690/
> first. Let me try to address the concerns with that one.
This is now taken care of by disabling this optimization when using style
sheets. We can re-enable it for that case once the other patch lands.
looking good, but I think we should try to have more test coverage in that area.
On 2018/05/14 20:27:20, hub wrote: > looking good, but I think we should try to have more test coverage in that area. I'm struggling with this. The following tests should already check if nothing related to style sheet loads or DOM mutations is broken: - testPseudoClassPropertiesOnStyleSheetLoad - testPlainAttributeOnDomMutation - testPseudoClassContainsOnDomMutation - testPseudoClassHasOnDomMutation - testPseudoClassHasWithClassOnDomMutation - testPseudoClassHasWithPseudoClassContainsOnDomMutation Do you mean we should test that unrelated DOM elements are being ignored as a result of this patch?
On 2018/05/15 12:44:09, Manish Jethani wrote: > Do you mean we should test that unrelated DOM elements are being ignored as a > result of this patch? That would be awesome.
On 2018/05/15 17:57:25, hub wrote: > On 2018/05/15 12:44:09, Manish Jethani wrote: > > > Do you mean we should test that unrelated DOM elements are being ignored as a > > result of this patch? > > That would be awesome. OK, I have an idea how to do it. Basically in "debug mode" or "test mode" the ElemHideEmulation object will keep track of all the patterns and elements processed. The testing code can then request the object to return this data (an array for the patterns and a weak set for the elements) and examine this data to see if it is what was expected; it can also ask the object to clear all this data. This will only have an effect in debug/test mode, so it should not affect the normal execution of the code. I have already written some code for this, but I think this would be cleaner as a separate patch (also test all the optimizations, not just this one). What do you think about landing this one first and then doing the testing patch? Alternatively I can implement that as a separate patch, then rebase this one on that and add the specific tests. Let me know what you think.
Patch Set 7: Add tests On 2018/05/16 03:38:25, Manish Jethani wrote: > On 2018/05/15 17:57:25, hub wrote: > > On 2018/05/15 12:44:09, Manish Jethani wrote: > > > > > Do you mean we should test that unrelated DOM elements are being ignored as > a > > > result of this patch? > > > > That would be awesome. > > OK, I have an idea how to do it. Basically in "debug mode" or "test mode" the > ElemHideEmulation object will keep track of all the patterns and elements > processed. The testing code can then request the object to return this data (an > array for the patterns and a weak set for the elements) and examine this data to > see if it is what was expected; it can also ask the object to clear all this > data. This will only have an effect in debug/test mode, so it should not affect > the normal execution of the code. > > I have already written some code for this, but I think this would be cleaner as > a separate patch (also test all the optimizations, not just this one). What do > you think about landing this one first and then doing the testing patch? > Alternatively I can implement that as a separate patch, then rebase this one on > that and add the specific tests. > > Let me know what you think. OK, this is already done.
https://codereview.adblockplus.org/29714601/diff/29783555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29783555/lib/content/elemHid... lib/content/elemHideEmulation.js:32: lastProcessedElements: new Set() We could add another entry here, lastProcessedPatterns, but that would be another patch
It seems that the test "testOnlyRelevantElementsProcessed" fails on Firefox AssertionError: The element with ID 'n1' should have been processed AssertionError: The element with ID 'n2' should have been processed ... (and so on) I run the test applying this patch beforehand: https://codereview.adblockplus.org/29720661/
On 2018/05/17 20:35:10, hub wrote: > [...] > > I run the test applying this patch beforehand: > https://codereview.adblockplus.org/29720661/ After applying the patch and running npm install and npm test, I get the error: Error: Cannot find module './test/runners/chromium_process'
Patch Set 8: Ignore own DOM modifications in debug mode Patch Set 9: Rename debug mode to test mode The problem was that the version of Firefox in the tests was observing modifications to attribute values, even though we set "attributes: false". It's better to just ignore any such mutations for the purpose of testing, and that's what the updated patch does.
My comments on this are opening a can of worm concerning the testing frameworks we use. I'm not super strong on blocking this, but ideally we should avoid having code that is only needed for testing in the main JS code. https://codereview.adblockplus.org/29714601/diff/29784654/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29784654/lib/content/elemHid... lib/content/elemHideEmulation.js:43: exports.getTestInfo = getTestInfo; I'm a bit skeptical with that way or test, including leaving unused code in the release. Sadly we don't have spies in our test framework (maybe we should). https://codereview.adblockplus.org/29714601/diff/29784654/lib/content/elemHid... lib/content/elemHideEmulation.js:954: } ... and this :-/ There should be a better way to go at this.
A couple of comments inline. So what do you think we should do now? We have a useful optimization that helps the user to not drain their CPU/battery and lets filter authors be more confident about using filters of this kind. The way we are testing this code sucks, but I'm not sure that's a good reason to postpone this any more. https://codereview.adblockplus.org/29714601/diff/29784654/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29714601/diff/29784654/lib/content/elemHid... lib/content/elemHideEmulation.js:43: exports.getTestInfo = getTestInfo; On 2018/05/24 19:19:09, hub wrote: > I'm a bit skeptical with that way or test, including leaving unused code in the > release. > > Sadly we don't have spies in our test framework (maybe we should). We can work on improving our test framework, but for now this is what we have. Between not having this testing code at all and having it as "dead code" in production, I'd still go with the latter. At least we're checking our assumptions in the tests and making sure nothing breaks. https://codereview.adblockplus.org/29714601/diff/29784654/lib/content/elemHid... lib/content/elemHideEmulation.js:954: } On 2018/05/24 19:19:09, hub wrote: > ... and this :-/ > > There should be a better way to go at this. OK, do you have any suggestions? This is only done in test mode, so it's literally dead code otherwise. We don't have preprocessing or we could have stripped it out.
if Dave is good with it, I'm good with it.
On 2018/05/25 16:24:44, hub wrote: > if Dave is good with it, I'm good with it. Moved Dave to reviewers, thanks.
On 2018/05/25 22:36:33, Manish Jethani wrote: > On 2018/05/25 16:24:44, hub wrote: > > if Dave is good with it, I'm good with it. > > Moved Dave to reviewers, thanks. Hubert, it's unlikely that Dave will have time for this (he probably has the longest code review backlog of us all), and he has deferred to you about element hiding emulation in the past. This patch was uploaded 3 months and 3 weeks ago and has still not landed. If you don't have any constructive suggestions about how to improve this patch, would you mind giving me a quick LGTM? This would complete the work on my element hiding emulation optimizations.
LGTM. (I'll file a bug to improve the tests)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
