Various prerequisites for parallel CSS parsing

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
Just breaking out some of my patches that can land first.
(Assignee)

Comment 1

a year ago
MozReview-Commit-ID: 2BolVJXz8pz
Attachment #8967796 - Flags: review?(emilio)
(Assignee)

Comment 2

a year ago
The existing suppression tries to be specific about the callstack that
arrives at the function, but that breaks when we rejigger the machinery
up the callstack. In practice, the existing suppressions cover most of
the ways we would arrive at selector parsing, and so I think the
specificity here is more trouble than it's worth.

MozReview-Commit-ID: 2k52xq64SQX
Attachment #8967797 - Flags: review?(jseward)
(Assignee)

Comment 3

a year ago
This will allow the Rust code hold a copy-free strong reference to the string
past callstack unwind.

The byte offset thing is very annoying. Suggestions welcome on a better
way to handle it.

MozReview-Commit-ID: HCop9h2abZU
Attachment #8967798 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

a year ago
This is necessary because we can't create GeckoStyleSheets off-main-thread,
so we need a placeholder until it can be filled in.

MozReview-Commit-ID: ssRme4fLYg
Attachment #8967799 - Flags: review?(emilio)
Comment on attachment 8967796 [details] [diff] [review]
Part 1 - Stop asserting main thread when looking up CSS keywords. v1

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

I think PLDHashTable::Search is thread-safe per my read, but please double-check, to avoid issues like bug 1348600.
Attachment #8967796 - Flags: review?(emilio) → review+
Comment on attachment 8967798 [details] [diff] [review]
Part 3 - Pass a bonafide nsACString to Servo. v1

>+                          const nsACString& aBytes,

aUTF8Bytes?  Or just aUTF8....

I agree the offset thing is annoying.  Given that the only caller here is layout/style/StreamLoader.cpp can we just move the complication to it as follows:

1)  In StreamLoader::WriteSegmentFun if !self->mTriedBOMDetection make sure to not stick more than 3 bytes total into mBytes (so copy over min(aCount, 3-mBytes.Length() or something), then do the Encoding::ForBOM thing and save the result.  And truncate mBytes at that point, so it will only end up containing the non-BOM data.

2)  In StreamLoader::OnStopRequest use our saved encoding, if any.  Might need to do Encoding::ForBOM() again if we never did it in WriteSegmentFun (e.g. if we got < 3 bytes total).  That will necessitate a memmove to remove the BOM if found now, but since by definition we're moving 0 or 1 bytes... that's OK.

3)  When all is said and done, StreamLoader's mBytes will just be the relevant bytes, and we can pass it in.

Removing review flag pending you deciding whether you want to do it that way.

All that said, if we are passing an nsACString to the rust side, how does that guarantee that it can be held past stack unwind?  An nsACString doesn't have to be backed by a stringbuffer.  Do we have something that will share if it can, copy if not?
Flags: needinfo?(bobbyholley)
Attachment #8967798 - Flags: review?(bzbarsky)
Comment on attachment 8967799 [details] [diff] [review]
Part 4 - Allow placeholder import sheets. v1

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

::: servo/components/style/stylesheets/rules_iterator.rs
@@ +15,4 @@
>  
> +/// Holder for various RulesIterator bits that we don't want to populate for
> +/// empty iterators.
> +struct RulesIteratorInner<'a, 'b>

I don't understand why this is needed. The only caller of RulesIterator::empty does have a guard, a device, and a quirks_mode.

::: servo/components/style/stylesheets/stylesheet.rs
@@ +202,5 @@
>      fn media<'a>(&'a self, guard: &'a SharedRwLockReadGuard) -> Option<&'a MediaList>;
>  
> +    /// Return an iterator using the condition `C`.
> +    #[inline]
> +    fn iter_rules<'a, 'b, C>(

Instead of this, I think it'd be better to add a:

  fn rules(&'a self, guard: &'a SharedRwLockReadGuard) -> &'a [CssRule]

That allows you return &[] for the placeholder sheet, avoiding the special-casing of ImportRule everywhere else, including rules_iterator.

Then you can implement iter_rules in terms of that here in a single place instead.

@@ +210,5 @@
> +    ) -> RulesIterator<'a, 'b, C>
> +    where
> +        C: NestedRuleIterationCondition;
> +
> +    /* Methods below are implemented in terms of the methods above */

This comment doesn't seem ultra-helpful, but doesn't hurt either I guess.
Attachment #8967799 - Flags: review?(emilio)
(Assignee)

