Closed Bug 1382925 Opened 7 years ago Closed 7 years ago

stylo: Keep the UA parts of the stylist across changes to document sheets

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 12 obsolete files)

Bug 1382925 - Part 1: Make Servo_StyleSet_NoteStyleSheetsChanged take a set of origins to dirty.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1382925 - Part 2: Add FFI function to get stylesheet origin.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1382925 - Part 3: Pass in relevant origin when style sheet rules change and when author styles are toggled.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1382925 - Part 4: Test origin iteration order by checking @font-face rule cascade.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Servo PR
41 bytes, text/x-github-pull-request
Details | Review 
Right now any time document sheet bits change we rebuild the UA parts of the stylist.  This can be somewhat expensive, since our UA sheets are not small.

We should preserve the UA (and user) parts somehow across page sheet changes.  Maybe even have a separate stylist like we do for XBL?  And in that case, maybe share it across pages...
Blocks: 1382927
Note that this is a much bigger deal with ABP installed, since the corresponding stylist update can take 10s of milliseconds. Sharing it across documents is probably important.
Priority: -- → P1
Blocks: 1386045
Blocks: 1384945
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> Note that this is a much bigger deal with ABP installed, since the
> corresponding stylist update can take 10s of milliseconds. Sharing it across
> documents is probably important.

I think this is a separate topic than what described in comment 0, actually.

bz was talking about that we should avoid refreshing rules from UA and User sheets when we update stylist for document sheet changes, while what you want here is to share the same stylesheets (as well as, maybe stylist) among different documents.

