Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671233002:
[typedarrays] move %TypedArray%.prototype.copyWithin to C++ (Closed)
Description[typedarrays] move %TypedArray%.prototype.copyWithin to C++
- Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js
- Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which
relies on std::memmove rather than accessing individual eleements.
- Fixes the case where copyWithin is invoked on a TypedArray with a
detached buffer.
- Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the
algorithm
The C++ version gets through the benchmark more than 25000 times as
quickly as the JS implementation.
BUG=v8:5925, v8:5929, v8:4648
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org
Review-Url: https://codereview.chromium.org/2671233002
Cr-Commit-Position: refs/heads/master@{#42975}
Committed: https://chromium.googlesource.com/v8/v8/+/0f1c626d556cbf84b0e572635eb803729f88cbb3
Patch Set 1 #Patch Set 2 : alphabetize JSTests.json #
Total comments: 5
Patch Set 3 : nits + move ValidateTypedArray to a helper as it is likely to be reused later #
Total comments: 7
Patch Set 4 : Add Object::ToFiniteInt64 thing and some tests #Patch Set 5 : version without changes to objects.h/objects-inl.h #Patch Set 6 : make win32 happy #
Total comments: 2
Messages
Total messages: 39 (25 generated)
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL when time allows, thanks. Also, if I should move this to gerrit, I can do that.
On 2017/02/06 01:32:02, caitp wrote: > PTAL when time allows, thanks. Also, if I should move this to gerrit, I can do > that. Can you land the perf test separately first? That way we see the improvement of this cl explicitly.
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
Nice, just couple of mostly nits. Thanks. https://codereview.chromium.org/2671233002/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2671233002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:2489: Builtins::kTypedArrayPrototypeCopyWithin, 2, false); Just use the SimpleInstallFunction which takes a C string here and avoid adding the root string (it's only used once). https://codereview.chromium.org/2671233002/diff/20001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:192: size_t len = obj->length_value(); How about using int64_t for these to avoid sprinkling static_cast everywhere. https://codereview.chromium.org/2671233002/diff/20001/src/heap-symbols.h File src/heap-symbols.h (right): https://codereview.chromium.org/2671233002/diff/20001/src/heap-symbols.h#newc... src/heap-symbols.h:53: V(copyWithin_string, "copyWithin") \ Avoid this. See comment in bootstrapped.cc
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2671233002/diff/20001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2671233002/diff/20001/src/bootstrapper.cc#new... src/bootstrapper.cc:2489: Builtins::kTypedArrayPrototypeCopyWithin, 2, false); On 2017/02/06 03:54:33, Benedikt Meurer wrote: > Just use the SimpleInstallFunction which takes a C string here and avoid adding > the root string (it's only used once). Done. https://codereview.chromium.org/2671233002/diff/20001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/20001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:192: size_t len = obj->length_value(); On 2017/02/06 03:54:33, Benedikt Meurer wrote: > How about using int64_t for these to avoid sprinkling static_cast everywhere. Done
Description was changed from ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Adds a js-perf-test featuring %TypedArray%.prototype.copyWithin(), which copies a reasonably large array of doubles. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ==========
On 2017/02/06 14:21:13, caitp wrote: > https://codereview.chromium.org/2671233002/diff/20001/src/bootstrapper.cc > File src/bootstrapper.cc (right): > > https://codereview.chromium.org/2671233002/diff/20001/src/bootstrapper.cc#new... > src/bootstrapper.cc:2489: Builtins::kTypedArrayPrototypeCopyWithin, 2, false); > On 2017/02/06 03:54:33, Benedikt Meurer wrote: > > Just use the SimpleInstallFunction which takes a C string here and avoid > adding > > the root string (it's only used once). > > Done. > > https://codereview.chromium.org/2671233002/diff/20001/src/builtins/builtins-t... > File src/builtins/builtins-typedarray.cc (right): > > https://codereview.chromium.org/2671233002/diff/20001/src/builtins/builtins-t... > src/builtins/builtins-typedarray.cc:192: size_t len = obj->length_value(); > On 2017/02/06 03:54:33, Benedikt Meurer wrote: > > How about using int64_t for these to avoid sprinkling static_cast everywhere. > > Done It looks like there should be a similar benefit to using memcpy for %TypedArray%.prototype.slice, particularly for 8bit and 16bit element sizes. After this lands, it shouldn't be too much work to do something similar for slice, but I'll add a benchmark to measure it
Almost there. LGTM once comment addressed. https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:209: int64_t relative = static_cast<int64_t>(target_int->Number()); I think this is wrong for infinities (also below). You probably need to do the comparisons on doubles. Please also add a test for this.
lgtm https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:209: int64_t relative = static_cast<int64_t>(target_int->Number()); nit: only if you feel motivated ;) could you add a Object::ToInt64 (check Object::ToUint32) which has a nicely inlineable fast-path for smi lengths and thus avoids an uncessary and costly double conversion. https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:219: relative = static_cast<int64_t>(start->Number()); ditto. https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:227: relative = static_cast<int64_t>(end->Number()); ditto.
test + fix shortly https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:209: int64_t relative = static_cast<int64_t>(target_int->Number()); On 2017/02/06 14:35:23, Benedikt Meurer wrote: > I think this is wrong for infinities (also below). You probably need to do the > comparisons on doubles. Please also add a test for this. That's a good point, didn't realize ToInteger() kept non-finite numbers. Dan: it seems like there isn't any test262 coverage for this point
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:209: int64_t relative = static_cast<int64_t>(target_int->Number()); On 2017/02/06 14:43:35, Camillo Bruni wrote: > nit: only if you feel motivated ;) could you add a Object::ToInt64 (check > Object::ToUint32) which has a nicely inlineable fast-path for smi lengths and > thus avoids an uncessary and costly double conversion. Because of the issue with -Infiity/+Infinity, we'd need it to be like `ToInt64Clamped()` to restrict -Infinity to 0, and +Infinity to be the length value. Does that seem reasonable? It could be a helper function specific to builtins-typedarray.cc
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add Object::ToFiniteInt64(), which performs ToInteger() but converts the resulting value to an int64, replacing non-finite values with passed in `minimum` / `maximum` values. For current uses, this should never cause problems as typed array lengths never exceed Smi::kMaxValue, but it could potentially cause problems if used carelessly. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ==========
https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/40001/src/builtins/builtins-t... src/builtins/builtins-typedarray.cc:209: int64_t relative = static_cast<int64_t>(target_int->Number()); On 2017/02/06 14:35:23, Benedikt Meurer wrote: > I think this is wrong for infinities (also below). You probably need to do the > comparisons on doubles. Please also add a test for this. Tests added, and I've tried to add a new helper for this as suggested by cbruni, but it's a bit awkward in some ways. What about something like ``` Maybe<int64_t> result = Object::ToInteger(isolate, input, &flags); if (flags & ToIntegerFlags::kFinite) { // Do the max(relativeXXX + len, len) stuff ... } else { to = (flags & ToIntegerFlags::kNegative) ? 0 : len; } ``` Or, alternatively, just using doubles the whole way as originally suggested.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
Description was changed from ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add Object::ToFiniteInt64(), which performs ToInteger() but converts the resulting value to an int64, replacing non-finite values with passed in `minimum` / `maximum` values. For current uses, this should never cause problems as typed array lengths never exceed Smi::kMaxValue, but it could potentially cause problems if used carelessly. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ==========
Description was changed from ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ==========
Description was changed from ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ==========
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2671233002/#ps100001 (title: "make win32 happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486401743474210,
"parent_rev": "9e248dde60ebe3af279746395c144b529cf5bf84", "commit_rev":
"0f1c626d556cbf84b0e572635eb803729f88cbb3"}
Message was sent while issue was closed.
Description was changed from ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org ========== to ========== [typedarrays] move %TypedArray%.prototype.copyWithin to C++ - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which relies on std::memmove rather than accessing individual eleements. - Fixes the case where copyWithin is invoked on a TypedArray with a detached buffer. - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the algorithm The C++ version gets through the benchmark more than 25000 times as quickly as the JS implementation. BUG=v8:5925, v8:5929, v8:4648 R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org Review-Url: https://codereview.chromium.org/2671233002 Cr-Commit-Position: refs/heads/master@{#42975} Committed: https://chromium.googlesource.com/v8/v8/+/0f1c626d556cbf84b0e572635eb803729f8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/0f1c626d556cbf84b0e572635eb803729f8...
Message was sent while issue was closed.
cwhan.tunz@gmail.com changed reviewers: + cwhan.tunz@gmail.com
Message was sent while issue was closed.
https://codereview.chromium.org/2671233002/diff/100001/src/builtins/builtins-... File src/builtins/builtins-typedarray.cc (right): https://codereview.chromium.org/2671233002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:217: isolate, array, ValiadateTypedArray(isolate, args.receiver(), method)); Validation check in here can be violated later. https://codereview.chromium.org/2671233002/diff/100001/src/builtins/builtins-... src/builtins/builtins-typedarray.cc:227: isolate, num, Object::ToInteger(isolate, args.at<Object>(1))); This ToInteger call may neuter the buf. See https://bugs.chromium.org/p/v8/issues/detail?id=5929#c5
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2693753002/ by littledan@chromium.org. The reason for reverting is: Due to security issue described in review thread.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
