Issue 29679796: Issue 6298 - Split injected CSS hiding rule into groups of 1,024 selectors (Closed)
|
Created:
Jan. 25, 2018, 5:12 p.m. by Manish Jethani Modified:
Jan. 30, 2018, 8 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #
Total comments: 7
Patch Set 2 : Increase group size to 1,024 #Patch Set 3 : Use generator functions #Patch Set 4 : Update comment explaining why we split the selectors into groups of 1,024 #MessagesTotal messages: 12
Patch Set 1 https://codereview.adblockplus.org/29679796/diff/29679797/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/include.preload.js#... include.preload.js:433: // Chromium's Blink engine supports only up to 8,192 selectors; more I've just updated this comment here because the previous one was outdated.
https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js... lib/cssInjection.js:38: // Chromium's Blink engine supports only up to 8,192 selectors; more If we're limited to 8192 why do we go with 200?
https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js... lib/cssInjection.js:38: // Chromium's Blink engine supports only up to 8,192 selectors; more On 2018/01/25 17:46:47, kzar wrote: > If we're limited to 8192 why do we go with 200? It's "up to 8,192" selectors. If you have 8,192 simple selectors ("#foo", ".bar", ...), they'll all be taken into account. If you have 8,191 simple selectors and one compound selector consisting of a million plain selectors ("#foo .bar div a:visited ..."), they will still all work. On the other hand, if you move the last selector from the aforementioned list to the beginning of the list, such that now the list starts with a compound selector consisting of a million plain selectors, then all but the first selector will be ignored. The minimum therefore is 1, the maximum is 8,192. Now ideally we would count the plain selectors the way Chromium does and divide them up correctly so we can have the fewest possible rules, but this is not worth it. It's reasonable to assume that the average selector in EasyList won't exceed a certain size. Maybe 200 is too low a number, but it's been working OK for inline style sheets so there's no good reason to change this number.
https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js... lib/cssInjection.js:38: // Chromium's Blink engine supports only up to 8,192 selectors; more On 2018/01/25 18:42:41, Manish Jethani wrote: > If you have 8,192 simple selectors ("#foo", ".bar", ...), they'll all be taken > into account. If you have 8,191 simple selectors and one compound selector > consisting of a million plain selectors ("#foo .bar div a:visited ..."), they > will still all work. On the other hand, if you move the last selector from the > aforementioned list to the beginning of the list, such that now the list starts > with a compound selector consisting of a million plain selectors, then all but > the first selector will be ignored. This was not technically true by the way but now [1] it is. [1]: https://chromium.googlesource.com/chromium/src.git/+/5814e68d9cf56da285688558...
https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js... lib/cssInjection.js:38: // Chromium's Blink engine supports only up to 8,192 selectors; more On 2018/01/25 18:42:41, Manish Jethani wrote: > On 2018/01/25 17:46:47, kzar wrote: > > If we're limited to 8192 why do we go with 200? > > It's "up to 8,192" selectors. > > If you have 8,192 simple selectors ("#foo", ".bar", ...), they'll all be taken > into account. If you have 8,191 simple selectors and one compound selector > consisting of a million plain selectors ("#foo .bar div a:visited ..."), they > will still all work. On the other hand, if you move the last selector from the > aforementioned list to the beginning of the list, such that now the list starts > with a compound selector consisting of a million plain selectors, then all but > the first selector will be ignored. > > The minimum therefore is 1, the maximum is 8,192. > > Now ideally we would count the plain selectors the way Chromium does and divide > them up correctly so we can have the fewest possible rules, but this is not > worth it. It's reasonable to assume that the average selector in EasyList won't > exceed a certain size. Maybe 200 is too low a number, but it's been working OK > for inline style sheets so there's no good reason to change this number. Now that I think about it, maybe we should increase this from 200 to something less arbitrary and more reasonable like 1,024. This would assume that the average selector in the group can consist of up to 8 plain selectors (e.g. "lorem > ipsum > dolor > sit > amet > consectetur > adipiscing > elit"). This is only the average, most selectors are smaller (usually just one), therefore even if there are a few very long selectors 1,024 should be a reasonable size for the group.
Patch Set 2: Increase group size to 1,024
Patch Set 3: Use generator functions
https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js... lib/cssInjection.js:38: // Chromium's Blink engine supports only up to 8,192 selectors; more On 2018/01/30 06:19:43, Manish Jethani wrote: > On 2018/01/25 18:42:41, Manish Jethani wrote: > > On 2018/01/25 17:46:47, kzar wrote: > > > If we're limited to 8192 why do we go with 200? > > > > It's "up to 8,192" selectors. > > > > If you have 8,192 simple selectors ("#foo", ".bar", ...), they'll all be taken > > into account. If you have 8,191 simple selectors and one compound selector > > consisting of a million plain selectors ("#foo .bar div a:visited ..."), they > > will still all work. On the other hand, if you move the last selector from the > > aforementioned list to the beginning of the list, such that now the list > starts > > with a compound selector consisting of a million plain selectors, then all but > > the first selector will be ignored. > > > > The minimum therefore is 1, the maximum is 8,192. > > > > Now ideally we would count the plain selectors the way Chromium does and > divide > > them up correctly so we can have the fewest possible rules, but this is not > > worth it. It's reasonable to assume that the average selector in EasyList > won't > > exceed a certain size. Maybe 200 is too low a number, but it's been working OK > > for inline style sheets so there's no good reason to change this number. > > Now that I think about it, maybe we should increase this from 200 to something > less arbitrary and more reasonable like 1,024. This would assume that the > average selector in the group can consist of up to 8 plain selectors (e.g. > "lorem > ipsum > dolor > sit > amet > consectetur > adipiscing > elit"). This is > only the average, most selectors are smaller (usually just one), therefore even > if there are a few very long selectors 1,024 should be a reasonable size for the > group. Sounds good to me, but let's rewrite the comment to make that plain: // Chromium's Blink engine does not support rules with more than 8192 // selectors, it discards the excess selectors. We don't know for sure // how many plain selectors are in a selector group, so to be safe we'll // insert the groups in batches of 1024. The assumption is that there // will be less than 8 plain selectors in a group on average. // See issue #6298 and https://crbug.com/804179
https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29679796/diff/29679797/lib/cssInjection.js... lib/cssInjection.js:38: // Chromium's Blink engine supports only up to 8,192 selectors; more On 2018/01/30 11:39:59, kzar1 wrote: > On 2018/01/30 06:19:43, Manish Jethani wrote: > > On 2018/01/25 18:42:41, Manish Jethani wrote: > > > On 2018/01/25 17:46:47, kzar wrote: > > > > If we're limited to 8192 why do we go with 200? > > > > > > It's "up to 8,192" selectors. > > > > > > If you have 8,192 simple selectors ("#foo", ".bar", ...), they'll all be > taken > > > into account. If you have 8,191 simple selectors and one compound selector > > > consisting of a million plain selectors ("#foo .bar div a:visited ..."), > they > > > will still all work. On the other hand, if you move the last selector from > the > > > aforementioned list to the beginning of the list, such that now the list > > starts > > > with a compound selector consisting of a million plain selectors, then all > but > > > the first selector will be ignored. > > > > > > The minimum therefore is 1, the maximum is 8,192. > > > > > > Now ideally we would count the plain selectors the way Chromium does and > > divide > > > them up correctly so we can have the fewest possible rules, but this is not > > > worth it. It's reasonable to assume that the average selector in EasyList > > won't > > > exceed a certain size. Maybe 200 is too low a number, but it's been working > OK > > > for inline style sheets so there's no good reason to change this number. > > > > Now that I think about it, maybe we should increase this from 200 to something > > less arbitrary and more reasonable like 1,024. This would assume that the > > average selector in the group can consist of up to 8 plain selectors (e.g. > > "lorem > ipsum > dolor > sit > amet > consectetur > adipiscing > elit"). This > is > > only the average, most selectors are smaller (usually just one), therefore > even > > if there are a few very long selectors 1,024 should be a reasonable size for > the > > group. > > Sounds good to me, but let's rewrite the comment to make that plain: > > // Chromium's Blink engine does not support rules with more than 8192 > // selectors, it discards the excess selectors. We don't know for sure > // how many plain selectors are in a selector group, so to be safe we'll > // insert the groups in batches of 1024. The assumption is that there > // will be less than 8 plain selectors in a group on average. > // See issue #6298 and https://crbug.com/804179 Unfortunately this comment is confusing and technically incorrect (it's 8 plain selectors per compound selector, not per group), and it still doesn't explain why we can't just add the selectors in batches of 8,192. Here's my second attempt at explaining what we're doing in the comment: // Chromium's Blink engine supports only up to 8,192 simple selectors, and // even fewer compound selectors, in a rule. The exact number of selectors // that would work depends on their sizes (e.g. "#foo .bar" has a size of 2). // Since we don't know the sizes of the selectors here, we simply split them // into groups of 1,024, based on the reasonable assumption that the average // selector won't have a size greater than 8. The alternative would be to // calculate the sizes of the selectors and divide them up accordingly, but // this approach is more efficient and has worked well in practice. In theory // this could still lead to some selectors not working on Chromium, but it is // highly unlikely. // See issue #6298 and https://crbug.com/804179
Patch Set 4: Update comment explaining why we split the selectors into groups of 1,024 What do you think, Dave?
Assuming you tested this it LGTM 
|
