1933557 - new Map([[fiber, expirationTime]]) is 7x slower than Chrome, affects scheduleWork on TodoMVC-React-Redux

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Markus Stange [:mstange]]

After the work in bug 1851662, creating Map and Set objects has mostly disappeared from the sp3 comparison reports.

The one remaining instance I found was scheduleWork on TodoMVC-React-Redux, which does new Map([[fiber, expirationTime]]):

function scheduleWork(fiber, expirationTime) {
  if (50 < nestedUpdateCount) throw nestedUpdateCount = 0, rootWithNestedUpdates = null, Error(formatProdErrorMessage(185));
  fiber = markUpdateTimeFromFiberToRoot(fiber, expirationTime);
  if (null !== fiber) {
    var priorityLevel = getCurrentPriorityLevel();
    1073741823 === expirationTime ? (executionContext & LegacyUnbatchedContext) !== NoContext && (executionContext & (RenderContext | CommitContext)) === NoContext ? performSyncWorkOnRoot(fiber) : (ensureRootIsScheduled(fiber), executionContext === NoContext && flushSyncCallbackQueue()) : ensureRootIsScheduled(fiber);
    (executionContext & 4) === NoContext || 98 !== priorityLevel && 99 !== priorityLevel || (null === rootsWithPendingDiscreteUpdates ? rootsWithPendingDiscreteUpdates = new Map([[fiber, expirationTime]]) : (priorityLevel = rootsWithPendingDiscreteUpdates.get(fiber), (void 0 === priorityLevel || priorityLevel > expirationTime) && rootsWithPendingDiscreteUpdates.set(fiber, expirationTime)));
  }
}

Profiles:
Firefox: https://share.firefox.dev/3B0jqjO
Chrome: https://share.firefox.dev/4eJAbhb (7x faster)

Fixing this difference would improve Firefox's score on TodoMVC-React-Redux by 1.2% and Firefox's overall sp3 score by 0.1%.

Comment 1

[Jan de Mooij [:jandem]]

Calling Map/Set with an argument is exactly the case I didn't fully optimize yet :) Some thoughts:

1. The Set constructor has a fast path for packed arrays where it avoids calling from C++ into self-hosted code. We should probably do something similar for Map. At least if the length is small (like in this case) then checking IsPackedArray for each element is definitely much faster than calling into self-hosted code.

2. Ideally we'd also do the Map/Set object allocation in JIT code in this case. Then we could do a VM call to either initialize the object in C++, or return a value that means "call the self-hosted MapConstructorInit or SetConstructorInit function directly from JIT code".

I'll try to get to this the coming weeks. It's probably pretty common and 1.2% on React-Redux is worth it.

Comment 2

[Jan de Mooij [:jandem]]

I'll post patches soon that implement item 1 and parts of of item 2 from comment 1. I don't think it will make us 7x faster but it's a good start.

Fun fact: I added some logging and the News-Nuxt subtest has a lot of new Set(undefined) calls. With my patches this is almost as fast as a plain new Set(). Vue has some of this too but less than Nuxt.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1933557 part 1 - Refactor IsOptimizableInitForSet a bit. r?iain!

This makes it easier to reuse this function for Map and WeakMap objects.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1933557 part 2 - Add optimization for packed arrays to Map constructor. r?iain!

This is very similar to the existing optimization for Set and WeakSet, but we
have to ensure the array elements are also packed arrays with at least 2 elements.

This avoids calling the self-hosted MapConstructorInit function. In most cases
on Speedometer 3 the iterable argument is an array with a few elements so it's
much faster to handle this in C++.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1933557 part 3 - Add AddWeakSetEntryImpl to deduplicate some WeakSet code. r?iain!

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1933557 part 4 - Add optimization for packed arrays to WeakMap constructor. r?iain!

For consistency with WeakSet and Map/Set.

Existing WeakMap jit-tests cover the obvious edge cases.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1933557 part 5 - Optimize Map/Set constructor calls with an iterable argument in the JITs. r?iain!

It's pretty common to call the constructor with a null or undefined argument and
with this patch we can stay in JIT code in this case.

If the argument is not null or undefined we can still allocate the object in
JIT code but we then call into C++ to do the initialization.

This could be optimized more later by emitting CacheIR guards to replace the
IsOptimizableInitForMapOrSet call.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbb14aa1182d
part 1 - Refactor IsOptimizableInitForSet a bit. r=iain
https://hg.mozilla.org/integration/autoland/rev/f4ded77414ee
part 2 - Add optimization for packed arrays to Map constructor. r=iain
https://hg.mozilla.org/integration/autoland/rev/5e1d9bc0078f
part 3 - Add AddWeakSetEntryImpl to deduplicate some WeakSet code. r=iain
https://hg.mozilla.org/integration/autoland/rev/04ea59eeadde
part 4 - Add optimization for packed arrays to WeakMap constructor. r=iain
https://hg.mozilla.org/integration/autoland/rev/dfe8a8d407c4
part 5 - Optimize Map/Set constructor calls with an iterable argument in the JITs. r=iain
https://hg.mozilla.org/integration/autoland/rev/13919e3e2290
apply code formatting via Lando

Comment 9

[amarc]

https://hg.mozilla.org/mozilla-central/rev/bbb14aa1182d
https://hg.mozilla.org/mozilla-central/rev/f4ded77414ee
https://hg.mozilla.org/mozilla-central/rev/5e1d9bc0078f
https://hg.mozilla.org/mozilla-central/rev/04ea59eeadde
https://hg.mozilla.org/mozilla-central/rev/dfe8a8d407c4
https://hg.mozilla.org/mozilla-central/rev/13919e3e2290