They may end up being fixed in the same way, though (via splitting stylist into multiple so that they can be shared, and we don't need to update them when not necessary).
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> Note that this is a much bigger deal with ABP installed, since the
> corresponding stylist update can take 10s of milliseconds. Sharing it across
> documents is probably important.

But is that also the case for the WebExtensions version of ABP?  Last I heard (which maybe was a year ago) the WebExtensions version wasn't going to add style sheets in the same way, and wouldn't benefit from cascade level caching.

That being said, I think we should still do this.  Separating cascade levels for the one document would already be an improvement, but if we can go the extra step and share them across documents like Gecko's RuleProcessorCache, even better.
Yeah we should share across documents if possible. Stylist rebuilds are killing us on pages that don't do a lot of actual styling, so every little win in that area helps.
My understanding is that the WebExtension version of ABP, and certainly whatever version Bobby has installed, uses separate sheets per document.  See https://perfht.ml/2vreK32

Specifically, it uses nsIDOMWindowUtils::AddSheet via http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/toolkit/components/extensions/ExtensionContent.jsm#309 (sounds to me like this is a webextension, fwiw).

So the ABP sheet can't be shared trivially across documents right now: it's added on a per-document basis.  We could try to do some sort of deduplication/caching/sharing anyway, because it sure would be nice if webextension-added-sheets could be shared like this, but that might be out of scope for stylo per se.  What we can and should do is avoid having to deal with it more than once per page in the stylist.
(In reply to Boris Zbarsky [:bz] from comment #5)
> So the ABP sheet can't be shared trivially across documents right now: it's
> added on a per-document basis.  We could try to do some sort of
> deduplication/caching/sharing anyway, because it sure would be nice if
> webextension-added-sheets could be shared like this, but that might be out
> of scope for stylo per se.  What we can and should do is avoid having to
> deal with it more than once per page in the stylist.

Does this mean that ABP adds its sheet to the user level, and not the author level?  (Because if it's at the author level, then we can't avoid dealing with it more than once.)
> Does this mean that ABP adds its sheet to the user level, and not the author level? 

Good question.  I had missed that this API allowed adding sheets on author or user levels as opposed to user/ua.

So I just grabbed ABP off AMO, and it's not actually using the API I link to above: it's calling addSheet directly.  Presumably this is the non-webextension version?  It's passing USER_SHEET to the addSheet call.

I did find https://downloads.adblockplus.org/devbuilds/adblockplusfirefox/ and check what it's doing.  It's using |cssOrigin: "user"| in its chrome.tabs.insertCSS call, which will do the right thing in webextensions, via the API I linked to above.  So we're good there.
(In reply to Boris Zbarsky [:bz] from comment #7)
> > Does this mean that ABP adds its sheet to the user level, and not the author level? 
> 
> Good question.  I had missed that this API allowed adding sheets on author
> or user levels as opposed to user/ua.
> 
> So I just grabbed ABP off AMO, and it's not actually using the API I link to
> above: it's calling addSheet directly.  Presumably this is the
> non-webextension version?  It's passing USER_SHEET to the addSheet call.
> 
> I did find https://downloads.adblockplus.org/devbuilds/adblockplusfirefox/
> and check what it's doing.  It's using |cssOrigin: "user"| in its
> chrome.tabs.insertCSS call, which will do the right thing in webextensions,
> via the API I linked to above.  So we're good there.

They still insert a different stylesheet per domain it seems, though, so we're probably unable to share them anyway?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> They still insert a different stylesheet per domain it seems, though, so
> we're probably unable to share them anyway?

IIUC, the problem for the legacy ABP is that, it inserts a huge stylesheet, which includes all rules on all websites, as a user stylesheet, so it would be a big pain not to share them between documents. However, if the new ABP splits the rule itself, and provides different stylesheets per domain, the size of the sheet should be significantly smaller, so this may no longer be a big problem. Although more sharing is usually better, we may not need to worry that much in this case.

heycam may know more about the legacy ABP case.
> the size of the sheet should be significantly smaller

Sort of.  The default filter list is at https://easylist-downloads.adblockplus.org/easylist.txt

It contains, as of right now, 30984 CSS filters (the lines containing '##') and 18466 of them apply to all sites (start with '##').

So yes, the sheet is "significantly smaller".  But we're talking a factor of 1.5, not orders of magnitude.
I wonder if the API that we expose to ABP should support requesting adding a style sheet to a unique cascade level (say, between user and author), without any particular requirement on how it might cascade with other similarly added sheets.  That would help us avoid issues of other sheets existing at user or author level causing us to not be able to cache the cascade.

Anyway, I'm going to start looking at this for the non-cross-document sharing of cascade level data first.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Getting the first patch in earlier here, since Emilio wants to based some work on top of it:

https://github.com/servo/servo/pull/17990
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d4dc42f88ec0968ef27ee7db835f645ba5dafe
Some refactoring patches to split out all origin-specific cascade data:

https://github.com/servo/servo/pull/18017
Comment on attachment 8894432 [details]
style: Move Origin into its own file.

https://reviewboard.mozilla.org/r/165612/#review173052

::: servo/components/style/stylesheets/origin.rs:5
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +///! [CSS cascade origins](http://dev.w3.org/csswg/css-cascade/#cascading-origins).

Let's change these links to drafts.csswg.org, otherwise I think tidy will complain.

(Hmm... actually this is pre-existing, so probably doesn't matter. whatever you prefer).
Attachment #8894432 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8894433 [details]
style: Factor out per-origin data storage.

https://reviewboard.mozilla.org/r/165614/#review173054

Looks ok, but can you elaborate (with comments would be even better) why do we need the raw pointer to implement `PerOriginIterMut`? I _think_ we should be able to manage without the unsafe code.

::: servo/components/style/stylesheets/origin.rs:96
(Diff revision 2)
> +        self.author.clear();
> +        self.user.clear();
> +    }
> +}
> +
> +impl<T> Default for PerOrigin<T> where T : Default {

nit: Can just `derive(Default)`, I think.

::: servo/components/style/stylesheets/origin.rs:131
(Diff revision 2)
> +        self.cur += 1;
> +        Some(result)
> +    }
> +}
> +
> +pub struct PerOriginIterMut<'a, T: 'a> {

Hmm... Why does this need to be a raw pointer? It deserves comments, at least.
Attachment #8894433 - Flags: review?(emilio+bugs)
Comment on attachment 8894434 [details]
style: Split style sheet invalidations per-origin.

https://reviewboard.mozilla.org/r/165616/#review173056
Attachment #8894434 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896584 [details]
style: Move cached media query results into per-origin data.

https://reviewboard.mozilla.org/r/167848/#review173058
Attachment #8896584 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896585 [details]
style: Move rule source order counter into per-origin data.

https://reviewboard.mozilla.org/r/167850/#review173060
Attachment #8896585 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8894432 [details]
style: Move Origin into its own file.

https://reviewboard.mozilla.org/r/165612/#review173062

::: commit-message-bb8de:1
(Diff revision 2)
> +style: Move Origin into its own file.r r?emilio

Oh, also (ultra-nit): There's an extra `r` in the commit message.
Comment on attachment 8896586 [details]
style: Move cleared state into per-origin data.

https://reviewboard.mozilla.org/r/167852/#review173064

::: servo/components/style/stylist.rs:203
(Diff revision 1)
> -        // preserve current quirks_mode value
>          self.cascade_data.clear();
>          self.precomputed_pseudo_element_decls.clear();
> -        // We want to keep rule_tree around across stylist rebuilds.
> -        // preserve num_rebuilds value, since it should stay across
> -        // clear()/rebuild() cycles.
> +        self.viewport_constraints = None;
> +
> +        // XXX(heycam) Why do this, if we are preserving the Device?

Yeah... Servo doesn't do much with this either... Wonder if we could just shuffle a bit how we do this stuff in servo and remove `is_device_dirty` entirely.
Attachment #8896586 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896587 [details]
style: Add ability to clear and rebuild individual origins.

https://reviewboard.mozilla.org/r/167854/#review173066

::: servo/components/style/stylist.rs:256
(Diff revision 1)
>  
> -        for (data, _) in self.cascade_data.iter_mut_origins() {
> +        // Determine the origins that actually need updating.
> +        //
> +        // XXX(heycam): What is the relationship between `stylesheets_changed`
> +        // and the `is_cleared` fields on each origin's `CascadeData`?  Can
> +        // we avoid passing in `stylesheets_changed`?

For stylo we can, `stylesheets_changed` should be pretty equivalent to "any origin cleared". But servo doesn't do this clear stuff, so we need to pass it explicitly. Unless you want to implement that of course ;)

::: servo/components/style/stylist.rs:310
(Diff revision 1)
> -        if let Some(ref constraints) = self.viewport_constraints {
> +            if let Some(ref constraints) = self.viewport_constraints {
> -            self.device.account_for_viewport_rule(constraints);
> +                self.device.account_for_viewport_rule(constraints);
> -        }
> +            }
> +        }
>  
> -        extra_data.clear();
> +        // XXX(heycam): Why don't we do this in `Stylist::clear` instead?

Yeah, we probably should. (You reviewed the `extra_data` stuff, I think, btw :P).
Attachment #8896587 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896588 [details]
style: Only flush stylesheet invalidations for origins that have changed.

https://reviewboard.mozilla.org/r/167856/#review173068

::: servo/components/style/stylesheet_set.rs:180
(Diff revision 1)
>          E: TElement,
>      {
>          debug!("StylesheetSet::flush");
>          debug_assert!(self.has_changed());
>  
>          for data in self.invalidation_data.iter_mut_origins() {

what about using `(invalidation_data, origin)` instead of `data.0` and `data.1` below?

::: servo/components/style/stylesheet_set.rs:209
(Diff revision 1)
>          }
>      }
> +
> +    /// Mark the stylesheets for the specified origin as dirty, because
> +    /// something external may have invalidated it.
> +    pub fn force_dirty_origin(&mut self, origin: &Origin) {

This seems unused, but I guess it's used in future patches?
Attachment #8896588 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896589 [details]
style: Fix origin iteration order.

https://reviewboard.mozilla.org/r/167858/#review173070
Attachment #8896589 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896594 [details]
Bug 1382925 - Part 3: Pass in relevant origin when style sheet rules change and when author styles are toggled.

https://reviewboard.mozilla.org/r/167868/#review173072

::: layout/style/ServoStyleSet.h:112
(Diff revision 1)
>    void RecordStyleSheetChange(mozilla::ServoStyleSheet*, StyleSheet::ChangeType);
>  
>    void RecordShadowStyleChange(mozilla::dom::ShadowRoot* aShadowRoot) {
>      // FIXME(emilio): When we properly support shadow dom we'll need to do
>      // better.
> -    ForceAllStyleDirty();
> +    ForceAllStyleDirty(OriginFlags::All);

Can shadow dom have non-author styles anyway? but... Yeah, I guess it doesn't really matter right now.

::: layout/style/ServoStyleSet.cpp:126
(Diff revision 1)
>    bool viewportUnitsUsed = false;
>    const bool rulesChanged =
>      Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), &viewportUnitsUsed);
>  
>    if (rulesChanged) {
> -    ForceAllStyleDirty();
> +    // XXXheycam Should be able to tell which origin to pass in here.

Can you file a followup and reference it from here? Seems relatively straight-forward (would just need to return an OriginFlags from the function)

::: layout/style/ServoStyleSet.cpp:954
(Diff revision 1)
>      PrepareAndTraverseSubtree(aRoot, flags);
>    MOZ_ASSERT(!postTraversalRequired);
>  }
>  
>  void
> -ServoStyleSet::ForceAllStyleDirty()
> +ServoStyleSet::ForceAllStyleDirty(OriginFlags aChangedOrigins)

Maybe this function should be renamed to `ForceStyleDirty`, or something that doesn't mention `All`?
Attachment #8896594 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896593 [details]
Bug 1382925 - Part 2: Add FFI function to get stylesheet origin.

https://reviewboard.mozilla.org/r/167866/#review173074
Attachment #8896593 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896592 [details]
geckolib: Add FFI function to get stylesheet origin.

https://reviewboard.mozilla.org/r/167864/#review173076
Attachment #8896592 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896591 [details]
Bug 1382925 - Part 1: Make Servo_StyleSet_NoteStyleSheetsChanged take a set of origins to dirty.

https://reviewboard.mozilla.org/r/167862/#review173078
Attachment #8896591 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896590 [details]
style: Make Servo_StyleSet_NoteStyleSheetsChanged take a set of origins to dirty.

https://reviewboard.mozilla.org/r/167860/#review173080
Attachment #8896590 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896588 [details]
style: Only flush stylesheet invalidations for origins that have changed.

https://reviewboard.mozilla.org/r/167856/#review173082

::: servo/components/style/stylesheet_set.rs:172
(Diff revision 1)
>      /// Flush the current set, unmarking it as dirty, and returns an iterator
>      /// over the new stylesheet list.
>      pub fn flush<E>(
>          &mut self,
> -        document_element: Option<E>
> +        document_element: Option<E>,
> +        stylist: &mut Stylist,

After looking at the rest of the patches, I _think_ we shouldn't need to pass the stylist down here and clear it.
Comment on attachment 8896595 [details]
geckolib: Only dirty the relevant origin when stylesheets are added or removed.

https://reviewboard.mozilla.org/r/167870/#review173084

Looks good to me, but some questions:

 * Can we add a perf-reftest for this? Perhaps based on (or just) the test-case at bug 1382927?
 * Can we test the stylesheet origin order thing? Maybe we don't have a way to insert stylesheets in a given origin?

Also, if you could test whether we need to pass the stylist down to the stylesheet invalidation code, which I think we don't need to, that'd be amazing :)

With those, r=me. Thanks for splitting them up, that made reviewing this very straight-forward :)
Attachment #8896595 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8894433 [details]
style: Factor out per-origin data storage.

https://reviewboard.mozilla.org/r/165614/#review173054

> Hmm... Why does this need to be a raw pointer? It deserves comments, at least.

I don't think it's possible to write an iterator that returns mutable references to part of an object without unsafe code.  I tried for a while before reading https://stackoverflow.com/questions/25730586/returning-mutable-references-from-an-iterator. :-)
Comment on attachment 8896586 [details]
style: Move cleared state into per-origin data.

https://reviewboard.mozilla.org/r/167852/#review173064

> Yeah... Servo doesn't do much with this either... Wonder if we could just shuffle a bit how we do this stuff in servo and remove `is_device_dirty` entirely.

Yeah, it would be nice to align how Servo and Gecko deals with sheets / device / dirtiness, really.  But I won't tackle that right now.
Comment on attachment 8896587 [details]
style: Add ability to clear and rebuild individual origins.

https://reviewboard.mozilla.org/r/167854/#review173066

> For stylo we can, `stylesheets_changed` should be pretty equivalent to "any origin cleared". But servo doesn't do this clear stuff, so we need to pass it explicitly. Unless you want to implement that of course ;)

Later. :-)

