1307592 - stylo: Find how to efficiently implement the slow selector flags.

Status: RESOLVED DUPLICATE of bug 1336646

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

Right now Servo and Gecko share an optimization (the HAS_SLOW_SELECTOR/HAS_SLOW_SELECTOR_LATER_SIBLINGS/HAS_EDGE_CHILD_SELECTOR flags), that they use to optimize away a few children restyles.

These flags have to be set atomically in the Servo case, and also in the Stylo case, since they're set while matching selectors on the children of the same element.

A way to implement this correctly in Stylo would be calling through ffi into Gecko and use the atomic intrinsics to atomically OR the flags. This imposes an unnecessary function call for every set.

Other way to do it would be in rust, but we can't right now because:

 * AtomicU32 is unstable.
 * intrinsics::atomic_fetch_or and atomic_or_fetch are unstable.

Implementing this with something like AtomicU32 + relaxed operations would be nice, since that would allow us to set all the node flags with the same mechanism, and not bother about two different paths, one for the dirty and other restyle flags, and other for the selector flags, and also would allow us to stop worring about whether to pass immutable references instead of raw pointers.

These flags are not *extremely* hot (at least they shouldn't in the presence of the bloom filter), but they're really annoying, so we can try to find a decent solution for them.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
>  * AtomicU32 is unstable.
>  * intrinsics::atomic_fetch_or and atomic_or_fetch are unstable.

Is there any hope of stabilizing either of these in the not-too-distant future? Alternatively, is there any kind of hackery that could let us use a single unstable feature on stable rust?

Comment 2

[Manish Goregaokar [:manishearth]]

Trying to start the discussion for stabilization in https://github.com/rust-lang/rust/issues/32976#issuecomment-251497456

How many bits do we actually need? Is the issue that usize is too small on 32 bit, or too large on 64? Note that the issue of there not being 64 bit atomics on 32 bit is fine; using two relaxed atomics will work here since the flags seem to have a monotonic consistency model.


fetch_or looks stable? https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.html#method.fetch_or

Comment 3

[Emilio Cobos Álvarez [:emilio]]

(In reply to Manish Goregaokar [:manishearth] from comment #2)
> Trying to start the discussion for stabilization in
> https://github.com/rust-lang/rust/issues/32976#issuecomment-251497456

Thanks, that's great.

> How many bits do we actually need? Is the issue that usize is too small on
> 32 bit, or too large on 64? Note that the issue of there not being 64 bit
> atomics on 32 bit is fine; using two relaxed atomics will work here since
> the flags seem to have a monotonic consistency model.

The issue is that, plus the fact that a) we shouldn't grow nsINode, and b) the layout between Rust and C++ should stay consistent.

> fetch_or looks stable?
> https://doc.rust-lang.org/nightly/std/sync/atomic/struct.AtomicUsize.
> html#method.fetch_or

That's the fetch_or method on AtomicUsize, not the intrinsics::atomic_or (https://doc.rust-lang.org/nightly/std/intrinsics/fn.atomic_or.html), for example.

The thing is we can't use a word-sized atomic because we need exactly 32 bits.

Comment 4

[Manish Goregaokar [:manishearth]]

> The thing is we can't use a word-sized atomic because we need exactly 32 bits.

Oh, I thought your two points of things that need stabilizing were ANDed, not OR :)

Yeah, not growing nsinode because there's no 32 bit atomic on 64 bit is an issue which can't be solved without stabilization. The *opposite* situation (no 64 bit atomic on 32 bit) can be solved.

I'll try and push for stabilization then.

Comment 5

[Bobby Holley (:bholley)]

Hacky suggestion - what about carefully ensuring that the 32-bit node flags are 64-bit aligned, fetching them as usize, and in the 64-bit (little-endian) case treating the high bytes as opaque?

Though TBH, that's nasty enough that we should only do that if the OOL calls create a measurable performance problem.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Hacky suggestion - what about carefully ensuring that the 32-bit node flags
> are 64-bit aligned, fetching them as usize, and in the 64-bit
> (little-endian) case treating the high bytes as opaque?
> 
> Though TBH, that's nasty enough that we should only do that if the OOL calls
> create a measurable performance problem.

Yeah, that's really nasty, over all taking into account that the two fields we're talking about are in to different parts of the inheritance chain, so doing that hack either in bindgen or in the regen script is going to be non-trivial. Also, the fields would be to be cfg'd, since an usize is not the same size in a 32-bit build, if we care about those.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Duping this in favor of bug 1336646