1462448 - Nursery allocate const-string-split array copy and avoid double heap-slot initialization in CharSplitHelper

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[André Bargull [:anba]]

- The array result for the Baseline ConstStringSplit optimization is always tenured allocated, which probably isn't necessary.
- In CharSplitHelper we can avoid to initialize the dense-elements twice when use a separate branch for Latin-1 strings.

Comment 1

[André Bargull [:anba]]

Attachment: bug1462448.patch

js/src/builtin/String.cpp:

If we split Latin-1 strings from Two-byte strings, we can use |NativeObject::setDenseInitializedLength(uint32_t)| for the Latin-1 case, which saves us the additional HeapSlot::init(...) calls in |NativeObject::ensureDenseInitializedLength(...)|. 

This changes improves this µ-benchmark from 165ms to 125ms for me:
---
function f() {
    var str = "abcdefghijklmnopqrstuvwyxz";
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 500000; ++i) {
        q += str.split("").length;
    }
    return [dateNow() - t, q];
}
print(f());
---



js/src/jit/BaselineIC.cpp:

GetTemplateObjectForNative:
- The template object created in GetTemplateObjectForNative(...) seems to be unused, because it's never accessed from BaselineInspector::getTemplateObjectForNative(...).

CopyArray/CopyStringSplitArray:
- Renamed |CopyArray| to |CopyStringSplitArray| to make it more clear when this function is used.
- Added some additional assertions and changed it to use |NativeObject::getDenseInitializedLength()| instead of |ArrayObject::length()|, because even if both functions return the same value here, getDenseInitializedLength() is more correct to use when accessing dense elements. 
- Removed |TenuredObject| to ensure the array copies can be nursery allocated.

TryAttachConstStringSplit:
- Replaced the JSOP_NEW check with a check for |constructing| in DoCallFallback(...), because it's confusing to explicitly check for JSOP_NEW here, but ignore |JSOP_SUPERCALL|. (Actually we know that StringSplitString is only called with JSOP_CALL, but I didn't want to restrict it too much.)
- Removed the callee assertions, because |callee| is never inspected after the |IsOptimizableConstStringSplit| call, so it doesn't seem necessary to me to assert anything about its type.
- Performed similar |ArrayObject::length()| to |NativeObject::getDenseInitializedLength()| changes.
- Optimized the dense element initialization and made it more similar to how the string-split arrays are initialized in js/src/builtin/String.cpp.


This improves this µ-benchmark from 500/800ms (alternating between both times) to 280ms for me (with --no-ion):
---
function f() {
    var str = "abcdefghijklmnopqrstuvwyxz";
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 500000; ++i) {
        q += str.split("").length;
    }
    return [dateNow() - t, q];
}
print(f());
---

Comment 2

[André Bargull [:anba]]

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

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8976699 [details] [diff] [review]
bug1462448.patch

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

Good finds! We really shouldn't (unconditionally) allocate non-template objects as tenured.

Comment 4

[André Bargull [:anba]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/455665169fa7
Use nursery allocation for baseline string-split and avoid extra heap-slot init for Latin1 strings. r=jandem

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/455665169fa7