TypedArray constructor inlining doesn't always seem to work as expected
Categories
(Core :: JavaScript Engine: JIT, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(3 files, 4 obsolete files)
Updated•8 years ago
|
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
Comment 7•8 years ago
|
||
Comment 8•8 years ago
|
||
Comment 9•8 years ago
|
||
Comment 10•8 years ago
|
||
Comment 11•8 years ago
|
||
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
Assignee | ||
Comment 17•7 years ago
|
||
I don't think the failing assertion [1] mentioned in comment #16 makes any sense, because the same situation can also apply in the non-OOL path [2]:
|Nursery::allocateBuffer| is not required to return a buffer which resides itself in the nursery, cf. [3]. That means the |!nursery.isInside(buf)| part of the assertion can be true in [2]. Furthermore |!tarray->hasInlineElements()| is always true in [2], because the newly created TypedArray doesn't have its elements point to its inline storage. So both pre-conditions of the assertion can be true in [2], but its consequent |tarray->isTenured()| is still false, because the new TypedArray is allocated from the nursery space.
Taking all that into account, it looks like the assertion is bogus and we don't need to care about it. And to fix this bug, we can simply call Nursery::allocate(Zeroed)Buffer (bug 1518101) in |makeTypedArrayWithTemplate|, similar to the code in |AllocateObjectBufferWithInit|.
Drive-by change:
- Directly return in |TypedArrayObject::objectMoved| when |buf| is nullptr to avoid unnecessary calls to |nursery.isInside| and |nursery.removeMallocedBuffer|.
[1] https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/js/src/vm/TypedArrayObject.cpp#514-515
[2] https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/js/src/jit/MacroAssembler.cpp#1194
[3] https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/js/src/gc/Nursery.cpp#429
Interesting side-effect of properly nursery allocating TypedArray objects in the OOL path: Some µ-benchmarks are now considerably slower!
function f() {
var r = 0;
var t = dateNow();
for (var i = 0; i < 40000; ++i) {
var ta = new Int32Array(2650 + (i & 0x1));
r += ta.length;
}
return [dateNow()-t, r];
}
var t = dateNow()
for (var i = 0; i < 5; ++i) print(f());
print("total:", (dateNow() - t));
Prints ~3200ms for the "total" count before this change, but with the patch the "total" count is ~4200ms (where each iteration takes between 400ms and 1400ms).
This happens because of more lock contention when allocating/freeing memory: The TypedArray object allocation requests more than |MaxNurseryBufferSize| bytes [4], so the memory is malloc allocated and managed by the nursery in |Nursery::mallocedBuffers|. And nursery malloc memory is freed in a background thread [5]. That means for the µ-benchmark, the main thread is constantly acquiring and releasing the jemalloc memory lock when it is requesting new memory, while the background is constantly acquiring and releasing the same lock when freeing the memory.
When commenting out [5] to force main thread freeing of the malloced memory, the complete µ-benchmark completes in less than 300ms (with this patch applied).
[4] https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/js/src/gc/Nursery.cpp#429-430
[5] https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/js/src/gc/Nursery.cpp#1017-1020
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #18)
We don't need to worry about overflow because nbytes must be <= INT32_MAX? We might change these limits at some point so to be extra explicit maybe add something like:
MOZ_ASSERT(nbytes <= INT32_MAX, "JS_ROUNDUP must not overflow");
Okay if I simply copy the check from [1] here, too?
Comment 20•7 years ago
|
||
(In reply to André Bargull [:anba] from comment #19)
Okay if I simply copy the check from [1] here, too?
Yes that's great, thanks!
Assignee | ||
Comment 21•7 years ago
|
||
Update patch to include assertion per review comments, carrying r+.
Assignee | ||
Comment 22•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d37d41cf36cd21d8c19b1a891d778eb647ba5f8b
Comment 23•7 years ago
|
||
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0140cfbc71db
Don't enforce tenure allocation for TypedArrays from inlined constructor ool-path. r=jandem
Comment 24•7 years ago
|
||
bugherder |
Updated•6 years ago
|
Description
•