Closed Bug 1344668 Opened 9 years ago Closed 8 years ago

stylo: Record restyle hints for devtools

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: hiro, Unassigned)

References

Details

For devtools performance tool we have to record all restyle hints. For animation's restyle, I've already opened a bug (bug 1344662).
Gah, I was wrong we can't record in the pre-traverse for animation hints. A solution what I can think of is to create a SequentialTask if we are recording the timeline markers and pass the start time and the end time to the SequentialTask in somewhere (in resolve_style_internal?) and record all of the timeline in the SequentialTask. The start is the time before doing match_element and the end time is the time after cascade_element?
It's possible that the new style system model might be less compatible with the way we currently show hints in devtools. In particular, the way we accumulate restyle hints on elements both before and during traversal makes it hard to say exactly what restyle hint is responsible for what work. And then there's the whole question of whether or not we need to cascade, or to call CalcStyleDifference. In general, the new model is very granular. It might make sense to just drop the restyle hint from the devtools UI. Cameron, what do you think?
Flags: needinfo?(cam)
Just a quick note: As for animations we use the recorded entries for tests to check we surely throttle animation restyles for some kind of animations (e.g. compositor animations). Those tests will be very useful when we enable compositor animations on stylo. But for those tests we don't use time values of the records at all, so at least for those tests we can record animation's entries in the pre-traversal step (bug 1344619). So it's fine to drop the hints from the devetools UI to me.
The existing restyle hint reporting to the profiler can already be misleading, as far as I can tell, since it only reports the hint found at each restyle root, and not any subsequent restyle hints found while traversing the subtree from that root. So if you had an eRestyle_Self on some element, as a restyle root, and an eRestyle_Subtree on its child, then we would only end up reporting the eRestyle_Self on the parent. The existing profiler reporting also doesn't take into account the need to cascade or to call CalcStyleDifference, either. (The existing Gecko model isn't too different from the new model wrt granularity, IMO.) We could find the equivalent of restyle roots during traversal (i.e. once we've traversed down and found an element with a restyle hint), but since it's not really simple to measure the time of a single subtree's restyling, given the work-stealing setup we have, I'd be OK with just dropping it. (We can still have profiler data for entire ProcessPendingRestyle calls.) But I'd check with devtools folks that we're not dropping a feature that people actually use, without a replacement.
Flags: needinfo?(cam)
I think it's pretty useful for web developers to know "yeah, this change you made forced us to restyle the world". To the extent that we can surface that, it would be very nice to do so...
I'll inquire with the devtools team to see what the pain would be if this feature was not implemented in Stylo.
And if they're not ok with dropping it, we should ask them to brainstorm what information they really want. The current mechanism of showing the restyle hint that happened to be posted for the restyle root (without actually saying what the root was) seems not-very-useful. It certainly doesn't give the information bz wants in comment 6.
Just to be clear, which part of the functionality would actually be dropped? Would it be just the hint? We'd still have the marker, timing information, and JS callstack, correct? I brought it up with the team today, and we're reviewing. We don't really have any concrete telemetry on whether people use this type of data as it's just a row in a side panel. As stylo is so different, I'd be up for discussing what data is useful and actionable from what you all can gather. We're moving our client work to perf.html and don't have a solid markers view yet. https://github.com/devtools-html/perf.html
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #9) > Just to be clear, which part of the functionality would actually be dropped? > Would it be just the hint? We'd still have the marker, timing information, > and JS callstack, correct? That's right. We would also probably have more accurate separation than we do now, because the style traversal happens atomically before any layout changes, rather than the interleaving that we can have in Gecko today. > I brought it up with the team today, and we're > reviewing. We don't really have any concrete telemetry on whether people use > this type of data as it's just a row in a side panel. Ok. Given that we don't provide much public documentation about what these hints mean, I would be pretty surprised if anyone was making use of them. What _might_ be useful is the ability to indicate whether we're restyling to tick animations or something else. It's possible that the callstack alone might be enough there, but Hiro would know better. > > As stylo is so different, I'd be up for discussing what data is useful and > actionable from what you all can gather. We're moving our client work to > perf.html and don't have a solid markers view yet. > https://github.com/devtools-html/perf.html There's probably a fair amount of interesting performance statistic we could surface there (number of threads, thread utilization, cache statistics, etc). But it probably doesn't block shipping stylo.
I'd like to give a day or two to let other people on devtools to weigh in if they care, but my intuition is that it would be fine to drop for now, but we should regroup to talk about relevant performance metrics with stylo. perf.html is primarily focusing on our internal engineers so I would love to take stylo's performance metrics, and provide useful tools to analyze what's going on under the hood to make all of your job easier. The "Recalculate Style" marker and "Restyle Hint" is documented here: https://developer.mozilla.org/en-US/docs/Tools/Performance/Waterfall
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #11) > I'd like to give a day or two to let other people on devtools to weigh in if > they care, but my intuition is that it would be fine to drop for now, but we > should regroup to talk about relevant performance metrics with stylo. > perf.html is primarily focusing on our internal engineers so I would love to > take stylo's performance metrics, and provide useful tools to analyze what's > going on under the hood to make all of your job easier. Agreed. Should probably regroup about this sometime in the summer. > > The "Recalculate Style" marker and "Restyle Hint" is documented here: > https://developer.mozilla.org/en-US/docs/Tools/Performance/Waterfall Sure, but there's no documentation about what those strings actually mean.
Sounds good to me! I'll reach back out in Q3.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6) > I think it's pretty useful for web developers to know "yeah, this change you > made forced us to restyle the world". To the extent that we can surface > that, it would be very nice to do so... I agree. However, I don't think the current UI really solves this. If we dropped the hint recording, you could still find restyles that just take a long time. You wouldn't really know *why* the restyle took such a long time. But you'd still be able to get the call stack of the first change that required a restyle. It would also be nice to have an advanced mode that has more overhead during recording but gives you something that's a lot closer to the full causal graph of causes and costs. But I expect that having a restyling system that tracks causes throughout the system would look a lot different from a system that has the goal of being fast.
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #13) > Sounds good to me! I'll reach back out in Q3. Sounds good to me too. Lets plan to meet in SF this summer to review.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10) > What _might_ be useful is the ability to indicate whether we're restyling to > tick animations or something else. It's possible that the callstack alone > might be enough there, but Hiro would know better. The call stack is not enough there, unfortunately. On a regular tick of the refresh driver we can either end up doing an animation restyle followed by a regular non-animation restyle, or just an animation restyle. While we haven't introduced animation restyles to Stylo yet, we plan to do so as part of implementing transitions. (Normally we distinguish between the two cases either by setting a flag based on the restyle hint when posting restyles, e.g. see GeckoRestyleManager::PostRestyleEvent, or by looking at the restyle hints we're currently processing, e.g. see nsStyleSet::ResolveStyleWithReplacement.)
Priority: -- → P5
Hiro brought to my attention that there is an animations test[1] that uses the restyle hints within the markers to check whether various things triggered animation restyles or not. Should Stylo continue recording hints in markers for this test, or is there a different way the test should go about checking this info? [1]: http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/dom/animation/test/chrome/test_restyles.html#50
Flags: needinfo?(bobbyholley)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17) > Should Stylo continue recording hints in markers for this test Really I mean: should we _start_ recording hints in markers for Stylo because of this test? (The comments so far in this bug suggest dropping the hints within the markers.)
I still think we need to drop the markers. There's nothing sane we can really give there. AFAICT, the code in comment 17 basically just wants to know if we were doing an animation/transition traversal. That's pretty orthogonal the actual restyle hint machinery, and is something that we could just note explicitly on the timeline.
Flags: needinfo?(bobbyholley)
I believe we're not planning to do anything else here right now. We don't have a reasonable hint to record in the marker for Stylo. If we can think of a good way to expose details here in the future, let's do it, but I'll close this for now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.