Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(364)

Issue 29714638: Issue 6446 - Ignore emulated selectors if unchanged (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by Manish Jethani
Modified:
1 year, 11 months ago
Reviewers:
kzar, Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -0 lines) Patch
M include.preload.js View 1 2 3 chunks +22 lines, -0 lines 1 comment Download
A lib/hash.js View 1 2 1 chunk +101 lines, -0 lines 6 comments Download

Messages

Total messages: 19
Manish Jethani
1 year, 11 months ago (2018-03-05 22:42:20 UTC) #1
Manish Jethani
Patch Set 1
1 year, 11 months ago (2018-03-05 22:43:08 UTC) #2
Sebastian Noack
I'm not too happy with keeping another copy of the injected selectors in memory. Also ...
1 year, 11 months ago (2018-03-06 20:17:39 UTC) #3
Manish Jethani
On 2018/03/06 20:17:39, Sebastian Noack wrote: > I'm not too happy with keeping another copy ...
1 year, 11 months ago (2018-03-07 06:14:24 UTC) #4
Manish Jethani
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#newcode38 include.preload.js:38: let ...
1 year, 11 months ago (2018-03-07 06:23:05 UTC) #5
Manish Jethani
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#newcode476 include.preload.js:476: if (!this.selectorsChanged(selectors, ...
1 year, 11 months ago (2018-03-07 16:03:22 UTC) #6
Sebastian Noack
Couldn't we just make the background page remove the old style sheet, before inserting a ...
1 year, 11 months ago (2018-03-07 19:38:51 UTC) #7
Manish Jethani
On 2018/03/07 19:38:51, Sebastian Noack wrote: > Couldn't we just make the background page remove ...
1 year, 11 months ago (2018-03-08 15:21:19 UTC) #8
Manish Jethani
On 2018/03/08 15:21:19, Manish Jethani wrote: > On 2018/03/07 19:38:51, Sebastian Noack wrote: > > ...
1 year, 11 months ago (2018-03-08 16:09:35 UTC) #9
Sebastian Noack
On 2018/03/08 15:21:19, Manish Jethani wrote: > On 2018/03/07 19:38:51, Sebastian Noack wrote: > > ...
1 year, 11 months ago (2018-03-08 20:04:57 UTC) #10
Manish Jethani
On 2018/03/08 20:04:57, Sebastian Noack wrote: > On 2018/03/08 15:21:19, Manish Jethani wrote: > > ...
1 year, 11 months ago (2018-03-08 22:18:59 UTC) #11
Manish Jethani
On 2018/03/08 22:18:59, Manish Jethani wrote: > > [...] > > I think patch #29717611 ...
1 year, 11 months ago (2018-03-08 22:24:25 UTC) #12
Sebastian Noack
On 2018/03/08 22:18:59, Manish Jethani wrote: > It also depends on what A is. If ...
1 year, 11 months ago (2018-03-11 00:16:02 UTC) #13
Manish Jethani
On 2018/03/11 00:16:02, Sebastian Noack wrote: > On 2018/03/08 22:18:59, Manish Jethani wrote: > > ...
1 year, 11 months ago (2018-03-15 20:03:54 UTC) #14
kzar
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: > ...
1 year, 11 months ago (2018-03-19 20:49:31 UTC) #15
Sebastian Noack
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 ...
1 year, 11 months ago (2018-03-19 21:13:11 UTC) #16
Manish Jethani
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: > ...
1 year, 11 months ago (2018-03-20 11:54:23 UTC) #17
Manish Jethani
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: > ...
1 year, 11 months ago (2018-03-20 11:57:43 UTC) #18
Manish Jethani
1 year, 11 months ago (2018-03-21 16:20:12 UTC) #19
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5