1380198 - stylo: reuse the bloom filter and style sharing cache across traversals

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

In bug 1371496, we've identified a bunch of extra work involved in managing the large thread-local allocations used by the traversal:
* The allocations themselves.
* Initializing the allocations.
* Memmoving them a few times because of rustc stupidity.
* Pulling the allocations back over to the main thread to poison them.

The simplest solution is to reuse the allocations. I'm investigating this now.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07a67ab78d8c58c8c3401270cd3f18a04f987bf5

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add owning_ref back as a style dep. v1

Note that this introduces two vendored copies of owning_ref, because
parking_lot needs to bump its owning_ref dep. That should be fine
though, because it's not a lot of code.

MozReview-Commit-ID: Ihxzb839TT5

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Store the bloom filter in TLS and reuse it across traversals. v1

MozReview-Commit-ID: EbzDehr2Y2L

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Reuse style sharing cache across traversals. v1

MozReview-Commit-ID: 9wCJtciqs6K

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Stop using SendElement in StyleSharingCandiate. v1

MozReview-Commit-ID: AuNPDsq8bgk

Comment 6

[Patrick Walton (:pcwalton)]

I agree that reusing the allocations is the ideal solution.

Per previous discussions, I think rustc should improve as well. I've started (well, resurrected) a tracking bug on the Rust side: https://github.com/rust-lang/rust/issues/13707#issuecomment-314619805

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8885537 [details] [diff] [review]
Part 1 - Add owning_ref back as a style dep. v1

Review of attachment 8885537 [details] [diff] [review]:
-----------------------------------------------------------------

Could you file an issue to update it, maybe an easy issue on servo or something like that?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8885538 [details] [diff] [review]
Part 2 - Store the bloom filter in TLS and reuse it across traversals. v1

Review of attachment 8885538 [details] [diff] [review]:
-----------------------------------------------------------------

Makes me super-sad, but r=me, I guess.

::: servo/components/style/bloom.rs
@@ +55,5 @@
> +    /// A handle to the bloom filter from the thread upon which this StyleBloom
> +    /// was created. We use AtomicRefCell so that this is all |Send|, which allows
> +    /// StyleBloom to live in ThreadLocalStyleContext, which is dropped from the
> +    /// parent thread.
> +    filter: OwningHandle<Arc<AtomicRefCell<BloomFilter>>, AtomicRefMut<'static, BloomFilter>>,

