As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 77999 - share rule processors for XUL, scoped, UA, and user stylesheets
: share rule processors for XUL, scoped, UA, and user stylesheets
Status: RESOLVED FIXED
[nav+perf][MemShrink:P2]
: footprint, perf
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 normal with 24 votes (vote)
: mozilla41
Assigned To: Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 1169512 1169514 1169525 1171342 1178155 1196379 1209124
Blocks: 385229 798137 904836 49141 91351 681201 988266 1177563
  Show dependency treegraph
 
Reported: 2001-04-27 16:38 PDT by Cathleen
Modified: 2016-11-11 02:03 PST (History)
57 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
41+


Attachments
Part 1: Add nsDocumentRuleResultCacheKey. (7.12 KB, patch)
2015-06-04 05:21 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
dbaron: review+
Details | Diff | Splinter Review
Part 2: Add RuleProcessorCache. (25.59 KB, patch)
2015-06-04 05:21 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
dbaron: review+
Details | Diff | Splinter Review
Part 3: Gather document rules and produce an nsDocumentRuleResultCacheKey in nsCSSRuleProcessors. (12.79 KB, patch)
2015-06-04 05:22 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
dbaron: review+
Details | Diff | Splinter Review
Part 4: Mark nsCSSRuleProcessors as ready for expiration from the RuleProcessorCache once no nsStyleSets are using them. (3.59 KB, patch)
2015-06-04 05:22 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
dbaron: review+
Details | Diff | Splinter Review
Part 5: Cache eAgentSheet and eUserSheet rule processors in the RuleProcessorCache. (6.55 KB, patch)
2015-06-04 05:22 PDT, Cameron McCormack (:heycam) (away 27 Jan–1 Feb)
dbaron: review+
Details | Diff | Splinter Review

Description User image Cathleen 2001-04-27 16:38:17 PDT
 
Comment 1 User image Peter Trudelle 2001-06-01 16:53:47 PDT
->0.9.3, but probably not a limbo candidate (risk)
Comment 2 User image Cathleen 2001-06-01 19:26:02 PDT
hyatt, how much will this help improve window open and what's the scope of 
fixing this?  -thanks!
Comment 3 User image Peter Trudelle 2001-12-01 13:07:33 PST
Will this really land by Dec 11?  Is it required for mozilla1.0?  MachV?  Will
it affect Gecko? 
Comment 4 User image Cathleen 2001-12-03 20:17:43 PST
estimate 5-6% win on new window. needed for machV and moz1.0
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2003-04-07 21:39:40 PDT
So what sort of caching are we talking about here?  Are we talking caching in
the particular style set (so that ClearRuleProcessors() will not clear the
cache, necessarily)?  Or are we caching globally?
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2011-11-08 19:50:52 PST
So an interesting question here is what to do about the prescontext dependency the rule cascade has...
Comment 7 User image David Baron :dbaron: ⌚️UTC-8 2011-11-08 19:58:33 PST
Somehow turn it into a "set of decisions regarding conditions", perhaps somehow related to media query cache keys...
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2011-11-08 21:15:56 PST
OK, so the prescontext only matters for the following purposes as far as I can tell:

1)  Media query matching.  RuleCascadeData::mCacheKey covers this already.
2)  Quirks vs standards mode.  This could just be part of the data we check when looking
    for a rule cascade match, along with the media query key.  We could even make this
    into a -moz media query, if desired, and get rid of the manual quirk stylesheet
    disabled state munging.
3)  @-moz-document rules.

David, anything else you can think of?

It seems like we'd want some sort of expiration policy here too, by the way; otherwise separate rule cascades due to @-moz-document rules would stick around forever.  It might be enough to limit the expiration to rule cascades that matched some @-moz-document rules.

