Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 7 years ago

So, comparing the code between nsCSSRuleProcessor::RefreshRuleCascade [1] and media_features_change_changed_style [2], it seems to me that there is an important difference between Gecko and Servo that in Servo we always traverse the stylesheet to identify whether there is any media rule changes, while Gecko just checks all media expressions used.

This difference means that comparing to Gecko, we are wasting time on iterating the stylesheets, which can be rather big.

Looking at the profile in bug 1420423 comment 56, the self time of media_features_change_changed_style is 18ms in total for Stylo, while there is no self time for nsCSSRuleProcessor::RefreshRuleCascade at all.

Further more, Gecko doesn't use pointers to each item for key in its cache. Instead, it caches the result of each simple media expression it evaluated. This means that the evaluating logic would become much simpler (only a single level iteration). Based on this, it also make the cache per-media-type based, so it never needs to check media type again.

We probably need to consider how we would optimize it carefully, especially if we are going to also use invalidation for @media change in bug 1435940.


[1] https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/layout/style/nsCSSRuleProcessor.cpp#3453
[2] https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/servo/components/style/stylist.rs#1127

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 1 • 7 years ago

My initial idea for optimizing this would be to cache media_queries::Expression in EffectiveMediaQueryResults somehow. Maybe we can have two hash tables, one for expressions which match, one for which don't. But Expression is currently not hashable given it includes float and length value.

Another idea is to cache each feature with a list of expressions checking it (maybe just range and value) and their previous matched result. This has an extra benefit that we would only call each getter function at most once each time media values change. That should save some function call overhead, given they are always indirect calls.

These two approaches may also benefit bug 1435940 for collecting selectors to invalidate, but maybe evaluating a expression from the cache isn't that trivial.

	Jet Villegas (inactive)
	Comment 2 • 7 years ago

[ Triage 2017/02/20: P3 ]