This makes me pretty sad :(

Please leave a TODO or something to get rid of it once the rust issues are fixed, presumably with a link to them too.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8885539 [details] [diff] [review]
Part 3 - Reuse style sharing cache across traversals. v1

Review of attachment 8885539 [details] [diff] [review]:
-----------------------------------------------------------------

What clears the cache after a restyle? I don't see it.

::: servo/components/style/sharing/mod.rs
@@ +223,5 @@
>      element: SendElement<E>,
>      validation_data: ValidationData,
>  }
>  
> +struct FakeCandidate {

Perhaps this should be closer to the definition of the TypelessCache stuff? Maybe not...

Please leave a comment in StyleSharingCandidate about updating this if it changes.

Also, perhaps assert that size_of and align_of are the same.

@@ +472,5 @@
> +///
> +/// Given that the cache stores entries of type TElement, we transmute to usize
> +/// before storing in TLS. This is safe as long as we make sure to empty the cache
> +/// before we let it go.
> +type SharingCacheBase<Candidate> = LRUCache<[Candidate; SHARING_CACHE_BACKING_STORE_SIZE]>;

This is gross... Please add comments on why we're doing this, and I really hope we can get rid of this eventually... It makes the types not really reusable... Which is not a big deal, but I think it's not nice.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8885540 [details] [diff] [review]
Part 4 - Stop using SendElement in StyleSharingCandiate. v1

Review of attachment 8885540 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/sharing/mod.rs
@@ +218,5 @@
>  /// safe to access.
>  #[derive(Debug)]
>  pub struct StyleSharingCandidate<E: TElement> {
> +    /// The element.
> +    element: E,

Note why we don't need to use SendElement?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8885539 [details] [diff] [review]
Part 3 - Reuse style sharing cache across traversals. v1

Review of attachment 8885539 [details] [diff] [review]:
-----------------------------------------------------------------

Err, nevermind, we still drop the StyleSharingCache, since we're not storing _that_ in tls.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Comment on attachment 8885537 [details] [diff] [review]
> Part 1 - Add owning_ref back as a style dep. v1
> 
> Review of attachment 8885537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you file an issue to update it, maybe an easy issue on servo or
> something like that?

https://github.com/servo/servo/issues/17698

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Please leave a TODO or something to get rid of it once the rust issues are
> fixed, presumably with a link to them too.

Per IRC discussion, I don't think we're ever going to be able to get rid of this one, because we can't cheaply stack-allocate it (since the bloom filter needs to be zeroed). Reusing really does make sense here.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Please leave a comment in StyleSharingCandidate about updating this if it
> changes.

Done.

> 
> Also, perhaps assert that size_of and align_of are the same.

I did, but dynamically in ::new, because it depends on E.
> 
> @@ +472,5 @@
> > +///
> > +/// Given that the cache stores entries of type TElement, we transmute to usize
> > +/// before storing in TLS. This is safe as long as we make sure to empty the cache
> > +/// before we let it go.
> > +type SharingCacheBase<Candidate> = LRUCache<[Candidate; SHARING_CACHE_BACKING_STORE_SIZE]>;
> 
> This is gross... Please add comments on why we're doing this, and I really
> hope we can get rid of this eventually... It makes the types not really
> reusable... Which is not a big deal, but I think it's not nice.

I added some comments.

Comment 14

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/17701

Comment 15

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/autoland/rev/84de8f2d744f

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Sorry Bobby, had to back this out since it was failing a bunch of animation reftests that create a TLC while processing animation tasks (with other TLC on the stack, obviously), with double borrows.

The logic for that is going to need to change, probably moving the execution of the tasks after the destruction of the context, or shuffling fields and making the tasks vector a type itself that implements Drop and executes the tasks itself...

Anyway, sorry again. Backout is https://github.com/servo/servo/pull/17710

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(I can look into relanding it with the fix if you're very busy, btw, just let me know)

Comment 18

[Bobby Holley (:bholley)]

Hm, why didn't this show up in the try push in comment 1? Is it related to the refactoring in bug 1379505? If so, and if you're familiar with the new code that trips us up here and have a clear idea of how to fix it, then by all means please hack up the patch. Otherwise I can try to get back to this tomorrow.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #18)
> Hm, why didn't this show up in the try push in comment 1? Is it related to
> the refactoring in bug 1379505? If so, and if you're familiar with the new
> code that trips us up here and have a clear idea of how to fix it, then by
> all means please hack up the patch. Otherwise I can try to get back to this
> tomorrow.

Yeah, it is related to that refactoring (the crashtest added there is one of the ones that trigger that codepath I think).

Anyway, yeah, I think I have an idea to fix it, so I can probably take this.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped.

Review commit: https://reviewboard.mozilla.org/r/157064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157064/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886315 [details]
Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157064/diff/1-2/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8886315 [details]
Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157064/diff/2-3/

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8886315 [details]
Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped.

https://reviewboard.mozilla.org/r/157064/#review162268

Thanks for fixing this.

::: servo/components/style/context.rs:540
(Diff revision 3)
> +impl<E> Drop for SequentialTaskList<E>
> +where
> +    E: TElement,
> +{
> +    fn drop(&mut self) {
> +        debug_assert!(thread_state::get() == thread_state::LAYOUT);

I forget, does this condition pass if we're on a worker thread? If so we should make it tighter.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #23)
> Comment on attachment 8886315 [details]
> Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped.
> 
> https://reviewboard.mozilla.org/r/157064/#review162268
> 
> Thanks for fixing this.
> 
> ::: servo/components/style/context.rs:540
> (Diff revision 3)
> > +impl<E> Drop for SequentialTaskList<E>
> > +where
> > +    E: TElement,
> > +{
> > +    fn drop(&mut self) {
> > +        debug_assert!(thread_state::get() == thread_state::LAYOUT);
> 
> I forget, does this condition pass if we're on a worker thread? If so we
> should make it tighter.

It doesn't.

This relanded at https://github.com/servo/servo/pull/17728.