Issue 29714638: Issue 6446 - Ignore emulated selectors if unchanged (Closed)
|
Created:
March 5, 2018, 10:42 p.m. by Manish Jethani Modified:
March 21, 2018, 4:20 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6446 - Ignore emulated selectors if unchanged
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make emulatedSelectors member of ElemHide #Patch Set 3 : Use a hash function #
Total comments: 7
MessagesTotal messages: 19
Patch Set 1
I'm not too happy with keeping another copy of the injected selectors in memory. Also if the selectors changed, wouldn't this give false positives anyway, if we don't remove the previous style sheet. So isn't the problem rather (either way, whether the selectors changed or not) that the previous style sheet doesn't get removed? https://codereview.adblockplus.org/29714638/diff/29714639/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714638/diff/29714639/include.preload.js#... include.preload.js:38: let emulatedSelectors = []; Shouldn't this rather be a property on the ElemHide object?
On 2018/03/06 20:17:39, Sebastian Noack wrote: > I'm not too happy with keeping another copy of the injected selectors in memory. > Also if the selectors changed, wouldn't this give false positives anyway, if we > don't remove the previous style sheet. So isn't the problem rather (either way, I don't understand this part. If the selectors haven't changed, there's nothing to be done, that's exactly what this patch does. I don't get the "false positive" part here. The reason this problem even exists is this part of the code in lib/cssInjection.js: https://github.com/adblockplus/adblockpluschrome/blob/c3a9ca1de963bebf7806711... 1. We can't rely on frame.injectedStyleSheets 2. We want to add the new style sheet before removing the previous one 3. If the previous one is identical, we can't remove it, because on Firefox it'll just remove the one style sheet (whereas on Chrome there are actually multiple identical style sheets and the last one should be removed since it's a duplicate!) Besides, why make the round trip to the background page anyway? The content script, not the background page, is in the best position to know which selectors were injected previously, so it can decide whether or not to ask the background page to inject the same selectors again. Regarding memory usage, typically the list of selectors for emulation hiding filters will be small. It may be a larger list once we start using selectors for -abp-has and -abp-contains, but even then it'll be in the order of 10 or even 100 selectors, not the thousands of selectors we have for the standard hiding filters. If memory becomes a problem, we can just keep a hash of the selectors rather than the entire textual list. > whether the selectors changed or not) that the previous style sheet doesn't get > removed? Yeah, so the reason this issue exists now is because of the difference between how Firefox and Chrome treat duplicate injected style sheets. Firefox throw an error, Chrome allows it.
Patch Set 2: Make emulatedSelectors member of ElemHide https://codereview.adblockplus.org/29714638/diff/29714639/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714638/diff/29714639/include.preload.js#... include.preload.js:38: let emulatedSelectors = []; On 2018/03/06 20:17:39, Sebastian Noack wrote: > Shouldn't this rather be a property on the ElemHide object? Done.
Patch Set 3: Use a hash function https://codereview.adblockplus.org/29714638/diff/29716606/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/include.preload.js#... include.preload.js:476: if (!this.selectorsChanged(selectors, "emulated")) This will go well with the collapsing selectors: https://codereview.adblockplus.org/29670575/ We can just keep a hash of the current selectors, then just update it. I think I could update the hash function to do incremental hashing. https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js File lib/hash.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js#newcode24 lib/hash.js:24: function SuperFastHash(message) This hash function is 4 times as fast as MD5 on Node.js. It's used by Google Chrome internally for non-crypto hashes, we should adopt in our code as well.
Couldn't we just make the background page remove the old style sheet, before inserting a new one, and regardless of whether it has changed?
On 2018/03/07 19:38:51, Sebastian Noack wrote: > Couldn't we just make the background page remove the old style sheet, before > inserting a new one, and regardless of whether it has changed? We could do that, but there are two issues: 1. We'd have to remove the old style sheet before adding the new one, which could cause elements to reappear momentarily before being hidden again 2. DOM mutations can happen every few milliseconds, which means we'd be removing and re-adding style sheets (running all the associated browser code) too frequently and really unnecessarily Given #1 and #2 above, this would be bad for performance and user experience.
On 2018/03/08 15:21:19, Manish Jethani wrote: > On 2018/03/07 19:38:51, Sebastian Noack wrote: > > Couldn't we just make the background page remove the old style sheet, before > > inserting a new one, and regardless of whether it has changed? > > We could do that, but there are two issues: > > 1. We'd have to remove the old style sheet before adding the new one, which > could cause elements to reappear momentarily before being hidden again > > 2. DOM mutations can happen every few milliseconds, which means we'd be > removing and re-adding style sheets (running all the associated browser code) > too frequently and really unnecessarily > > Given #1 and #2 above, this would be bad for performance and user experience. So let's consider that there are three selector groups: "standard", "emulated", and "collapsing" (the last one is just a proposal for now). We may have to remove the style sheet and add it back for the standard selectors, since there are too many of them (18,000+ in EasyList+AA) and they are not updated frequently. The proposed collapsing selectors are append-only by design (see review 29670575) and we are already checking for new selectors in that review. This problem really exists only for emulated selectors, because they can be updated so frequently (especially once we start using selectors instead of the style attribute). Typically that aren't that many emulated selectors since these filters are usually site-specific and the selector is added only if there's a match (unlike the standard selectors that are added blindly).
On 2018/03/08 15:21:19, Manish Jethani wrote: > On 2018/03/07 19:38:51, Sebastian Noack wrote: > > Couldn't we just make the background page remove the old style sheet, before > > inserting a new one, and regardless of whether it has changed? > > We could do that, but there are two issues: > > 1. We'd have to remove the old style sheet before adding the new one, which > could cause elements to reappear momentarily before being hidden again Could you actually reproduce any (additional) flickering when you remove the style sheet first? If the background page instructs the browser to remove one style sheet and then adds another one before the former even got removed, I could imagine that the browser wouldn't care to redraw the viewport in between. And even if there are some slight side effects, these would only occur in case of DOM modifications on websites with active element hiding emulation filters, where we have some flickering anyway, 1. since we delay reapplying emulation filters after DOM modifications, 2. if the resulting style sheet changed, and if the the viewport gets in fact redrawn in between style sheet changes, we have an inconsistent state anyway regardless whether we first add the new style sheet or first remove the old one. > 2. DOM mutations can happen every few milliseconds, which means we'd be > removing and re-adding style sheets (running all the associated browser code) > too frequently and really unnecessarily True, but we already have a delay in place to not reapply element hiding emulation filters more often than every 3 seconds. Not to mention that the whole element hiding emulation thing isn't particular performance efficient anyway. So that another operation, up to every 3 seconds, in case of DOM modifications unrelated to the emulation filters, shouldn't make much of a difference. Also considering Review 29717611 / Issue 6455, I feel that the logic becomes ridiculously complex, just for some micro-optimization (as it seems), avoiding to remove the style sheet first on every update.
On 2018/03/08 20:04:57, Sebastian Noack wrote:
> On 2018/03/08 15:21:19, Manish Jethani wrote:
> > On 2018/03/07 19:38:51, Sebastian Noack wrote:
> > > Couldn't we just make the background page remove the old style sheet,
before
> > > inserting a new one, and regardless of whether it has changed?
> >
> > We could do that, but there are two issues:
> >
> > 1. We'd have to remove the old style sheet before adding the new one,
which
> > could cause elements to reappear momentarily before being hidden again
>
> Could you actually reproduce any (additional) flickering when you remove the
> style sheet first? If the background page instructs the browser to remove one
> style sheet and then adds another one before the former even got removed, I
> could imagine that the browser wouldn't care to redraw the viewport in
between.
Chrome uses messaging to communicate between the extension and the process that
handles the page's rendering. It's possible, at least in theory, that
tabs.removeCSS and tabs.insertCSS make it to the renderer one or more
invalidations apart as the whole thing is multithreaded. In any case, it might
cause unnecessary invalidations.
I'll try to make a reproducible example HTML where it causes flickering.
> And even if there are some slight side effects, these would only occur in case
> of DOM modifications on websites with active element hiding emulation filters,
> where we have some flickering anyway, 1. since we delay reapplying emulation
The flickering we have anyway is because of new elements that need to be hidden.
Now the flickering I'm talking about is because "nothing changed". If nothing
changed, why should there be flickering? Even if there's no visible flickering,
I don't see why we should just keep thrashing Chrome with requests for removing
and re-adding the same style sheet.
> filters after DOM modifications, 2. if the resulting style sheet changed, and
if
> the the viewport gets in fact redrawn in between style sheet changes, we have
an
> inconsistent state anyway regardless whether we first add the new style sheet
or
> first remove the old one.
The difference between the two inconsistent states of course is that in one case
the same element is hidden and then unhidden, while in the other case it's just
delayed application of the hiding rule.
For example, let's say before it's "A, B, C, D, E { display: none }" and after
it's "B, C, D, E, F { display: none }", adding first means only A is affected
(it is removed after one or a few invalidation cycles), while removing first
means B, C, D, and E are affected (unhidden and then hidden again for no
reason). Also in the first case A is "affected" in theory but in terms of the
actual user experience it doesn't matter, the element just gets shown a few
milliseconds or even a second or two later.
It also depends on what A is. If it's an ID selector or something, that's fine,
but if it's a compound selector of the kind that's generated by element hiding
emulation (nth-child, etc.), we'll have to see how it works in practice.
Anyway I think using user styles sheets for -abp-has and -abp-contains may be
more challenging than I had initially assumed because of these issues.
> > 2. DOM mutations can happen every few milliseconds, which means we'd be
> > removing and re-adding style sheets (running all the associated browser
code)
> > too frequently and really unnecessarily
>
> True, but we already have a delay in place to not reapply element hiding
> emulation filters more often than every 3 seconds. Not to mention that the
whole
> element hiding emulation thing isn't particular performance efficient anyway.
So
Yeah, that's the point, I want to make it performance efficient (several
patches).
> that another operation, up to every 3 seconds, in case of DOM modifications
> unrelated to the emulation filters, shouldn't make much of a difference.
>
> Also considering Review 29717611 / Issue 6455, I feel that the logic becomes
> ridiculously complex, just for some micro-optimization (as it seems), avoiding
> to remove the style sheet first on every update.
I think patch #29717611 addresses the issue correctly on the side of the
background page. It makes a tradeoff, I think that's a good tradeoff. We can use
the first-add-then-remove order in the case of emulated selectors but only if
the old and new style sheets do not match.
On 2018/03/08 22:18:59, Manish Jethani wrote: > > [...] > > I think patch #29717611 addresses the issue correctly on the side of the > [...] To summarize: 1. I'll try to come up with a working example where there is some flickering or some other related performance issue 2. If #29717611 lands then we should be OK, though I'd still like to avoid the round trip to the background page
On 2018/03/08 22:18:59, Manish Jethani wrote: > It also depends on what A is. If it's an ID selector or something, that's fine, > but if it's a compound selector of the kind that's generated by element hiding > emulation (nth-child, etc.), we'll have to see how it works in practice. > > Anyway I think using user styles sheets for -abp-has and -abp-contains may be > more challenging than I had initially assumed because of these issues. Yeah, that is the case, I had in mind. With the :nth-child() selectors generated by :-abp-has()/:-abp-contains(), it is more like A, B, C vs D, E, F in which case it doesn't seem to make any difference whether you first remove the false positives or the false negatives. > Yeah, that's the point, I want to make it performance efficient (several > patches). I'm not convinced that this is worth it, or even possible. Emulated hiding filters are quite excessive by their nature, and while running all the required logic on every document and for every DOM mutation, on websites using them, I don't see how this could ever become remotely as efficient as static element hiding filters. Moreover, emulated element hiding filters are more a stop gap solution, only used on very few websites as a last resort. On 2018/03/08 22:24:25, Manish Jethani wrote: > 1. I'll try to come up with a working example where there is some flickering > or some other related performance issue > > 2. If #29717611 lands then we should be OK, though I'd still like to avoid the > round trip to the background page I'd be happy to see a proof of concept that demonstrates visible artifacts when removing the style sheet first and unconditionally.
On 2018/03/11 00:16:02, Sebastian Noack wrote: > On 2018/03/08 22:18:59, Manish Jethani wrote: > > It also depends on what A is. If it's an ID selector or something, that's > fine, > > but if it's a compound selector of the kind that's generated by element hiding > > emulation (nth-child, etc.), we'll have to see how it works in practice. > > > > Anyway I think using user styles sheets for -abp-has and -abp-contains may be > > more challenging than I had initially assumed because of these issues. > > Yeah, that is the case, I had in mind. With the :nth-child() selectors generated > by :-abp-has()/:-abp-contains(), it is more like A, B, C vs D, E, F in which > case it doesn't seem to make any difference whether you first remove the false > positives or the false negatives. See my comment here: https://issues.adblockplus.org/ticket/6422#comment:1 It's not going to be A, B, C vs. D, E, F, I am quite certain about that. When there's a DOM mutation only a fraction of the DOM has changed, there's no need to update all the selectors. > > Yeah, that's the point, I want to make it performance efficient (several > > patches). > > I'm not convinced that this is worth it, or even possible. Emulated hiding > filters are quite excessive by their nature, and while running all the required > logic on every document and for every DOM mutation, on websites using them, I Well it's definitely possible to do some very obvious optimizations, see this patch for example (already LGTM'd): https://codereview.adblockplus.org/29712655/ So I have a list of optimizations, I guess I should finish with all of them before coming back to this patch. > don't see how this could ever become remotely as efficient as static element > hiding filters. Moreover, emulated element hiding filters are more a stop gap > solution, only used on very few websites as a last resort. Isn't this backwards? We have a very powerful kind of filter, we don't use it because ... it doesn't perform well? Well, then let's make it perform well. There's no substitute for -abp-contains(/(foo|bar)/) in CSS. It's available as an option, it affords very effective ad blocking, why not make it perform well and then promote its use? "as efficient as static element hiding" is not the same as "as efficient as it can be"? > On 2018/03/08 22:24:25, Manish Jethani wrote: > > 1. I'll try to come up with a working example where there is some flickering > > or some other related performance issue > > > > 2. If #29717611 lands then we should be OK, though I'd still like to avoid > the > > round trip to the background page > > I'd be happy to see a proof of concept that demonstrates visible artifacts when > removing the style sheet first and unconditionally. Ack.
https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js File lib/hash.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js#newcode24 lib/hash.js:24: function SuperFastHash(message) On 2018/03/07 16:03:22, Manish Jethani wrote: > This hash function is 4 times as fast as MD5 on Node.js. It's used by Google > Chrome internally for non-crypto hashes, we should adopt in our code as well. Cool, but please add some unit tests.
https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js File lib/hash.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js#newcode24 lib/hash.js:24: function SuperFastHash(message) On 2018/03/19 20:49:31, kzar wrote: > On 2018/03/07 16:03:22, Manish Jethani wrote: > > This hash function is 4 times as fast as MD5 on Node.js. It's used by Google > > Chrome internally for non-crypto hashes, we should adopt in our code as well. > > Cool, but please add some unit tests. I'd rather not add this hash function. It just mitigates the memory usage, but doesn't remove any complexity (actually it does the opposite). I'm not even sure how much of an optimization it is. We trade in some memory usage in case of emulated filters vs. additional cpu cycles and a fair amount of additional code to be loaded/parsed for each document no matter what.
https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js File lib/hash.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js#newcode24 lib/hash.js:24: function SuperFastHash(message) On 2018/03/19 21:13:11, Sebastian Noack wrote: > On 2018/03/19 20:49:31, kzar wrote: > > On 2018/03/07 16:03:22, Manish Jethani wrote: > > > This hash function is 4 times as fast as MD5 on Node.js. It's used by Google > > > Chrome internally for non-crypto hashes, we should adopt in our code as > well. > > > > Cool, but please add some unit tests. > > I'd rather not add this hash function. It just mitigates the memory usage, but > doesn't remove any complexity (actually it does the opposite). > > I'm not even sure how much of an optimization it is. We trade in some memory > usage in case of emulated filters vs. additional cpu cycles and a fair amount of > additional code to be loaded/parsed for each document no matter what. Yeah, I have to agree with Sebastian that it's not making sense for our use case. There are only a handful of emulated selectors right now, we can just compare the strings as in my initial patch.
https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js File lib/hash.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js#newcode24 lib/hash.js:24: function SuperFastHash(message) On 2018/03/20 11:54:22, Manish Jethani wrote: > On 2018/03/19 21:13:11, Sebastian Noack wrote: > > On 2018/03/19 20:49:31, kzar wrote: > > > On 2018/03/07 16:03:22, Manish Jethani wrote: > > > > This hash function is 4 times as fast as MD5 on Node.js. It's used by > Google > > > > Chrome internally for non-crypto hashes, we should adopt in our code as > > well. > > > > > > Cool, but please add some unit tests. > > > > I'd rather not add this hash function. It just mitigates the memory usage, but > > doesn't remove any complexity (actually it does the opposite). > > > > I'm not even sure how much of an optimization it is. We trade in some memory > > usage in case of emulated filters vs. additional cpu cycles and a fair amount > of > > additional code to be loaded/parsed for each document no matter what. > > Yeah, I have to agree with Sebastian that it's not making sense for our use > case. There are only a handful of emulated selectors right now, we can just > compare the strings as in my initial patch. Also one more thing: I'm beginning to think that this should all be done on the Core side, so if addSelectors is called at all then the selectors have definitely changed (or Core doesn't care if they haven't changed, either way).
https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js File lib/hash.js (right): https://codereview.adblockplus.org/29714638/diff/29716606/lib/hash.js#newcode24 lib/hash.js:24: function SuperFastHash(message) On 2018/03/20 11:57:43, Manish Jethani wrote: > On 2018/03/20 11:54:22, Manish Jethani wrote: > > On 2018/03/19 21:13:11, Sebastian Noack wrote: > > > On 2018/03/19 20:49:31, kzar wrote: > > > > On 2018/03/07 16:03:22, Manish Jethani wrote: > > > > > This hash function is 4 times as fast as MD5 on Node.js. It's used by > > Google > > > > > Chrome internally for non-crypto hashes, we should adopt in our code as > > > well. > > > > > > > > Cool, but please add some unit tests. > > > > > > I'd rather not add this hash function. It just mitigates the memory usage, > but > > > doesn't remove any complexity (actually it does the opposite). > > > > > > I'm not even sure how much of an optimization it is. We trade in some memory > > > usage in case of emulated filters vs. additional cpu cycles and a fair > amount > > of > > > additional code to be loaded/parsed for each document no matter what. > > > > Yeah, I have to agree with Sebastian that it's not making sense for our use > > case. There are only a handful of emulated selectors right now, we can just > > compare the strings as in my initial patch. > > Also one more thing: I'm beginning to think that this should all be done on the > Core side, so if addSelectors is called at all then the selectors have > definitely changed (or Core doesn't care if they haven't changed, either way). By now I am convinced that this will have to be taken care of on the Core side. We already have to keep the selectors updated because of the use of style sheets instead of the style attribute, as well as some optimizations, and we get to know for free if any selectors have changed. This patch for example: https://codereview.adblockplus.org/29728690/ So I'm closing this now. 
|
