1461791 - Intrinsic lookup always performs a linear search

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Test case:
---
function createIntrinsicValues() {
    function* emptyGen(){}

    // Create more entries in the intrinsic holder.
    new Map(emptyGen())
    new Set(emptyGen())
    new WeakMap(emptyGen())
    new WeakSet(emptyGen())
    new Intl.Collator();
    new Intl.NumberFormat();
    new Intl.DateTimeFormat();
    new Intl.PluralRules();
}

function bench() {
    var a = [""];
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        a.toLocaleString();
    }
    return dateNow() - t;
}

bench(); // warm-up

print(`Time: ${bench()} ms`);
createIntrinsicValues();
print(`Time: ${bench()} ms`);
---


Expected: Both function calls need approx. the same time.
Actual: The second call is ~30% slower.

Source: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/js/src/vm/GlobalObject.h#695

Comment 1

[André Bargull [:anba]]

Attachment: bug1461791-part1-lookup-table.patch

If we use NativeObject::lookup() instead of NativeObject::lookupPure() in maybeGetIntrinsicValue(), we can trigger shape table creation for the intrinsic holder which avoids the linear lookup issue. 

maybeGetIntrinsicValue() only uses NativeObject::lookupPure(), because at some point the function was directly called from the JIT and we wanted to avoid passing a JSContext (bug 934526). This is no longer the case, so it seem to be okay to trigger shape table creation when looking up values from the intrinsic holder.

Comment 2

[André Bargull [:anba]]

Attachment: bug1461791-part2-call-selfhosted.patch

It's preferable to use CallSelfHostedFunction() to call (internal) self-hosted functions compared to using GlobalObject::getSelfHostedFunction(), because it avoids running a bit of extra code [1]. 

Additional changes:
- Removes the now unused |js::SelfHostedFunction(JSContext*, HandlePropertyName)| function.
- Changes the CallSelfHostedFunction(...) which takes a char* to debug-mode only, because non-debug code should avoid the extra Atomizing needed for this function.
- Changes |js::ErrorObject::getStack_impl(...)| to return the empty string instead of silently failing when the self-hosted functions returns a non-string value (this code is/was only used for SpiderNode when emulating V8 stack traces...).

[1] https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/js/src/vm/GlobalObject.cpp#914-940

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8976093 [details] [diff] [review]
bug1461791-part1-lookup-table.patch

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

Great find, thanks.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8976098 [details] [diff] [review]
bug1461791-part2-call-selfhosted.patch

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

Shorter and faster \o/

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7591e28391189c3b1ba61999c58368ddcc09d5be

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc1cef3a8fd
Part 1: Create shape table for intrinsic lookups. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/684a004e8181
Part 2: Use CallSelfHostedFunction in more places to call self-hosted functions. r=jandem

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6cc1cef3a8fd
https://hg.mozilla.org/mozilla-central/rev/684a004e8181