1305062 - Improve ES6 Map performance for fresh object keys

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: containertest.js

Performance can be improved for the case where we fill Map with entries where the key is a nursery allocated object.

Using a modified version of the benchmark from bug 1162665 (attached) we see many thousands of minor GCs.  This is because we quickly fill the generic store buffer and trigger minor GC.  This happens before we create enough object to consider pre-tenuring this source so the situation never resolves (the Map itself ends up tenured so the keys will end up tenured anyway).

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1305062-map-perf

The patch stores a vector of nursery keys on the Map/Set object and only puts a single generic store buffer entry to update the table.

Benchmark results without patch:

Time: 9596ms
Minor GCs: 94268
Major GCs: 68

Benchmark results with patch:

Time: 7862ms
Minor GCs: 50
Major GCs: 64

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8794302 [details] [diff] [review]
bug1305062-map-perf

Review of attachment 8794302 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I have been sitting on this for so long.

I guess the thing that bothers me is that I think of both this and bug 1293127 as being (specialized) parts of the store buffer. But now the store buffer is getting scattered into different parts of the codebase. Memory too, although that's probably better for cache locality anyway. (Specifically, these store buffer entries add a slot to Map. Which, btw, is wasted space for tenured Maps, but I can't bring myself to care about that.)

Actually, the main logic of using this store buffer *should* be here, since it's specific to the implementation of Map/Set. It's just that when looking at the store buffer code, you'll see no hint of these specialized store buffers. Maybe it would be enough to point via comments? As in, in the store buffer code, mention these additional store buffer components, and in the specific code, refer to the name "store buffer" somewhere somehow that a grep would pick up.

The implementation is good, although it feels a little duplicative given that these things are backed by an OrderedHashMap. An OHM already has a simple vector of entries in insertion order. So the portion of the OHM Data vector that could possibly have nursery keys is all contiguous, and runs from the spot of the previous nursery collection up through the end. It may have tenured keys mixed in, and it may have holes if there have been a bunch of deletions, but I wouldn't expect that to be all that common for the portion we're interested in. But on the bright side, you wouldn't need a separate allocation at all.

Sadly, you'd still need the extra slot -- you don't need a pointer to the vector anymore, but you have to store the Data index to start scanning. I guess you'd need to store an OHM::Range in the slot, so that it gets auto-updated if the table is resized/rehashed. And you'd need to add an extendToEnd() or something to Range. Which means you're *still* using a slot, and you're still doing a dynamic allocation (for the Range), but it's of a small fixed-size chunk of data instead of a whole Vector of keys.

Anyway, that gets complicated and none of that needs to block this review. r=me for what you have already, though I'd like the doc pointers.

::: js/src/builtin/MapObject.cpp
@@ +409,5 @@
> +FreeNurseryKeys(TableObject* t)
> +{
> +    auto keys = GetNurseryKeys(t);
> +    MOZ_ASSERT(keys);
> +    js_free(keys);

js_delete

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb98c845e96
Improve ES6 Map performance for fresh object keys r=sfink

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aeb98c845e96