> Yeah, we probably should. (You reviewed the `extra_data` stuff, I think, btw :P).

Oh, of course the `extra_data` currently lives on the `PerDocumentStyleData`.  I don't see why we shouldn't just store it on the `Stylist`, conditionally compiled.  But again, later.  I'll update the comment to say that's what we should do.
Blocks: 1389871
Comment on attachment 8896594 [details]
Bug 1382925 - Part 3: Pass in relevant origin when style sheet rules change and when author styles are toggled.

https://reviewboard.mozilla.org/r/167868/#review173072

> Can shadow dom have non-author styles anyway? but... Yeah, I guess it doesn't really matter right now.

Yeah, I guess once we support shadow tree scoped style sheets we'll need some other mechanism to indicate the particular shadow tree's `CascadeData` that must be cleared, so switching to `OriginFlags::Author` now probably doesn't help much.

> Maybe this function should be renamed to `ForceStyleDirty`, or something that doesn't mention `All`?

I've renamed it to "MarkOriginsDirty".  (Not so much a fan of "Force" in the name, when there is no "automatic" dirtying that it is being compared to.)  I've also made it private since we don't call it externally.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #49)
>  * Can we add a perf-reftest for this? Perhaps based on (or just) the
> test-case at bug 1382927?

I think it's hard to write a perf-reftest for this, since we really need to control what's in the UA or User level sheets...