Comment 8

a year ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Comment on attachment 8967796 [details] [diff] [review]
> Part 1 - Stop asserting main thread when looking up CSS keywords. v1
> 
> Review of attachment 8967796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think PLDHashTable::Search is thread-safe per my read, but please
> double-check, to avoid issues like bug 1348600.

I had a look quick, and it seems good to me too. That bug was before our static analysis existed, so I think it would have caught it.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> I don't understand why this is needed. The only caller of
> RulesIterator::empty does have a guard, a device, and a quirks_mode.

Yeah, fair point. I was trying to make empty() argument-less, but looking at it now I agree the churn wasn't worth it.

In any case, it's moot given the next point.
 
> ::: servo/components/style/stylesheets/stylesheet.rs
> Instead of this, I think it'd be better to add a:
> 
>   fn rules(&'a self, guard: &'a SharedRwLockReadGuard) -> &'a [CssRule]
> 
> That allows you return &[] for the placeholder sheet, avoiding the
> special-casing of ImportRule everywhere else, including rules_iterator.
> 
> Then you can implement iter_rules in terms of that here in a single place
> instead.

This is a nice simplification, thanks.

> This comment doesn't seem ultra-helpful, but doesn't hurt either I guess.

Fixed.
(Assignee)

Updated

a year ago
Attachment #8967799 - Attachment is obsolete: true
Comment on attachment 8968276 [details] [diff] [review]
Part 4 - Allow placeholder import sheets. v2

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

r=me, with the iterator bit fixed. Thanks!

::: servo/components/style/stylesheets/mod.rs
@@ +16,5 @@
>  pub mod origin;
>  mod page_rule;
>  mod rule_list;
>  mod rule_parser;
> +pub mod rules_iterator;

This doesn't seem needed.

::: servo/components/style/stylesheets/rules_iterator.rs
@@ +100,5 @@
>                              import_rule,
>                          ) {
>                              continue;
>                          }
> +                        match import_rule.stylesheet {

What about:

 import_rule.stylesheet.rules(self.guard).iter()

?

The continue is wrong because you're not returning the @import rule itself below. If you really care about skipping empty stuff, for which errored loads and such could also benefit, just do:

if sub_iter.len() != 0 {
    self.stack.push(sub_iter);
}

below, but I don't think it's very important.
Attachment #8968276 - Flags: review?(emilio) → review+
(Assignee)

Updated

a year ago
Depends on: 1454460
(Assignee)

Comment 11

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> Removing review flag pending you deciding whether you want to do it that way.

Nice idea, will do in bug 1454460.


> 
> All that said, if we are passing an nsACString to the rust side, how does
> that guarantee that it can be held past stack unwind?  An nsACString doesn't
> have to be backed by a stringbuffer.  Do we have something that will share
> if it can, copy if not?

The whole idea here is that we can use the nsString bindings from the rust side to grab a ref to the underlying string buffer, which will be exclusive after the caller unwinds.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 12

a year ago
Comment on attachment 8967798 [details] [diff] [review]
Part 3 - Pass a bonafide nsACString to Servo. v1

I moved this to bug 1454460.
Attachment #8967798 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
No longer depends on: 1454460
(Assignee)

Comment 13

a year ago
Comment on attachment 8967797 [details] [diff] [review]
Part 2 - Widen valgrind suppression for selector parsing. v1

Moved this to bug 1454511.
Attachment #8967797 - Attachment is obsolete: true
Attachment #8967797 - Flags: review?(jseward)

Comment 14

a year ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef9eec5460a7
Stop asserting main thread when looking up CSS keywords. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6f99cbcab6
Allow placeholder import sheets. r=emilio
> The whole idea here is that we can use the nsString bindings from the
> rust side to grab a ref to the underlying string buffer

OK.  That handles the case when the underlying thing is not a stringbuffer reasonably (e.g. by copying)?
(Assignee)

Comment 16

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> > The whole idea here is that we can use the nsString bindings from the
> > rust side to grab a ref to the underlying string buffer
> 
> OK.  That handles the case when the underlying thing is not a stringbuffer
> reasonably (e.g. by copying)?

It calls nsCString::Assign, so I think yes.
https://hg.mozilla.org/mozilla-central/rev/ef9eec5460a7
https://hg.mozilla.org/mozilla-central/rev/0b6f99cbcab6
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 18

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6e99e5290a8
followup: Fix Servo build bustage. r=me
You need to log in before you can comment on or make changes to this bug.