So what we really need to figure out here is how to express @-moz-document rules.  Most simply, just have an array of them and for each cascade have a corresponding array of match/not-match booleans.  But that can get slow and bloaty when there are lots of @-moz-document rules...
Comment 9 User image David Baron :dbaron: ⌚️UTC-8 2011-11-08 21:48:24 PST
I'm not sure if "cache" is even the word I want here -- how about "share" instead?
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2011-11-08 21:52:49 PST
I was assuming that was what was meant anyway.  ;)
Comment 11 User image Jonathan Watt [:jwatt] 2014-05-23 08:09:49 PDT
Bug 999931 comment 8 provides some info that seems to indicate that fixing this could save hundreds of kilobytes per document.
Comment 12 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-04-16 23:00:54 PDT
If we want to ensure that the ABP style sheets don't cause problems, with their 1000s of @-moz-document rules, then I think we'll need to handle/store them separately from the MQ cache key.  We won't want to re-check the result of all @-moz-document rules when we had a media feature change (such as a window resize).

I agree the naive approach of storing a big array of all DocumentRule objects and a parallel array of booleans for whether they matched is going to be bloaty for ABP (though still less bloaty than what we're doing currently!).  I think we are going to have fairly few matching @-moz-document rules per RuleCascadeData object, so if we can store these results as a "here's a list of matching rules" rather than listing all rules, that'd be better.  We can store the full list of rules on the rule processor itself, though we'll need to be careful of @-moz-document rules inside failing @supports or @media or other @-moz-document rules (but this case should also be not common, I reckon).

So how about this:

* When we are building the rule processor's first rule cascade (or the first after a
  ClearRuleCascades), then we gather up all of the DocumentRules we find into an array on the
  rule processor.  If we encounter a failing @supports or @media or @-moz-document, then we keep
  traversing into those rules, but just for the purpose of building this array.  Now, every
  DocumentRule in the style sheets represented by the rule processor has an integer index.
  We perhaps store this integer on the DocumentRule itself.

* When building a RuleCascadeData, we store the @-moz-document-related cache key as an array of
  rules that we matched (by their index) and an array of rules whose result we don't care about.
  These arrays should be relatively short with the content we're expecting.

* When checking whether a cached RuleCascadeData is valid for the pres context, we iterate over the
  DocumentRule objects in the rule processor's array, while simultaneously iterating over the
  cache key's arrays of matching and don't-care DocumentRule indexes.  We check or skip DocumentRules
  as appropriate.

Now, I think we might want to try to avoid doing this document rule cache key checking when we can help it.  So hopefully we can know whether the last result is still valid.


And if we really wanted to be clever, we could use the fact that @-moz-document url() rules (with different URLs in them) are mutually exclusive, and look up a hash table to turn the URL into its corresponding DocumentRule index, and thus know that all other @-moz-document url() rules must be failing.  And we could use a suffix tree to do matching of all @-moz-document url-prefix() rules simultaneously, and something similar for @-moz-document domain() (in reverse).  regexp() would be harder. :-)

But that might be over-engineering before we see how the above plan goes.
Comment 13 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-04-16 23:05:08 PDT
Since rule processors aren't shared across multiple pres contexts (except in the XBL case), we have this behaviour where the most recently used RuleCascadeData is the one at mRuleCascades, which means that for each pixel we resize a window and stay within the same set of matching @media rules, we don't have to look at any other RuleCascadeData in the list.  With rule processors shared across pres contexts, we might want to have a most-recently-used cache per pres context (or maybe per style set is a more accurate way of framing it).

Maybe we should expose the RuleCascadeData, or a handle on to it, to the pres context or style set, so that it can explicitly hold on to the last one it used?  That would give us the ability to track whether a RuleCascadeData is being used at all, which is something we'll need for the expiry mechanism.
Comment 14 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-04-18 17:05:43 PDT
Assigning integer indexes to each DocumentRule won't work, since style sheets can be shared across different rule processors and might be in different positions.

Instead we could keep the DocumentRule pointers sorted in the rule processor's array, letting us keep the simultaneous iteration over the rule processor's array and the RuleCascadeData's arrays.
Comment 15 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-04-20 22:14:04 PDT
I've started working on this in a branch here:

https://github.com/heycam/gecko-dev/commits/share-rule-processors
Comment 16 User image Boris Zbarsky [:bz] (still a bit busy) 2015-05-14 11:47:30 PDT
> If we encounter a failing @supports or @media or @-moz-document

