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)

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: jseward)

References

Details

Attachments

(1 file)

Priority: -- → P3
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.
Flags: needinfo?(jseward)
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)
Attachment #8906656 - Flags: review?(emilio)
Attachment #8906656 - Flags: review?(emilio) → review+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.