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

Issue 29342830: Issue 4057 - Further speedup ElemHide.getSelectorsforDomain (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by kzar
Modified:
3 years, 3 months ago
Visibility:
Public.

Description

Issue 4057 - Further speedup ElemHide.getSelectorsforDomain

Patch Set 1 #

Total comments: 2

Patch Set 2 : Take care to remove filters from lookups #

Total comments: 8

Patch Set 3 : Further improvements #

Patch Set 4 : Rebased, unconditionalSelectors #

Total comments: 18

Patch Set 5 : Speed up ElemHide.remove #

Patch Set 6 : Addressed comments #

Total comments: 9

Patch Set 7 : Avoid extra dirty variable #

Total comments: 4

Patch Set 8 : Addressed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -34 lines) Patch
M lib/elemHide.js View 1 2 3 4 5 6 7 7 chunks +104 lines, -28 lines 0 comments Download
M test/elemHide.js View 1 2 3 1 chunk +2 lines, -6 lines 0 comments Download

Messages

Total messages: 38
kzar
Patch Set 1
3 years, 4 months ago (2016-05-20 05:57:01 UTC) #1
kzar
https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js#newcode178 lib/elemHide.js:178: if (filterId == undefined) I guess we can remove ...
3 years, 4 months ago (2016-05-20 08:11:12 UTC) #2
kzar
Patch Set 2 : Take care to remove filters from lookups https://codereview.adblockplus.org/29342830/diff/29342831/lib/elemHide.js File lib/elemHide.js (right): ...
3 years, 4 months ago (2016-05-20 10:55:17 UTC) #3
Wladimir Palant
There are two parts here: 1) Referencing filters by numerical ID rather than text. 2) ...
3 years, 4 months ago (2016-05-20 12:33:27 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#newcode178 lib/elemHide.js:178: if (filterId == undefined) We always use typeof when ...
3 years, 4 months ago (2016-05-20 13:03:04 UTC) #5
kzar
On 2016/05/20 12:33:27, Wladimir Palant wrote: > There are two parts here: > > 1) ...
3 years, 4 months ago (2016-05-20 16:47:59 UTC) #6
Sebastian Noack
I'm not entirely surprised that the code is quite a bit faster if we have ...
3 years, 4 months ago (2016-05-20 17:16:41 UTC) #7
kzar
Patch Set 3 : Further improvements https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342834/lib/elemHide.js#newcode178 lib/elemHide.js:178: if (filterId == ...
3 years, 4 months ago (2016-05-21 04:43:09 UTC) #8
kzar
On 2016/05/20 17:16:41, Sebastian Noack wrote: > I'm not entirely surprised that the code is ...
3 years, 4 months ago (2016-05-21 06:08:57 UTC) #9
Wladimir Palant
On 2016/05/21 06:08:57, kzar wrote: > I'll update the review when I get a chance, ...
3 years, 4 months ago (2016-05-21 07:12:28 UTC) #10
kzar
On 2016/05/21 07:12:28, Wladimir Palant wrote: > On 2016/05/21 06:08:57, kzar wrote: > > I'll ...
3 years, 4 months ago (2016-05-21 08:02:16 UTC) #11
Wladimir Palant
Actually, there is another change you should try first. The current implementation is slow because ...
3 years, 4 months ago (2016-05-21 12:12:11 UTC) #12
kzar
Cool idea! I just got it working, I found it sped things up but not ...
3 years, 3 months ago (2016-05-23 13:02:55 UTC) #13
kzar
Wow, got the fastest result yet by combining your idea with using filterByKey lookup for ...
3 years, 3 months ago (2016-05-23 13:28:27 UTC) #14
Wladimir Palant
On 2016/05/23 13:28:27, kzar wrote: > Problem is it only worked when using an array ...
3 years, 3 months ago (2016-05-23 13:44:16 UTC) #15
kzar
On 2016/05/23 13:44:16, Wladimir Palant wrote: > What about ... `(Math.random() * 0x00FFFFFF) | 0` ...
3 years, 3 months ago (2016-05-23 16:01:35 UTC) #16
Wladimir Palant
On 2016/05/23 16:01:35, kzar wrote: > Nope, still much slower than if the lookup is ...
3 years, 3 months ago (2016-05-23 18:29:18 UTC) #17
kzar
On 2016/05/23 18:29:18, Wladimir Palant wrote: > If I combine the unconditionalSelectors() idea above with ...
3 years, 3 months ago (2016-05-24 08:42:33 UTC) #18
Sebastian Noack
On 2016/05/24 08:42:33, kzar wrote: > On 2016/05/23 18:29:18, Wladimir Palant wrote: > > If ...
3 years, 3 months ago (2016-05-24 08:59:53 UTC) #19
kzar
So I did some new tests without using the profiler like you suggested: let start ...
3 years, 3 months ago (2016-05-24 09:11:23 UTC) #20
kzar
Above tests were for Chrome 50. Now for Safari 9: Before 235: 19912ms (1x) 235: ...
3 years, 3 months ago (2016-05-24 09:45:00 UTC) #21
kzar
Patch Set 4 : Rebased, unconditionalSelectors https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode256 lib/elemHide.js:256: if (usingGetSelectorsForDomain) I'm ...
3 years, 3 months ago (2016-05-24 14:45:22 UTC) #22
Wladimir Palant
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode204 lib/elemHide.js:204: // The new filter's selector is unconditionally applied to ...
3 years, 3 months ago (2016-05-24 15:16:19 UTC) #23
Sebastian Noack
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode531 lib/elemHide.js:531: if (specificOnly) Why do you still account for specificOnly ...
3 years, 3 months ago (2016-05-24 15:20:08 UTC) #24
Wladimir Palant
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode531 lib/elemHide.js:531: if (specificOnly) On 2016/05/24 15:20:07, Sebastian Noack wrote: > ...
3 years, 3 months ago (2016-05-24 15:26:06 UTC) #25
kzar
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode256 lib/elemHide.js:256: if (usingGetSelectorsForDomain) On 2016/05/24 15:16:19, Wladimir Palant wrote: > ...
3 years, 3 months ago (2016-05-24 17:47:10 UTC) #26
kzar
https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode256 lib/elemHide.js:256: if (usingGetSelectorsForDomain) On 2016/05/24 17:47:09, kzar wrote: > On ...
3 years, 3 months ago (2016-05-24 17:52:12 UTC) #27
kzar
Patch Set 5 : Speed up ElemHide.remove https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode256 lib/elemHide.js:256: if (usingGetSelectorsForDomain) ...
3 years, 3 months ago (2016-05-25 04:57:17 UTC) #28
kzar
Patch Set 6 : Addressed comments https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29342997/lib/elemHide.js#newcode204 lib/elemHide.js:204: // The new ...
3 years, 3 months ago (2016-05-25 05:12:24 UTC) #29
Sebastian Noack
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#newcode509 lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); You ...
3 years, 3 months ago (2016-05-25 07:30:50 UTC) #30
Sebastian Noack
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#newcode509 lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); One ...
3 years, 3 months ago (2016-05-25 07:40:26 UTC) #31
kzar
Patch Set 7 : Avoid extra dirty variable https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#newcode509 lib/elemHide.js:509: let ...
3 years, 3 months ago (2016-05-25 08:05:20 UTC) #32
Sebastian Noack
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#newcode509 lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On ...
3 years, 3 months ago (2016-05-25 08:11:56 UTC) #33
kzar
https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js#newcode509 lib/elemHide.js:509: let selectors = specificOnly ? [] : unconditionalSelectors.slice(); On ...
3 years, 3 months ago (2016-05-25 08:29:04 UTC) #34
Wladimir Palant
I really like the current version, only a few nits below. https://codereview.adblockplus.org/29342830/diff/29343026/lib/elemHide.js File lib/elemHide.js (right): ...
3 years, 3 months ago (2016-05-25 10:43:04 UTC) #35
kzar
Patch Set 8 : Addressed nits https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29342830/diff/29343029/lib/elemHide.js#newcode65 lib/elemHide.js:65: * the array ...
3 years, 3 months ago (2016-05-25 10:51:29 UTC) #36
Wladimir Palant
LGTM
3 years, 3 months ago (2016-05-25 11:01:11 UTC) #37
Sebastian Noack
3 years, 3 months ago (2016-05-25 11:19:37 UTC) #38
LGTM
Sign in to reply to this message.

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