Closed
Bug 1398751
Opened 6 years ago
Closed 6 years ago
Add a fast path for typed arrays to js::GetElements
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
()
Details
Attachments
(2 files, 2 obsolete files)
3.12 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
This will lead to almost exactly the same speed-ups as in V8 (https://bugs.chromium.org/p/v8/issues/detail?id=2435), i.e. 2-3x faster on the µ-benchmark in https://bugs.chromium.org/p/v8/issues/detail?id=2435#c6.
Assignee | ||
Comment 1•6 years ago
|
||
This improves the benchmark in https://bugs.chromium.org/p/v8/issues/detail?id=2435#c6 (with |n| changed to 1e4 instead of 1e5, because my PC is slower and I'm testing in a VM :-) ) as follows: stringFromCharCode: 985ms -> 305ms stringFromCodePoint: 1210ms -> 515ms Before the V8 patch, V8 and SM were both slower than JSC/Chakra, now that V8 is patched, we're the slowest engine for that µ-benchmark. But with this patch, we're faster than JSC/Chakra and I guess we get comparable numbers to V8 again, but since I haven't yet recompiled V8, I can't tell for sure.
Attachment #8906606 -
Flags: review?(evilpies)
Assignee | ||
Comment 2•6 years ago
|
||
And this gives a further improvement for the stringFromCodePoint case: With patch 1: 1210ms -> 515ms With patch 1+2: 1210ms -> 335ms
Attachment #8906607 -
Flags: review?(evilpies)
Comment 3•6 years ago
|
||
Comment on attachment 8906606 [details] [diff] [review] bug1398751-part1-getelements-for-typedarrays.patch Review of attachment 8906606 [details] [diff] [review]: ----------------------------------------------------------------- Nice, very clean. ::: js/src/jsarray.cpp @@ +443,5 @@ > + MOZ_ASSERT(length == typedArray->length()); > + MOZ_ASSERT_IF(length > 0, !typedArray->hasDetachedBuffer()); > + > + for (uint32_t i = 0; i < length; ++i, ++vp) { > + *vp = typedArray->getElement(i); I hope compilers are clever enough to do loop unswitching here and we don't check the type in every iteration.
Updated•6 years ago
|
Attachment #8906606 -
Flags: review?(evilpies) → review+
Comment 4•6 years ago
|
||
Comment on attachment 8906607 [details] [diff] [review] bug1398751-part2-codepoint-from-int32.patch Review of attachment 8906607 [details] [diff] [review]: ----------------------------------------------------------------- Wow this is a big difference. ::: js/src/jsstr.cpp @@ +3315,5 @@ > + > + // Fast path for the common case - the input is already an int32. > + if (code.isInt32()) { > + int32_t nextCP = code.toInt32(); > + if (!(nextCP < 0 || nextCP > int32_t(unicode::NonBMPMax))) { I think our usual style is to invert the conditions.
Attachment #8906607 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #3) > I hope compilers are clever enough to do loop unswitching here and we don't > check the type in every iteration. Good remark! I just tested this and at least GCC 5.4 doesn't seem to hoist the type check out of the loop. Manually switching the loop and the switch-statement further improved the µ-benchmark to stringFromCharCode: 215ms stringFromCodePoint: 240ms
Assignee | ||
Comment 6•6 years ago
|
||
New patch for part 1, because compilers don't seem to perform loop unswitching. Reduces the time needed to complete the benchmark by another 90ms when compared to the previous patch.
Attachment #8906606 -
Attachment is obsolete: true
Attachment #8906747 -
Flags: review?(evilpies)
Comment 7•6 years ago
|
||
Comment on attachment 8906747 [details] [diff] [review] bug1398751-part1-new-getelements-for-typedarrays.patch Review of attachment 8906747 [details] [diff] [review]: ----------------------------------------------------------------- Great
Attachment #8906747 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 8•6 years ago
|
||
Invert condition in if-statement as suggested in review comment. Carrying r+ from :evilpie.
Attachment #8906607 -
Attachment is obsolete: true
Attachment #8907277 -
Flags: review+
Assignee | ||
Comment 9•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0788b617badc84b193056a344f549e556f2a8720
Keywords: checkin-needed
Comment 10•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbf2ccf813e Part 1: Add fast-path for typed arrays in js::GetElements to speed-up Function.apply with typed arrays. r=evilpie https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f98cfb1a45 Part 2: Add fast path to convert int32 value to a code point. r=evilpie
Keywords: checkin-needed
![]() |
||
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fdbf2ccf813e https://hg.mozilla.org/mozilla-central/rev/f0f98cfb1a45
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•