Closed
Bug 1398593
Opened 7 years ago
Closed 7 years ago
stylo: mitigate performance impact of fallible allocation on stylist rebuilds
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bholley, Assigned: jseward)
References
Details
Attachments
(1 file)
2.36 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Comment 1•7 years ago
|
||
Mitigations I would try, in order: (1) Make any cross-crate non-generic calls as inline, because otherwise they can't be. Emilio did this in one place already in his fixup PR in #18434. (2) Stop propagating errors from multiplication overflow, and panic instead. That stuff probably doesn't need to be fallible. (3) Map to a smaller type (Result<(),()>). (4) Squelch OOM further down the stack.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(jseward)
Assignee | ||
Comment 2•7 years ago
|
||
Profiling a startup/quit of 100x-myspace shows the following calls hashglobe::hash_map::...::try_entry 609853 calls at 129 insns/call smallvec<T>::...::try_push 610826 calls at 98 insns/call The attached patch changes them to #[inline(always)]. For good measure it also inlines vec<T>::try_push, although that didn't really come on the radar to the same extent. Re-profiling verified that all 3 fns did get inlined. Time in ms, for the longest impl DocumentCascadeData :: rebuild call, a la bug 1395064 comment 26, are before: min 111.83 after: min 105.55 (-5.6%) That is, at least for this rebuild call, it recovers more than the perf lost in bug 1395064's fallible-ization. Raw numbers below. This seems surprisingly good, so I tried it a couple of times with and without, and the speedup reproduces reliably. ------------ Before 113.31082310061902 rebuild time ms 117.55347601138055 rebuild time ms 112.29764891322702 rebuild time ms 112.52675508148968 rebuild time ms 112.84803203307092 rebuild time ms 112.37904999870807 rebuild time ms 112.57833894342184 rebuild time ms 113.41941601131111 rebuild time ms 115.7894879579544 rebuild time ms 112.68248804844916 rebuild time ms 113.34050795994699 rebuild time ms 112.30860895011574 rebuild time ms 112.15484503190964 rebuild time ms 111.83528997935355 rebuild time ms 112.57043096702546 rebuild time ms 112.250539008528 rebuild time ms 112.62355395592749 rebuild time ms 112.69196099601686 rebuild time ms 112.34057100955397 rebuild time ms 113.109449041076 rebuild time ms 112.25187010131776 rebuild time ms 112.12728195823729 rebuild time ms After 105.97411904018372 rebuild time ms 107.74134506937116 rebuild time ms 106.54112510383129 rebuild time ms 105.92529294081032 rebuild time ms 106.18745302781463 rebuild time ms 105.55109695997089 rebuild time ms 106.5326709067449 rebuild time ms 106.16152104921639 rebuild time ms 106.02356703020632 rebuild time ms 108.37697505485266 rebuild time ms 105.8483689557761 rebuild time ms 106.25109204556793 rebuild time ms 106.40255699399859 rebuild time ms 105.83429306279868 rebuild time ms 110.44508998747915 rebuild time ms 105.82982003688812 rebuild time ms 106.62948398385197 rebuild time ms 106.41615802887827 rebuild time ms 107.97405592165887 rebuild time ms 109.44087104871869 rebuild time ms 105.84615299012512 rebuild time ms 106.39835090842098 rebuild time ms
Flags: needinfo?(jseward)
Assignee | ||
Updated•7 years ago
|
Attachment #8906656 -
Flags: review?(emilio)
Updated•7 years ago
|
Attachment #8906656 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 3•7 years ago
|
||
I opened https://github.com/rust-lang/rust/issues/44517 to request that such inline directives are applied to the std:: variants of same, when fallible functionality is added to std::Vec and std::HashMap.
Assignee | ||
Comment 4•7 years ago
|
||
https://github.com/servo/servo/pull/18460
Assignee | ||
Updated•7 years ago
|
Status: NEW → 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
•