>  * Can we test the stylesheet origin order thing? Maybe we don't have a way
> to insert stylesheets in a given origin?

From a mochitest we can.
Comment on attachment 8894433 [details]
style: Factor out per-origin data storage.

https://reviewboard.mozilla.org/r/165614/#review173126

I'm not 100% sure the transmutes are needed. r=me anyway.
Attachment #8894433 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896685 [details]
Bug 1382925 - Part 4: Test origin iteration order by checking @font-face rule cascade.

https://reviewboard.mozilla.org/r/167984/#review173128

Thanks!

::: layout/style/test/test_font_face_cascade.html:38
(Diff revision 1)
> +
> +is([...document.fonts].map(f => f.family).join(" "),
> +   '"TestAuthor" "TestUser" "TestAgent"',
> +   "@font-face rules are returned in correct cascade order");
> +
> +SimpleTest.finish();

Do you need `SimpleTest.finish()`? You didn't call `waitForExplicitFinish`.
Attachment #8896685 - Flags: review?(emilio+bugs) → review+
Attachment #8894432 - Attachment is obsolete: true
Attachment #8894433 - Attachment is obsolete: true
Attachment #8894434 - Attachment is obsolete: true
Attachment #8896584 - Attachment is obsolete: true
Attachment #8896585 - Attachment is obsolete: true
Attachment #8896586 - Attachment is obsolete: true
Attachment #8896587 - Attachment is obsolete: true
Attachment #8896588 - Attachment is obsolete: true
Attachment #8896589 - Attachment is obsolete: true
Attachment #8896590 - Attachment is obsolete: true
Attachment #8896592 - Attachment is obsolete: true
Attachment #8896595 - Attachment is obsolete: true
Attached file Servo PR — 

    
    

    
  
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/217e04bd7188
Part 1: Make Servo_StyleSet_NoteStyleSheetsChanged take a set of origins to dirty. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cbbd01a74577
Part 2: Add FFI function to get stylesheet origin. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1ff9809cc7aa
Part 3: Pass in relevant origin when style sheet rules change and when author styles are toggled. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fb2e833fe98d
Part 4: Test origin iteration order by checking @font-face rule cascade. r=emilio

    
    

    
  
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a63fec8ffe91
Update test expectations for fewer timeouts. r=me

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: