Closed
Bug 1454030
Opened 6 years ago
Closed 6 years ago
Various prerequisites for parallel CSS parsing
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files, 3 obsolete files)
941 bytes,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
21.76 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
Just breaking out some of my patches that can land first.
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: 2BolVJXz8pz
Attachment #8967796 -
Flags: review?(emilio)
Assignee | ||
Comment 2•6 years 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•6 years 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•6 years 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 5•6 years ago
|
||
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 6•6 years ago
|
||
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 7•6 years ago
|
||
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•6 years 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 | ||
Comment 9•6 years ago
|
||
Attachment #8968276 -
Flags: review?(emilio)
Assignee | ||
Updated•6 years ago
|
Attachment #8967799 -
Attachment is obsolete: true
Comment 10•6 years ago
|
||
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 | ||
Comment 11•6 years 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•6 years 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 | ||
Comment 13•6 years 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•6 years 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
Comment 15•6 years ago
|
||
> 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•6 years 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.
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ef9eec5460a7 https://hg.mozilla.org/mozilla-central/rev/0b6f99cbcab6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 18•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c6e99e5290a8 followup: Fix Servo build bustage. r=me
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6e99e5290a8
You need to log in
before you can comment on or make changes to this bug.
Description
•