Closed
Bug 1395064
Opened 7 years ago
Closed 7 years ago
stylo: Add uses of fallible Vec, SmallVec and HashMap facilities
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jseward, Assigned: jseward)
References
Details
Attachments
(7 files, 3 obsolete files)
3.19 KB,
patch
|
Details | Diff | Splinter Review | |
46.40 KB,
patch
|
Details | Diff | Splinter Review | |
7.13 KB,
text/plain
|
Details | |
33.43 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
text/plain
|
Details | |
8.42 KB,
text/plain
|
Details | |
4.34 KB,
text/plain
|
Details |
Bug 1389009 and bug 1393656 add fallible versions of Vec::push, SmalLVec::push and HashMap::insert. Now we need to to identify big infallible memory allocations in Stylo (potential OOM points) and change them to use the fallible versions instead. There's also the tricky question of how to test this stuff without actually running machines out of memory.
Assignee | ||
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Here are some Stylo OOM crash reports: bp-b2be6492-c375-435d-936d-e3efc0170829 (bug 1390838) bp-28222723-711a-448c-a741-177210170829 bp-1a316f28-8500-4563-9d93-67ec50170828 bp-4b62bccf-9c77-4a10-803d-6b30b0170825 bp-da99d167-e727-47f2-bd23-1f3d40170826 bp-bb734ad8-4613-4139-b410-f8e8d0170825
Blocks: stylo-release, 1390838
Priority: -- → P1
Assignee | ||
Comment 2•7 years ago
|
||
Stylesheet::parse_rules: add parsed rules fallibly to their containing vector.
Updated•7 years ago
|
Assignee: nobody → jseward
Assignee | ||
Comment 3•7 years ago
|
||
A simple valgrind tool (recycled version of dhat) for selectively causing allocations to fail. Intercepts, symbolises and filters all large allocation stacks and can cause any specific stack to fail.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Handles allocation failures in the path Servo_StyleSet_FlushStyleSheets -> style::gecko::data::PerDocumentStyleDataImpl::flush_stylesheets -> insert<style::stylist::Rule> -> <hashglobe::hash_map::HashMap<K, V, S>>::entry -> <hashglobe::hash_map::HashMap<K, V, S>>::resize -> malloc as that seems to happen frequently in bug 1390838. With the patch in place, and testing with the tool in comment 3, the system stays alive when loading the Obama test case and also https://www.w3.org/TR/html5/single-page.html, when all allocations >= 16384 bytes along the abovementioned path are forced to fail. That is in total 5128 allocation attempts. The pages don't look right but the system stays alive and responsive.
Assignee | ||
Updated•7 years ago
|
Attachment #8904611 -
Flags: feedback?(bobbyholley)
Comment 6•7 years ago
|
||
Comment on attachment 8904611 [details] [diff] [review] bug1395064-use-fallible-collections-2.diff Review of attachment 8904611 [details] [diff] [review]: ----------------------------------------------------------------- ISTR some discussion on the Vec of Rules within the stylesheet needing fallible allocation as well. You can probably test this on 100x myspace [1], since that has an abnormally-large stylesheet. [1] https://www.dropbox.com/s/h51fspacftf1lcq/myspace.tar.bz2?dl=0 ::: servo/components/style/selector_map.rs @@ +250,5 @@ > .or_insert_with(SmallVec::new) > } > Bucket::Class(class) => { > + match self.class_hash.try_entry(class.clone(), quirks_mode) { > + Err(_err) => return (), // We OOMd. Give up. Nit: You don't need the (), and can just do |return|. I'd also shorten the comment to just say // OOM ::: servo/components/style/stylist.rs @@ +336,5 @@ > debug!("Found valid keyframe animation: {:?}", animation); > + match origin_cascade_data.animations.try_insert(keyframes_rule.name.as_atom().clone(), animation) { > + Err(_err) => return (), // We OOMd. Give up. > + Ok(_rule) => () > + } Instead, just do: if origin_cascade_data.animations.try_insert(keyframes_rule.name.as_atom().clone(), animation).is_err() { return; }
Attachment #8904611 -
Flags: feedback?(bobbyholley) → feedback+
Comment 7•7 years ago
|
||
Comment on attachment 8904611 [details] [diff] [review] bug1395064-use-fallible-collections-2.diff Review of attachment 8904611 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/stylist.rs @@ +336,5 @@ > debug!("Found valid keyframe animation: {:?}", animation); > + match origin_cascade_data.animations.try_insert(keyframes_rule.name.as_atom().clone(), animation) { > + Err(_err) => return (), // We OOMd. Give up. > + Ok(_rule) => () > + } (Or, better yet, propagate the error up so we don't try to keep inserting stuff? If we've reached OOM conditions once it seems not a big deal to just bail out from the pending rules). Also, that'd allow to write this as: animations.try_insert(...)?; Which is shorter and nicer IMO.
Comment 8•7 years ago
|
||
Comment on attachment 8904611 [details] [diff] [review] bug1395064-use-fallible-collections-2.diff Review of attachment 8904611 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/selector_map.rs @@ +490,5 @@ > self.0.entry(key) > } > > + /// HashMap::try_entry > + pub fn try_entry(&mut self, mut key: Atom, quirks_mode: QuirksMode) -> Result<hash_map::Entry<Atom, V>, FailedAllocationError> { Is this function used anywhere? nit: probably should wrap this before the ->
Attachment #8904611 -
Flags: feedback+
Assignee | ||
Comment 9•7 years ago
|
||
* uses shorter forms where possible, per comment 6 * adds handling for Vec of Rules OOM, per comment 6 * adds handling for an OOM with a large SmallVec allocation * impl DocumentCascadeData, add_stylesheet(): propagates failure into rebuild, per Emilio's suggestion in comment 7, and if rebuild catches it, it just returns immediately Tested on Obama testcase, 100x myspace, https://www.w3.org/TR/html5/single-page.html.
Attachment #8904611 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8905484 -
Flags: review?(bobbyholley)
Comment 10•7 years ago
|
||
Comment on attachment 8905484 [details] [diff] [review] bug1395064-use-fallible-collections-3.diff Review of attachment 8905484 [details] [diff] [review]: ----------------------------------------------------------------- In general, I think we should bubble OOMs up more from the selector_map stuff, and handle them higher up the stack, so that we don't keep trying to allocate in low-memory situations. This will also make the APIs cleaner, since we can use the |?| operator, which means that we only need a single extra character (plus the function signature modifications) to handle allocation failure.
Attachment #8905484 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 11•7 years ago
|
||
Ok, I can try for that. I had assumed, perhaps unwisely, that the goal was merely not to crash at a few specific large allocation points. Is there a particular function or functions that would be good places to end the propagation chain(s) ? I can try to find candidate(s), but if you already have some in mind, that would be good to know.
Flags: needinfo?(bobbyholley)
Comment 12•7 years ago
|
||
For stylist flushing, let's propagate up to add_stylesheet (and have that function continue to return void). For parsing, parse_rules is probably the right place.
Flags: needinfo?(bobbyholley)
Comment 13•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12) > For stylist flushing, let's propagate up to add_stylesheet (and have that > function continue to return void). For parsing, parse_rules is probably the > right place. I think propagating it up to rebuild() is nicer. Or at the very minimum to add_stylesheet. Otherwise, you basically keep calling add_stylesheet() with the following stylesheets, trying to allocate more and more, which is pretty bad if you're already in OOM conditions. WDYT, Bobby?
Flags: needinfo?(bobbyholley)
Comment 14•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #13) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12) > > For stylist flushing, let's propagate up to add_stylesheet (and have that > > function continue to return void). For parsing, parse_rules is probably the > > right place. > > I think propagating it up to rebuild() is nicer. Or at the very minimum to > add_stylesheet. > > Otherwise, you basically keep calling add_stylesheet() with the following > stylesheets, trying to allocate more and more, which is pretty bad if you're > already in OOM conditions. WDYT, Bobby? That is fine. This bug and bug 1389009 need to move faster. We're less than 2 weeks from the merge, and since we're expecting them to fix OOM crashes, we need time for them to percolate into the Nightly population, and possibly adjust. If time zones are an issue, please work directly with emilio for reviews. Ideally this would land within the next 24hrs. If you're not around, let me know and I will just do it.
Flags: needinfo?(bobbyholley) → needinfo?(jseward)
Assignee | ||
Comment 15•7 years ago
|
||
Respin. Passes (cd servo && ./mach test-unit -p style) but otherwise untested.
Attachment #8905484 -
Attachment is obsolete: true
Flags: needinfo?(jseward)
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Attachment #8906147 -
Flags: feedback?(emilio)
Comment 16•7 years ago
|
||
Comment on attachment 8906147 [details] [diff] [review] bug1395064-use-fallible-collections-5.diff Review of attachment 8906147 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty reasonable over all. I think I got a place where you were not using fallible push, and I'd propagate up at least note_selector(..), and probably add_stylesheet(..) too. The rest of comments are mostly formatting nits. ::: servo/components/fallible/lib.rs @@ +47,4 @@ > let old_ptr = vec.as_mut_ptr(); > let old_len = vec.len(); > > + let err_str = "capacity overflow when allocating Vec"; We can make this a `static const` or something like that, I think. @@ +100,3 @@ > #[inline(never)] > #[cold] > +fn try_double_small_vec<T>(svec: &mut SmallVec<T>) nit: I'd run this through rustfmt. @@ +112,3 @@ > let old_cap: usize = svec.capacity(); > + let new_cap: usize = if old_cap == 0 { 4 } else { > + old_cap.checked_mul(2).ok_or(FailedAllocationError::new(err_str)) ? nit: spacing. ::: servo/components/style/invalidation/element/invalidation_map.rs @@ +192,5 @@ > selector: &Selector<SelectorImpl>, > quirks_mode: QuirksMode) > { > self.collect_invalidations_for(selector, quirks_mode) > + .unwrap_or_else(|_| warn!("out of memory in InvalidationMap::note_selector")) I'd propagate this up, I think. Should only be called from rebuild(), right? @@ +209,5 @@ > fn collect_invalidations_for( > &mut self, > selector: &Selector<SelectorImpl>, > quirks_mode: QuirksMode) > + -> Result<(), FailedAllocationError> Let's format this as: fn collect_invalidations_for( // ... quirks_mode: QuirksMode, ) -> Result<(), FailedAllocationError> { ::: servo/components/style/selector_map.rs @@ +273,5 @@ > } > > impl<T: SelectorMapEntry> SelectorMap<T> { > /// Inserts into the correct hash, trying id, class, and localname. > + pub fn insert(&mut self, entry: T, quirks_mode: QuirksMode) Let's format it as: pub fn insert( &mut self, entry: T, quirks_mode: QuirksMode, ) -> Result< ... > { @@ +489,5 @@ > #[inline] > fn find_push<Str: Eq + Hash, V, VL>(map: &mut PrecomputedHashMap<Str, VL>, > key: Str, > value: V) > + -> Result<(), FailedAllocationError> Ditto about formatting, though it's pre-existing. @@ +494,3 @@ > where VL: VecLike<V> + Default > { > + map.try_entry(key) ? .or_insert_with(VL::default).push(value); nit: spacing seems weird here. Also, don't you need to use try_push here? If so, why not? @@ +519,5 @@ > self.0.entry(key) > } > > + /// HashMap::try_entry > + pub fn try_entry(&mut self, mut key: Atom, quirks_mode: QuirksMode) nit: ditto about formatting. ::: servo/components/style/stylist.rs @@ +149,5 @@ > } > > + if self.add_stylesheet(device, quirks_mode, stylesheet, > + guards.ua_or_user, extra_data, > + SheetRebuildKind::Full).is_err() { I'd really propagate this one, seems not great to keep trying to add stuff to the data structures after an OOM. @@ +186,5 @@ > > for (stylesheet, rebuild_kind) in flusher { > + if self.add_stylesheet(device, quirks_mode, stylesheet, > + guards.author, extra_data, > + rebuild_kind).is_err() { Ditto for these.
Attachment #8906147 -
Flags: feedback?(emilio) → feedback+
Comment 17•7 years ago
|
||
Comment on attachment 8906147 [details] [diff] [review] bug1395064-use-fallible-collections-5.diff Review of attachment 8906147 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/fallible/lib.rs @@ +47,4 @@ > let old_ptr = vec.as_mut_ptr(); > let old_len = vec.len(); > > + let err_str = "capacity overflow when allocating Vec"; Or just inline it, given that you only use it twice. Shortening to "Vec capacity overflow" is fine. @@ +67,5 @@ > }; > > if new_ptr.is_null() { > + return Err(FailedAllocationError::new( > + "out of memory when allocating Vec")); This probably doesn't need to wrap. ::: servo/components/style/invalidation/element/invalidation_map.rs @@ +192,5 @@ > selector: &Selector<SelectorImpl>, > quirks_mode: QuirksMode) > { > self.collect_invalidations_for(selector, quirks_mode) > + .unwrap_or_else(|_| warn!("out of memory in InvalidationMap::note_selector")) I don't think detailed error messages for these warnings are worth the visual cost, given that they're very unlikely to be observed. Please just make all these warn!("OOM") ::: servo/tests/unit/style/stylist.rs @@ +169,5 @@ > fn test_insert() { > let (rules_list, _) = get_mock_rules(&[".intro.foo", "#top"]); > let mut selector_map = SelectorMap::new(); > + selector_map.insert(rules_list[1][0].clone(), QuirksMode::NoQuirks) > + .expect("test_insert #1: OOM"); Either just .unwrap() or .expect("OOM");
Attachment #8906147 -
Flags: feedback+
Assignee | ||
Comment 18•7 years ago
|
||
Addresses all review comments (comment 16, comment 17).
Attachment #8906147 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8906253 -
Flags: review?(emilio)
Assignee | ||
Comment 19•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22298b13658d8991f28b477ac2015b00206077e3 talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ea7b55d65d76214f97aaae502d65cb26fc6f5659&newProject=try&newRevision=22298b13658d8991f28b477ac2015b00206077e3&framework=1&showOnlyImportant=0 Looks like some large regressions in bloom_basic, tp5o responsiveness, tp6_facebook. Investigating.
Comment 20•7 years ago
|
||
Comment on attachment 8906253 [details] [diff] [review] bug1395064-use-fallible-collections-6.diff Review of attachment 8906253 [details] [diff] [review]: ----------------------------------------------------------------- Modulo formatting nits, this lgtm. ::: servo/components/style/invalidation/element/invalidation_map.rs @@ +191,4 @@ > pub fn note_selector( > &mut self, > selector: &Selector<SelectorImpl>, > quirks_mode: QuirksMode) nit: Paren and brace to the same line as the return type, here and everywhere else: ) -> Result<(), FailedAllocationError> { ::: servo/components/style/selector_map.rs @@ +274,5 @@ > > impl<T: SelectorMapEntry> SelectorMap<T> { > /// Inserts into the correct hash, trying id, class, and localname. > + pub fn insert(&mut self, > + entry: T, nit: Arguments four-space indented: pub fn insert( &mut self, entry: T, quirks_mode: QuirksMode, ) -> Result<(), FailedAllocationError> { @@ +519,5 @@ > self.0.entry(key) > } > > + /// HashMap::try_entry > + pub fn try_entry(&mut self, mut key: Atom, quirks_mode: QuirksMode) ditto here. ::: servo/components/style/stylist.rs @@ +150,5 @@ > } > > + self.add_stylesheet(device, quirks_mode, stylesheet, > + guards.ua_or_user, extra_data, > + SheetRebuildKind::Full)?; nit: Keep the indentation as it was, and just add the question mark, here and below.
Attachment #8906253 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 21•7 years ago
|
||
The Talos link in comment 19 shows a 30.15% perf regression on bloom_basic opt e10s linux64. I can't reproduce anything like that. On a quiet machine w/ cpu locked at 2.4GHz, and doing 30 runs, for style-perf-tests/perf-reftest/bloom-basic.html, I have the following traversal_time_ms numbers: base MIN 42.80 AVG 46.76 infallible MIN 42.88 AVG 47.01 This is with the default number of threads (6, I assume) and e10s disabled.
Assignee | ||
Comment 22•7 years ago
|
||
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #18) > Addresses all review comments (comment 16, comment 17). On testing this against forced fails, it handles {Vec,SmallVec}::try_push failures OK. But for HashMap forced fails, Gecko segfaults outside and presumably downstream from Stylo. This didn't happen with earlier versions of the patch. I assume that's because this version propagates failures more agressively than earlier ones. Bobby: (1) [see failure log in next comment] any insights into this? (2) Shall I land it anyway and deal with this in a followup? Or try to fix it before landing?
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 24•7 years ago
|
||
Comment 25•7 years ago
|
||
Per IRC discussion, the crash is not something to worry about, since XUL is known to crash if anonymous box style are wrong, and that's what happens when we force OOM when rebuilding the UA stylist. The main concern is performance of stylesheet parsing and stylist building. Once those are measured to be OK (per discussion) we should be good to land.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 26•7 years ago
|
||
Further measurements, per IRC discussion. Same methodology as in comment 21. All times are milliseconds. Summary: ./mach gtest Stylo.Servo_StyleSheet_FromUTF8Bytes_Bench 10 runs: BASE min 358 avg 362.3 PATCHED min 358 (+0.00%) avg 364.0 (+0.47%) tp6 facebook, time for impl DocumentCascadeData :: rebuild, biggest number only 20 runs: BASE min 5.32 avg 5.43 PATCHED min 5.49 (+3.20%) avg 5.61 (+3.31%) 100x myspace, time for impl DocumentCascadeData :: rebuild, biggest number only 20 runs: BASE min 108.8 avg 109.9 PATCHED min 111.3 (+2.30%) avg 112.3 (+2.18%)
Assignee | ||
Comment 27•7 years ago
|
||
Assignee | ||
Comment 28•7 years ago
|
||
In short a 2% to 3% perf loss in stylist rebuild. Possible causes (surmised, not measured): * increased number of conditional branches due to null checks * increased icache cost * poorer optimization of affected functions due to increased number of control flow paths to exit nodes * needing to return a Result<(), FallibleAllocationError> in the affected functions * possibly non-optimal initial vec size of 4 following resize from zero * missing inline directive(s) in some crucial place? Possible mitigations * change return type to Result<(), ()> if that reduces the number of returned machine words from 2 to 1 * tune initial vec size of 4 following resize from zero * more inlining Proceeding to land regardless, since we need this functionality anyway, measurements/mitigations will delay further, and proposed mitigations are low risk.
Assignee | ||
Comment 29•7 years ago
|
||
https://github.com/servo/servo/pull/18434
Comment 30•7 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #28) > Proceeding to land regardless, since we need this functionality anyway, > measurements/mitigations will delay further, and proposed mitigations are > low risk. Yeah, makes sense. I filed bug 1398593 to track the mitigations.
Comment 31•7 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/903b743e407e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
status-firefox57:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•