1301860 - Consider making change hints per-property instead of per-struct?

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

While investigating a bit about the performance issues of the demo in bug 1301859, one of the patches I found it could be worth is making change hints per-shorthand instead of per-struct.

This way, doing an animation-only restyle doesn't need to incur in CalcStyleDifference (since we can know the properties we're animating).

This would cut down the time spent in CalcDifference in the demo in bug 1301859 by a lot, which is where most of the time is spent (if the profiler isn't lying to me), by making transition and animation change hints basically free to compute.

I thought that the CC'd people here could have more opinions, so before that somewhat large patch I prefer to ask :-)

Is there any inconvenient in doing it? I can't see any off-hand, apart from the amount of changes required in `CalcDifference` methods to avoid having two sources of truth that could get out of sync.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't understand what you're suggesting.  What do you mean by making change hints per-property?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

R(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #1)
> I don't understand what you're suggesting.  What do you mean by making
> change hints per-property?

Right now the change hint is computed per struct, in <struct_name>::CalcDifference(). I'm proposing something like a table from shorthand that changes to change hint, i.e., something like:

  display -> nsChangeHint_ReconstructFrame
  color -> nsChangeHint_RepaintFrame

And make CalcDifference only look at that table when the members difer. That way, for transitions and animations we get the change hint for free looking at that table.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It would need to be more than data, since many properties have value-specific logic for what the change hint is.  (And there are even more special cases, like the one from bug 1251075).

I'm somewhat surprised CalcDifference is taking any substantial part of the time for transitions.  Generally CalcDifference should be much cheaper than the code to actually *handle* the change hint, when there are change hints.  Is the problem that we're calling CalcDifference for lots of style contexts where there aren't actually any changes (perhaps, say, descendants of the element with the transition)?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #3)
> It would need to be more than data, since many properties have
> value-specific logic for what the change hint is.  (And there are even more
> special cases, like the one from bug 1251075).
> 
> I'm somewhat surprised CalcDifference is taking any substantial part of the
> time for transitions.  Generally CalcDifference should be much cheaper than
> the code to actually *handle* the change hint, when there are change hints. 
> Is the problem that we're calling CalcDifference for lots of style contexts
> where there aren't actually any changes (perhaps, say, descendants of the
> element with the transition)?

Sorry, s/CalcDifference/CalcStyleDifference, that includes walking the rule tree computing data.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #3) 
> Is the problem that we're calling CalcDifference for lots of style contexts
> where there aren't actually any changes (perhaps, say, descendants of the
> element with the transition)?

Oh, regarding this question, in that page all the transitioned divs are created via javascript, and completely empty, so that's not the case (at least there).

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Mainly, I think most of the time under ComputeStyleChangeFor is spent in lazily computing style structs by walking up the rule tree to compare with others that we know that haven't change (because we're transitioning background-color), but were used by layout.

I wonder if that computation would actually need to happen unconditionally (if that's the case, this wouldn't help that much), but given in that specific case we only need to repaint, I doubt it, and I think we can do better than what we're currently doing.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So we currently depend on the invariant that if a struct has been computed in an old context, it will always be computed in the new context -- so that CalcStyleDifference can then only compare the structs that we've actually used.

If we wanted to stop computing the new structs every time, we'd need to either (a) copy a record of which structs were gotten for future CalcStyleDifference or (b) remove that optimization from CalcStyleDifference.

Additionally, animating a property (e.g., font-size) can cause changes to other properties (e.g., those with 'em' units), so the new change handling mechanism would need to replicate a good bit of the old to handle various cases where properties depend on each other.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Would it be too crazy to reuse the style context for animation restyles, and only evict from the cache and recompute the structs that we know that have changed?

Seems like a much less breaking change.