stylo: vector_longhand makes transient vecs for computed values

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bholley, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
This is causing a sizeable amount of malloc traffic during the cascade in the profile in bug 1360878 comment 1. These are the allocations that occur during the cascade functions that aren't inside do_make_mut (which is the style-struct copy-on-write). Notice that the cascade functions this occurs in are the ones that are vector_longhands.

The current setup makes sense for servo, because those vectors will go on to live in the style structs. But for stylo, they're just transient representations, since the persistent storage happens in the Gecko-side style structs which use a different representation.

It seems like rather than calling collect at [1], we should find some way to just pass the iterator directly to the gecko style struct setters. Might require a bit of fussing with the codegen to get the types right, but there's no reason this shouldn't be straightforward to do.

Simon or Manish, do either of you have a moment to take care of this?

[1] http://searchfox.org/mozilla-central/rev/38fdbbcaa4be3a3f5b0d207dc5d53fb687c42d6a/servo/components/style/properties/helpers.mako.rs#218
(Reporter)

Updated

a year ago
Flags: needinfo?(simon.sapin)
Flags: needinfo?(manishearth)
(Assignee)

Comment 1

a year ago
It's a bit tricky since it introduces a lifetime on a computed value, which probably screws a few things up (a good thing, in a way, because those are use after frees otherwise). Still, worth trying.


(At Rustfest over the weekend, and have backlog of other stylo to do next week, but could prioritize poking at this).

The hard part will be animations, where we need to construct a vec again. I guess a Cow-like enum (lazy, owned) would work.

I have often disliked the fact that we build Servo computed values and then immediately consume them though the majority of these cases are really fine.
(Reporter)

Comment 2

a year ago
(In reply to Manish Goregaokar [:manishearth] from comment #1)
> It's a bit tricky since it introduces a lifetime on a computed value, which
> probably screws a few things up (a good thing, in a way, because those are
> use after frees otherwise). Still, worth trying.

Yeah. And it's all codegen, right? ;-)
> 
> 
> (At Rustfest over the weekend, and have backlog of other stylo to do next
> week, but could prioritize poking at this).

Yeah I'd like to get this fixed this week. Simon might also be a good candidate, lets see what his schedule is like.

> 
> The hard part will be animations, where we need to construct a vec again. I
> guess a Cow-like enum (lazy, owned) would work.

Seems like we should be able to uncompute straight to the vec in the specified value without round tripping through a computed value vec.

> I have often disliked the fact that we build Servo computed values and then
> immediately consume them though the majority of these cases are really fine.

Yeah I agree.
Note that using smallvec may be a reasonable tradeoff here that we should do on servo anyway.

Using more than one transition/background/etc. is uncommon enough.
(Assignee)

Comment 4

a year ago
SmallVec should work too, yes. This is essentially how the Gecko style struct works anyway IIRC.
Flags: needinfo?(manishearth)
(Reporter)

Comment 5

a year ago
(In reply to Manish Goregaokar [:manishearth] from comment #4)
> SmallVec should work too, yes. This is essentially how the Gecko style
> struct works anyway IIRC.

SmallVec can still leave us with transient allocations though, right? I think we still want to be able to pass the iterator directly.
Flags: needinfo?(manishearth)
(Reporter)

Comment 6

a year ago
(Note that this is less urgent now after the smallvec thing, and could potentially wait if you have other higher-priority work on your plate. But it's worth at least thinking about what we'd do here and how much work it would be).
(Assignee)

Comment 7

a year ago
Yes, but the iterator model would probably require a bunch of special casing since animation code and such will assume that computed values are ownable.
(Reporter)

Comment 8

a year ago
Sure, I'm not suggesting changing the canonical ComputedValue, just changing vec_longhands to generate a separate ComputedValueFactory type (or somesuch), and then making the setters operate on that for those cases.
(Reporter)

Comment 9

a year ago
Basically, we would factor out the generation of the computed value into two stages, the first of which generates the iterator and the second of which does the .collect(). We'd then pass the first-stage thing to the setters, which shouldn't require us to change animation code at all.
(Assignee)

Comment 10

a year ago
Yeah, I was considering this as an alt fix (have a partial patch but need to wait on clobber build), didn't realize that was what you were suggesting in the first place :)
Flags: needinfo?(manishearth)
(Reporter)

Comment 11

a year ago
Ok great!
Assignee: nobody → manishearth
Flags: needinfo?(simon.sapin)
(Assignee)

Comment 13

a year ago
https://hg.mozilla.org/integration/autoland/rev/aaaf7747a289
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.