Open Bug 1552587 Opened 6 years ago Updated 3 years ago

Split nsStyleDisplay

Categories

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

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

Tracking

()

People

(Reporter: boris, Unassigned)

References

Details

Recently, the size of nsStyleDisplay is almost 512 bytes (because we start to use cbindgen to generate a lot of style types to replace some UniquePtr<...>s and RefPtr<...>s), so it's pretty hard to add new data members in nsStyleDisplay.

Perhaps we should move all the animations/transitions/transform properties outside nsStyleDisplay? Not sure if there are any issues after adding a new style struct.

No longer blocks: 1429299

I'm not sure about transform properties in particular, but we should be able to move a lot of stuff away from nsStyleDisplay.

The scrolling-related properties may be a better fit. But not sure, can try to think a bit about it.

Summary: Move transform-like properties into an independent style struct → Split nsStyleDisplay

The animation, transition and related stuff seems like it could be a separate struct.

Bug 1429299 will repack nsStyleDisplay, use cbindgen for offset-path, and add offset-distance, and the total size of nsStyleDisplay becomes 504 bytes. (i.e. only can add 8 more bytes in nsStyleDisplay.)

Besides, as Hiro mentions: mScrollSnapPoints{X,Y} (Bug 1218264), mScrollSnapDestination and mScrollSnapCoordinate (Bug 1218265) will be dropped once we ship the new scroll snap in 68. The size of these data members is 64 (=16 + 16 + 24 + 8) bytes. So in theory, we still have 440 (=504 - 64) bytes later.

I wanted to add a new style struct very long time ago (when I was implementing CSS Ruby I think), and the reason against that (by dbaron I think) was that it would bloat every nsStyleContext by one pointer, which can be huge. That may or may not still apply, but it might be worth measuring.

If the size of the struct is the problem we want to solve here, an easier approach might be spliting the stuff into a separate struct (but not a top level style struct), and referenced from nsStyleDisplay, just like what was done in bug 1388255 for grid template.

I mean, if we care about that, we should instead start removing silly structs like nsStyleColor. I guess we can teach the bindings code to use sub-structs easily, somehow, though that's more work.

I'd need to check which kind of jemalloc bucket does ComputedStyle end up in, but I suspect adding a couple pointers to it now that they're not allocated off the pres shell arena is not going to be a huge deal.

What do people think of:

  • Moving transforms and animations / transitions to its own structs (maybe the same struct). Looks like we have quite a bunch of transform properties these days with css-motion stuff, individual transforms... And they should rarely be set.
  • Moving nsStyleColor somewhere else and remove that struct.

Moving animations / transitions to a separate struct seems fine to me. Sharing with transforms also seems reasonable (since they two are used together a bit).

nsStyleColor could be moved to nsStyleBackground since I guess it's quite common to set those together.
We should probably rename it in that case though, since StyleBackground()->mColor would be quite confusing!

Emilio's plan sounds good to me. Can we place it directly on the ServoComputedData object? (I know that writing-mode is cached there, although I don't know if that's duplicated with storage in the style struct as well, and if so, whether we can eliminate that duplication?)

nsStyleColor could be moved to nsStyleBackground since I guess it's quite common to set those together.

We can't do that though, since color is inherited but background-* are not. Maybe nsStyleText? StyleText()->mColor seems to read nice, and it's where mWebkitTextFillCollor is already.

Can we place it directly on the ServoComputedData object? (I know that writing-mode is cached there, although I don't know if that's duplicated with storage in the style struct as well, and if so, whether we can eliminate that duplication?)

Maybe, yeah... writing-mode is a bit different since it's derived from other properties.

color nowadays is not directly used by anything. It's effectively just a builtin CSS variable (via currentcolor) resolved at used-value time. So it probably makes more sense conceptually to move it into upper level object if possible, especially given its small (which may be changed at some point if we start supporting wide-gamut color, though).

color nowadays is not directly used by anything. It's effectively just a builtin CSS variable (via currentcolor) resolved at used-value time.

It is used in quite a few places: https://searchfox.org/mozilla-central/search?q=nsStyleColor%3A%3AmColor&case=false&regexp=false&path=, though I guess you mean in the style system itself?

So it probably makes more sense conceptually to move it into upper level object if possible, especially given its small (which may be changed at some point if we start supporting wide-gamut color, though).

I agree with that, though it's more work (we'd need to special-case that property, the style system code generation assumes right now that all properties are on style structs). In terms of effort, it's way less work to move it to nsStyleText. But after I removed the old scroll-snap implementation I guess this is not blocking anything really, so we could spend a bit more time on it.

Well, I'm not sure I agree with the specialness of color in that sense, since as you said right now it just cascades like any other property and we use it at used-value time like any other property. But color and display are probably among the most accessed fields in style struct, so if we can avoid the extra pointer chase that's great. But that probably requires some measuring, so I'm tempted to just move it somewhere else for now and do some proper measuring afterwards if needed.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

color nowadays is not directly used by anything. It's effectively just a builtin CSS variable (via currentcolor) resolved at used-value time.

It is used in quite a few places: https://searchfox.org/mozilla-central/search?q=nsStyleColor%3A%3AmColor&case=false&regexp=false&path=, though I guess you mean in the style system itself?

Most of them should probably be using -webkit-text-fill-color instead for consistency, at least the ones for text color...

See Also: → 1554716
See Also: → 1695374
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.