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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by Manish Jethani
Modified:
2 years ago
Reviewers:
Sebastian Noack, kzar
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
2 years ago (2018-03-05 22:42:20 UTC) #1
Manish Jethani
Patch Set 1
2 years 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 ...
2 years 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 ...
2 years 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 ...
2 years 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, ...
2 years 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 ...
2 years 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 ...
2 years 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: > > ...
2 years 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: > > ...
2 years 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: > > ...
2 years 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 ...
2 years 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 ...
2 years 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: > > ...
2 years 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: > ...
2 years 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 ...
2 years 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: > ...
2 years 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: > ...
2 years ago (2018-03-20 11:57:43 UTC) #18
Manish Jethani
2 years 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