Or, when traversing children, a failing media condition on an @import?
Comment 17 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-05-14 15:36:54 PDT
(In reply to Not doing reviews right now from comment #16)
> Or, when traversing children, a failing media condition on an @import?

Oh, good point, we'll need to handle that in nsCSSRuleProcessor::CascadeSheet.
Comment 18 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-03 18:57:35 PDT
I have some patches now that work, but I am a bit concerned about the time needed to evaluate a pres context against each "document cache key", which is the list of matching and non-matching @-moz-document rules from a given list of style sheets.  On my couple of years old 3.2 Ghz i7-3930K, it can take a couple of milliseconds to go over the entire list of 6,000 @-moz-document rules that ABP has in its style sheet and evaluate them against the document URL.

OTOH, the time to build the RuleCascadeData, which we're saving by looking up an existing rule processor, is on the order of 15-20ms, so we're still winning.

I think I'll work on speeding up the @-moz-document rule matching in a followup, once the initial work has landed.
Comment 19 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 00:05:33 PDT
Although I'm not going to do it in this bug, we should see whether we can cache rule processors for document style sheets too.  This should be feasible, as while the CSS Loader returns a clone of any cached style sheets, they'll have the same inner unless inspected/modified, so we could make the cache look up match on lists of style sheets based on their inners.
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2015-06-04 00:08:37 PDT
Doing that while respecting HTTP semantics (e.g. no-cache headers) might take a bit of work, unfortunately.
Comment 21 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 00:31:15 PDT
Oh, Loader caches sheets per document, not globally (due to each document having its own Loader).  Yeah, that makes it not straightforward.
Comment 22 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 05:21:19 PDT
Created attachment 8615272 [details] [diff] [review]
Part 1: Add nsDocumentRuleResultCacheKey.
Comment 23 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 05:21:44 PDT
Created attachment 8615273 [details] [diff] [review]
Part 2: Add RuleProcessorCache.
Comment 24 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 05:22:06 PDT
Created attachment 8615274 [details] [diff] [review]
Part 3: Gather document rules and produce an nsDocumentRuleResultCacheKey in nsCSSRuleProcessors.
Comment 25 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 05:22:30 PDT
Created attachment 8615275 [details] [diff] [review]
Part 4: Mark nsCSSRuleProcessors as ready for expiration from the RuleProcessorCache once no nsStyleSets are using them.
Comment 26 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 05:22:55 PDT
Created attachment 8615276 [details] [diff] [review]
Part 5: Cache eAgentSheet and eUserSheet rule processors in the RuleProcessorCache.
Comment 27 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-04 05:23:51 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c261cd4cb95
Comment 28 User image David Baron :dbaron: ⌚️UTC-8 2015-06-09 17:32:46 PDT
Comment on attachment 8615272 [details] [diff] [review]
Part 1: Add nsDocumentRuleResultCacheKey.

> nsDocumentRuleResultCacheKey is analagous to nsMediaQueryResultCacheKey
> and stores the result of matching the @-moz-document rules from a set
> of style sheets.  In a later patch, nsCSSRuleProcessor will build up
> an nsDocumentRuleResultCacheKey as it visits the @-moz-document rules
> while building its RuleCascadeData.
> 
> Rather than represent the result using a list of both the matching and
> non-matching rules, we just store the matched rules.  The assumption is
> that in situations where we have a large number of rules -- such as the
> thousands added by AdBlock Plus -- that only a small number will be
> matched.  Thus to check if the nsDocumentRuleResultCacheKey matches a
> given nsPresContext, we also need the entire list of @-moz-document
> rules to know which rules must not match.

This probably all belongs in a code comment, probably in nsIMediaList.h
above the class definition.

nsIMediaList.h:
>+  bool operator==(const nsDocumentRuleResultCacheKey& aOther) const {
>+    MOZ_ASSERT(mFinalized);

Add the same assertion for aOther.mFinalized as well, I think?

nsCSSStyleSheet.cpp:

Should the presence of a rule in mMatchingRules that is not in aRules
lead to an assertion?  If not, maybe explain why it's ok and what it
means?

>+    while (pm < pm_end && *pm < *pr) ++pm;

{} and three lines, please

Unless, that is, the ++pm could just be an else on the if just below
(depending on your answer about the assertion, in which case the
assertion could assert that pm ends up at pm_end).
Comment 29 User image David Baron :dbaron: ⌚️UTC-8 2015-06-10 17:43:10 PDT
Comment on attachment 8615273 [details] [diff] [review]
Part 2: Add RuleProcessorCache.

> The RuleProcessorCache is a singleton object that caches
> nsCSSRuleProcessors keyed off a list of style sheets and the result of
> evaluating all @-moz-documents in the style sheets.  In a later patch,
> nsStyleSet will get and put nsCSSRuleProcessors from/to the
> RuleProcessorCache.
> 
> We add some state bits to CSSStyleSheet and nsCSSRuleProcessor to track
> whether they are in the RuleProcessorCache.  This lets us remove them
> from the RuleProcessorCache when they're going away.

Seems like this should go above the definition of the class in
RuleProcessorCache.h


>+      nsTArray<css::DocumentRule*>&& aDocumentRules,

Maybe name this either aDocumentRulesInSheets?

(both PutRuleProcessor and DoPutRuleProcessor, and Entry too)

Or comment that it's the list of all document rules in the list of
sheets?

>+  size_t DoSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);

This is (thankfully) never defined and never used; please remove
the declaration.

In DoRemoveSheet:
>+  auto last = std::remove_if(mEntries.begin(), mEntries.end(), HasSheet(aSheet));

It would probably be useful to give the type explicitly here, and also
break the line after the =.

That said, I don't think this is a valid use of remove_if.  C++ 2011
says:

  Note: each element in the range [ret,last), where ret is the returned
  value, has a valid but unspecified state, because the algorithms can
  eliminate elements by swapping with or moving from elements that were
  originally in that range.
   -- 25.3.8 [alg.remove], clause 6

Using partition or stable_partition instead would be valid, but has more
complexity than you need.

Would it be safe to do the SetInRuleProcessorCache / RemoveFromTracker
in a renamed HasSheet predicate?  I think it would be.  Then you could
just remove_if() and TruncateLength().

RuleProcessorCache::DoPutRuleProcessor should almost certainly assert
that !aRuleProcessor->IsInRuleProcessorCache().

Should RuleProcessorCache::DoRemoveRuleProcessor assert if it falls
through to the end (i.e., doesn't find the rule processor)?

Should RuleProcessorCache::SizeOfIncludingThis really be counting
the rule processor objects themselves?  Perhaps it should... but if
so, doesn't something else need to *stop* counting them?

Maybe rename ExpirationTracker::RemoveFromTracker to
RemoveObjectIfTracked to make it clearer how it differs from
RemoveObject -- especially at the caller side.

Is it necessary to have the aIsShared boolean parameter to the
nsCSSRuleProcessor constructor?  It's not used in this patch.  Maybe
it's used later?  If so, maybe a setter makes more sense -- even if used
after the constructor -- just because it avoids boolean parameters.
(Or is mIsShared intended to be immutable?  If so, maybe it should be
"const bool".)

I think the ClearRuleCascades change us sufficient for what you're
doing there; I looked for other cases, but I think the other cases
I was thinking of involve creation of a new rule processor because
the set of style sheets has changed.

r=dbaron with that
Comment 30 User image David Baron :dbaron: ⌚️UTC-8 2015-06-10 18:00:00 PDT
I don't understand what setting mMustGatherDocumentRules to false in RefreshRuleCascade does (in patch 3) -- what happens if the document rules change and we call RefreshRuleCascade again?
Comment 31 User image David Baron :dbaron: ⌚️UTC-8 2015-06-11 16:48:52 PDT
Comment on attachment 8615275 [details] [diff] [review]
Part 4: Mark nsCSSRuleProcessors as ready for expiration from the RuleProcessorCache once no nsStyleSets are using them.

You should use MozRefCountType for reference counts unless you have a very good reason to do otherwise.

Probably also good to assert in ReleaseStyleSetRef that mStyleSetRefCnt isn't 0 to begin with.

r=dbaron with that
Comment 32 User image David Baron :dbaron: ⌚️UTC-8 2015-06-11 17:11:37 PDT
Comment on attachment 8615276 [details] [diff] [review]
Part 5: Cache eAgentSheet and eUserSheet rule processors in the RuleProcessorCache.

File a followup on caching author sheets as well -- or at least measuring
whether it would be useful.

It would be nice to use a smart pointer for the AddStyleSetRef /
ReleaseStyleSetRef business, but I guess we don't have an appropriately
extensible one.

Please declare nsStyleSet as final given that you're giving it a
non-virtual public destructor.

Also, in part 4, please make ~nsCSSRuleProcessor assert that mStyleSetRefCnt
is 0.

This breaks media feature change handling for UA and user sheets when
combined with certain other dynamic changes, since MediumFeaturesChanged
depends on whether the current rule cascade changed to determine whether
to report that a change happened.  We have other problems with that
mechanism (bug 1089417), though.  I suspect you can get away with that
since UA and user sheets shouldn't be changing dynamically, but you should
at least file a followup bug.

r=dbaron with that
Comment 33 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-14 20:15:22 PDT
(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #30)
> I don't understand what setting mMustGatherDocumentRules to false in
> RefreshRuleCascade does (in patch 3) -- what happens if the document rules
> change and we call RefreshRuleCascade again?

So currently, we don't really handle @-moz-document rule evaluation changing properly.  If the document URL changes, we won't do anything, but if we do happen to call RefreshRuleCascade (for a new set of matched medium features, for example) then we will re-evaluate them.

mMustGatherDocumentRules = false was meant to just avoid doing some work since we don't handle this properly yet, although I suppose that there's going to be more work involved in re-evaluating the rules (which we still do when mMustGatherDocumentRules == false) than the actual building up of the mDocumentRules array.

It is worth noting that if some document rules do change their evaluation, we won't move the rule processor in the RuleProcessorCache (i.e. we won't now store it with its new document rule cache key).  So if we have a rule processor that has RefreshRuleCascades called on it and end up storing RuleCascadeDatas with now-wrong style information, this can end up being re-used in other documents that would currently be getting their own (up to date) rule processors.
Comment 34 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-14 20:22:39 PDT
Until we do correctly handle document rule re-evaluation in bug 798137, we could avoid making it worse by checking whether the document rule cache key that results from RefreshRuleCascade differs from the RuleCascadeData that's in mRuleCascades; if so, we should clear them all out from mRuleCascades before storing our new one.
Comment 35 User image David Baron :dbaron: ⌚️UTC-8 2015-06-15 19:56:55 PDT
I think mMustGatherDocumentRules causes the following problem:

Suppose somebody dynamically adds a new @-moz-document rule to a sheet.
CSSStyleSheet::InsertRuleInternal calls DidDirty() and
mDocument->StyleRuleAdded().  DidDirty calls ClearRuleCascades on the
sheet, which calls it on the rule processor.  This clears the
RuleCascadeData, but doesn't reset mMustGatherDocumentRules on the
rule processor.  (Then StyleRuleAdded works its way into
PresShell::RecordStyleSheetChange which triggers a restyle.)

If you reset mMustGatherDocumentRules to mIsShared in
nsCSSRuleProcessor::ClearRuleCascades, I guess it would then be false
when you build a second RuleCascadeData (i.e., for a different medium).

I think you probably want to do that (set mMustGatherDocumentRules to
mIsShared in nsCSSRuleProcessor::ClearRuleCascades.)  But I guess that
complicates the way it's used; you expect to call TakeDocumentRules()
and GetDocumentCacheKey once (in patch 5) and then be done with it.

So I guess what you should do here is carefully comment the known bugs
in the system.  i.e., add comments pointing out that dynamic insertion
of @-moz-document rules into a style sheet is broken with this patch
(do you think that's likely to be a problem? -- I'm a little worried),
add comments pointing out that nsCSSRuleProcessor::mDocumentRules
is only briefly valid between when it's built and when TakeDocumentRules
does a Move() from it and invalidates it.  (That's hairy in itself
unless nsTArray guarantees that a moved-from object is in a sane state
rather than just a destructable one.)

I guess if you actually wanted to fix this you'd need to evict the
RuleProcessorCache entry on ClearRuleCascades in addition to resetting
mMustGatherDocumentRules, or something like that.  Maybe that isn't so
bad, though?



Also, I wonder if it would make more sense for CascadeEnumData's
constructor to take a pointer to the rule processor (and be a friend
of it) rather than take a bunch of separate members.

Finally, I think it would be good if GatherDocRuleEnumFunc and
CascadeRuleEnumFunc had clear comments pointing to each other so
that somebody modifying one knows that they may well need to modify
the other.

Please brace the ifs in GatherDocRuleEnumFunc, and the new
ifs in CascadeRuleEnumFunc.
Comment 36 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-15 20:53:08 PDT
(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #35)
> I think mMustGatherDocumentRules causes the following problem:
> 
> Suppose somebody dynamically adds a new @-moz-document rule to a sheet.
> CSSStyleSheet::InsertRuleInternal calls DidDirty() and
> mDocument->StyleRuleAdded().  DidDirty calls ClearRuleCascades on the
> sheet, which calls it on the rule processor.  This clears the
> RuleCascadeData, but doesn't reset mMustGatherDocumentRules on the
> rule processor.  (Then StyleRuleAdded works its way into
> PresShell::RecordStyleSheetChange which triggers a restyle.)

I'm not so sure it's a problem.  In CSSStyleSheet::ClearRuleCascades, we call RuleProcessorCache::RemoveSheet, which ensures that if we try to look up a cached rule processor we won't get this one, as its list of document rules now may not match the one used as they key in the RuleProcessorCache.  mMustGatherDocumentRules controls whether we gather up the document rule array for the purpose of generating that key for the RuleProcessorCache (as obtained by TakeDocumentRules later).  But if we are no longer in the RuleProcessorCache, that doesn't matter; this rule processor won't get added to the RuleProcessorCache -- that only happens just after a new one is created by nsStyleSet.

The other part is mDocumentCacheKey.  This will remain out of date.  But again this doesn't matter, since it is also only taken right after we create a new rule processor and decide to cache it.

> add comments pointing out that nsCSSRuleProcessor::mDocumentRules
> is only briefly valid between when it's built and when TakeDocumentRules
> does a Move() from it and invalidates it.  (That's hairy in itself
> unless nsTArray guarantees that a moved-from object is in a sane state
> rather than just a destructable one.)

I will add some assertions that we never try to take mDocumentRules or get mDocumentCacheKey after ClearRuleCascades has been called.

> I guess if you actually wanted to fix this you'd need to evict the
> RuleProcessorCache entry on ClearRuleCascades in addition to resetting
> mMustGatherDocumentRules, or something like that.  Maybe that isn't so
> bad, though?

As mentioned above, I'm already evicting.

> Also, I wonder if it would make more sense for CascadeEnumData's
> constructor to take a pointer to the rule processor (and be a friend
> of it) rather than take a bunch of separate members.

Probably, yeah.  Sounds like a good followup.
Comment 37 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-16 00:53:11 PDT
[part 2]

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #29)
> That said, I don't think this is a valid use of remove_if.  C++ 2011
> says:

Good catch.  I followed your suggestion to do the tracker removal from within the predicate function.

> Should RuleProcessorCache::DoRemoveRuleProcessor assert if it falls
> through to the end (i.e., doesn't find the rule processor)?

Yes I think we can.

> Should RuleProcessorCache::SizeOfIncludingThis really be counting
> the rule processor objects themselves?  Perhaps it should... but if
> so, doesn't something else need to *stop* counting them?

Yes, nsStyleSet::SizeOfIncludingThis skips counting shared rule processors.  (Oh, that's in a later patch actually, since that later patch is where we decide which levels to cache rule processor for.)

> Is it necessary to have the aIsShared boolean parameter to the
> nsCSSRuleProcessor constructor?  It's not used in this patch.  Maybe
> it's used later?  If so, maybe a setter makes more sense -- even if used
> after the constructor -- just because it avoids boolean parameters.
> (Or is mIsShared intended to be immutable?  If so, maybe it should be
> "const bool".)

It is used later, and it's meant to be immutable.  I'll make it const.
Comment 38 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-16 01:20:59 PDT
(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #32)
> This breaks media feature change handling for UA and user sheets when
> combined with certain other dynamic changes, since MediumFeaturesChanged
> depends on whether the current rule cascade changed to determine whether
> to report that a change happened.  We have other problems with that
> mechanism (bug 1089417), though.  I suspect you can get away with that
> since UA and user sheets shouldn't be changing dynamically, but you should
> at least file a followup bug.

Can you explain how this works, and why the fix for bug 1089417 won't help (or would be different, at least)?
Comment 39 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-16 05:38:24 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a609233c8282
Comment 40 User image David Baron :dbaron: ⌚️UTC-8 2015-06-19 17:51:30 PDT
Comment on attachment 8615274 [details] [diff] [review]
Part 3: Gather document rules and produce an nsDocumentRuleResultCacheKey in nsCSSRuleProcessors.

OK, could you add a comment somewher related to mMustGatherDocumentRules
explaining the reasons that we only need to gather document rules once, 
i.e., the stuff you currently have in the commit message, plus what you
explained in your first paragraph in comment 36.  (You can probably
then remove much of that paragraph from the commit message.)

Please apply the changes at the end of comment 35 as well.

r=dbaron with that
Comment 41 User image David Baron :dbaron: ⌚️UTC-8 2015-06-19 17:59:45 PDT
(In reply to Cameron McCormack (:heycam) from comment #38)
> (In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from
> comment #32)
> > This breaks media feature change handling for UA and user sheets when
> > combined with certain other dynamic changes, since MediumFeaturesChanged
> > depends on whether the current rule cascade changed to determine whether
> > to report that a change happened.  We have other problems with that
> > mechanism (bug 1089417), though.  I suspect you can get away with that
> > since UA and user sheets shouldn't be changing dynamically, but you should
> > at least file a followup bug.
> 
> Can you explain how this works, and why the fix for bug 1089417 won't help
> (or would be different, at least)?

I guess that depends on how we fix bug 1089417 (I keep meaning to get back to that).

The basic problem is that nsCSSRuleProcessor::MediumFeaturesChanged determines whether any changes to the media features have caused a change in rules by calling RefreshRuleCascade and seeing whether a different rule cascade results.

The earlier patches in bug 1089417 extended that with mPreviousCacheKey, but that was still insufficient because in some cases (viewport size change) the media feature change is deferred, and we get a GetRuleCascade call after the media feature change has happened but before we're notified about it.

The additional problem that this introduces is that if we're sharing a rule processor across multiple documents, the concept of "most recent rule cascade" as a way to store the most recent media feature state becomes even worse.
Comment 42 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-19 20:09:15 PDT
(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #40)
> OK, could you add a comment somewher related to mMustGatherDocumentRules
> explaining the reasons that we only need to gather document rules once, 
> i.e., the stuff you currently have in the commit message, plus what you
> explained in your first paragraph in comment 36.  (You can probably
> then remove much of that paragraph from the commit message.)

I'm going to add some assertions too that we don't call TakeDocumentRules twice or call TakeDocumentRules or GetDocumentCacheKey after they would become invalid due to a ClearRuleCascades.
Comment 45 User image Nicholas Nethercote [:njn] 2015-06-25 21:50:27 PDT
5 digit bug down! Nice.

heycam, do you have any measurements on the memory usage impact? Does it help with AdBlock Plus?
Comment 46 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-25 22:28:15 PDT
(In reply to Nicholas Nethercote [:njn] from comment #45)
> 5 digit bug down! Nice.

My first one. :-)

> heycam, do you have any measurements on the memory usage impact? Does it
> help with AdBlock Plus?

It does.  For some reason I can't seem to get ABP working at the moment.  When I install it, I don't get a toolbar button anywhere, and it doesn't seem to be blocking anything...

But what I recall from testing last week is that when ABP is enabled, using its default blocklist, we save a bit over 2 MB per document.  (This is for every displayed document, including iframes, except one, since we are now sharing this data between documents.)

Without ABP, we just have the standard UA style sheets that Firefox adds to every document, and in that case we save about 120 KB per document.

We also save the time required to build up these tables, but I haven't measured that.
Comment 47 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-25 23:00:11 PDT
(In reply to Cameron McCormack (:heycam) from comment #46)
> For some reason I can't seem to get ABP working at the moment. 
> When I install it, I don't get a toolbar button anywhere, and it doesn't
> seem to be blocking anything...

Filed bug 1177643 for that.
Comment 48 User image Alhaitham Ibrahim 2015-06-25 23:33:27 PDT
(In reply to Cameron McCormack (:heycam) from comment #46)
> For some reason I can't seem to get ABP working at the moment. 
> When I install it, I don't get a toolbar button anywhere, and it doesn't
> seem to be blocking anything...

https://bugzilla.mozilla.org/show_bug.cgi?id=1176702
Comment 51 User image Kris Maglione [:kmag] 2015-06-26 10:11:13 PDT
I think I owe someone a beer. I was honestly not expecting to see this fixed in my lifetime.
Comment 52 User image timbugzilla 2015-06-26 10:15:13 PDT
Any idea of memory use reductions? Especially with ABP/uBlock.
Comment 53 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-26 10:17:48 PDT
(In reply to timbugzilla from comment #52)
> Any idea of memory use reductions? Especially with ABP/uBlock.

See comment 46.  uBlock doesn't use the same technique of adding a massive style sheet, IIRC, so it won't be affected by this bug.
Comment 54 User image Cameron McCormack (:heycam) (away 27 Jan–1 Feb) 2015-06-27 11:00:18 PDT
I measured how long it takes to build the rule cascade for the agent-level sheets (i.e. all of the built-in UA style sheets), and that's about 0.3ms on my 2012 2.6 GHz i7 rMBP.  For the user-level sheets, which is where the ABP sheet is, it's about 13ms.  So these are the times we'll save on each page load.
Comment 55 User image Carsten Book [:Tomcat] 2015-06-29 05:23:24 PDT
https://hg.mozilla.org/mozilla-central/rev/9798c03e3909
Comment 56 User image J. Ryan Stinnett [:jryans] (use ni?) 2015-06-30 06:19:26 PDT
Maybe worth a relnote for the significant memory savings?
Comment 57 User image Nicholas Nethercote [:njn] 2015-06-30 14:19:39 PDT
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #56)
> Maybe worth a relnote for the significant memory savings?

Definitely. I've written a blog post about this improvement, but I will hold off on posting it for a few days to avoid overlapping with today's new release -- I don't want to cause confusion whereby people think these improvements are part of Firefox 39, because they only landed in 41.
Comment 58 User image Nicholas Nethercote [:njn] 2015-06-30 22:10:52 PDT
Release Note Request (optional, but appreciated)

[Why is this notable]: Memory reductions for AdBlock Plus users, which is the #1 add-on. A blog post on the topic of AdBlock Plus's memory usage (https://blog.mozilla.org/nnethercote/2014/05/14/adblock-pluss-effect-on-firefoxs-memory-usage/) received a *lot* of attention last year.

[Suggested wording]: "Firefox will use less memory when running AdBlock Plus."

[Links (documentation, blog post, etc)]: https://blog.mozilla.org/nnethercote/2015/07/01/firefox-41-will-use-less-memory-when-running-adblock-plus/
Comment 59 User image Ritu Kothari (:ritu) 2015-07-01 17:42:31 PDT
This was added to Firefox 41.0a2 release notes.
Comment 60 User image michael-bugzilla-firefox 2015-09-25 09:32:10 PDT
Will this make it to the ESR 38 branch, or will I be waiting for ESR 38+7=45?
Comment 61 User image Boris Zbarsky [:bz] (still a bit busy) 2015-09-25 09:33:47 PDT
The latter.  This is not a critical security fix or critical web compat fix, so isn't suitable for uplift to ESR even if we ignore the complexity of the changes and the number of other things that would need to be backported (see the dependency tree of this bug).
Comment 62 User image michael-bugzilla-firefox 2015-09-25 09:34:56 PDT
Ok, thank you.
p.s. That was *fast*.

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