Skip to content
Commit 480bc367 authored by Steinar H. Gunderson's avatar Steinar H. Gunderson Committed by Chromium LUCI CQ
Browse files

Speed up SVGElement::PropertyFromAttribute().

Every SVGElement has a HeapHashMap mapping its (DOM) attributes from
the QualifiedName to the SVG property (SVGAnimatedPropertyBase),
so that mutating an attribute will also update the SVG property.
This causes every single attribute change to look up into a hash table
(that is _not_ shared between instances), causing large amounts of
cache misses.

Replace these hash tables by straight-up if/else code; there are rarely
more than ~10 attributes for an element, and each comparison is fairly
cheap. This removes the entire hash table, and with it ~100–200 bytes
of Oilpan-allocated memory for each and every SVG element.

The downside is that we get more code; it is quite verbose but mostly
obvious (the main trickiness is in the places where multiple
implementation inheritance is in use), and adds about 8 kB to the APK.
We do not expect a lot of increased instruction cache pressure,
since that would require pages to use all 40+ SVG elements roughly
equally.

An alternative would be to have a shared hash table for each type
instead of for each instance; however, the use of multiple
implementation inheritance makes this tricky, as we cannot reliably
cast the Member<Foo> variables to Member<SVGAnimatedPropertyBase>
(the base class pointer would need to be adjusted, which we cannot
do in standard C++, i.e., without doing ugly pointer arithmetic
ourselves).

This helps the MotionMark Suits benchmark roughly 1.5% on a modern
Intel CPU (i7-12700K, i.e., Alder Lake); Pinpoint seems to be even
happier, and also gives a gain on Leaves. M1 hardly sees any gain
at all, since PropertyFromAttribute() only takes ~0.1% CPU time
there according to Instruments. (This is most likely due to the
M1's large and fast L2 cache.) Of course, the memory gains are
largely independent of platform.

MotionMark on Pinpoint (linux-perf, 95% CI):

  motionmark_ramp_canvas_arcs               [ -1.1%,  +0.7%]
  motionmark_ramp_canvas_lines              [ -6.0%,  -1.3%]
  motionmark_ramp_design                    [ -2.8%,  +0.2%]
  motionmark_ramp_images                    [ -0.7%,  +1.7%]
  motionmark_ramp_leaves                    [ +0.5%,  +2.1%]
  motionmark_ramp_multiply                  [ -0.7%,  +0.4%]
  motionmark_ramp_paths                     [ -2.0%,  +4.0%]
  motionmark_ramp_suits                     [ +3.6%,  +4.5%]

The loss on “Canvas Lines” seems to be some kind of Pinpoint mirage;
I've tried unsuccessfully to reproduce it on two Linux machines with
very different hardware, and the test does not use any SVG elements.

These tests are without PGO. In theory, this should also be more
amenable to PGO than the hash table path.

Low-Coverage-Reason: Existing low coverage of these paths is now exposed
Change-Id: I9ce4c3f83090194382c56e57983e6caa8b6e62b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4518103


Reviewed-by: default avatarFredrik Söderquist <fs@opera.com>
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149128}
parent b50bde05
Loading
Loading
Loading
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment