1529272 - Reduce the cost of hashing/moving/cloning FontInstance

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P4)

Description

[Nicolas Silva [:nical]]

FontInstance is a fairly large struct used as a hash key in a couple of places (one of these hash maps get sampled for each text run), and is clones/moved around a fair bit. These moves turn into a memmove that contribute some portion of the __memmove_avx_unaligned_erms symbol which is high in the profiles I've collected for bug 1476368 (unfortunately perf won't give me stacks for these). It also holds a vector of font variations which causes memory allocations when we clone the instance.

I'm in the process of splitting the struct into a base instance held in an Arc<FontInstance>, and an "extended" instance which holds an arc to the base instance, plus the members that can change (transform, render_mode, color, size).

hashing the extended instance only requires hashing its own members and the FontInstanceKey of the base. Hashing the base instance is not needed and we can always use the FontInstanceKey.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Bug 1529272 - Reduce the amount of allocations, hashing and moves related to FontInstance. r=gw,lsalzman

Comment 2

[Nicolas Silva [:nical]]

Work in progress patch. The names aren't great but the general idea is there.

Comment 3

[Nicolas Silva [:nical]]

The patch is in a good state now. Lee suggested on irc that we might be able to get away with only storing the FontInstanceKey in the FontInstance instead of the Arc<BaseFontInstance>. This would let us store the base instance in the map without arcs (saving a bunch of allocations) and remove a guaranteed cache miss from hashing the FontInstance, although we'd have to make sure the map is always available when we manipulate font instances and it would cost a hash map lookup in all of these places

I think that it's worth trying out but since it's a pretty big change (A lot of code was spared in this patch thanks to the Deref impl) I'd like to first go through with the Arc.

Another possibility is to have both the key and the arc in the font instance (trading the cache miss when hashing with a bigger (by 64 bits) FontInstance and the other structs that depend on it.

Another (more objectionable) thing we could do is to use the address of the base instance instead of its key when hashing. It spares the cache miss but probably means we can't serialize the hash maps to disk anymore without re-implementing the serialization since the hashes wouldn't be stable between runs.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/361c7b99ae8f
Reduce the amount of allocations, hashing and moves related to FontInstance. r=lsalzman

Comment 5

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/361c7b99ae8f