Repository navigation
[VM] Speed up TypedArray creation from arrays - #2219
goutamadwant wants to merge 3 commits into
Conversation
Avoid iterator and iterator-result allocations when a JSArray uses the built-in array iterator methods. Preserve dynamic length, indexed access, and snapshot semantics while falling back for customized iterators. Add regression coverage for array growth and shrinkage, conversion and mapping side effects, and customized iterator methods. Fixes meta#2215
meta-cla
Bot
added
the
CLA Signed
| // Avoid allocating iterator and iterator-result objects for ordinary arrays. | ||
| // Both methods must be unmodified because the generic iterator protocol | ||
| // observes each of them. | ||
| auto sourceArray = Handle<JSArray>::dyn_vmcast(items); |
There was a problem hiding this comment.
Can you put this in a new method, iterableToArrayLikeFastPath?
| while (k < JSArray::getLength(*lv.sourceArray, runtime)) { | ||
| GCScopeMarkerRAII marker{gcScope}; | ||
|
|
||
| lv.nextValue = lv.sourceArray->at(runtime, k).unboxToHV(runtime); |
There was a problem hiding this comment.
I think ideally we want to to have another fast path for arrays that has no accessors and indexed properties in bound (see arrayFastPathCheck()).
lavenzg
left a comment
lavenzg
left a comment
There was a problem hiding this comment.
Could you update the benchmark numbers?
| JSArray::putLengthUnsafe(lv.A.get(), runtime, newLength); | ||
| for (uint32_t k = 0; k < length; ++k) { | ||
| if (LLVM_UNLIKELY(destStorage->at(k).isEmpty())) { | ||
| destStorage->set( |
There was a problem hiding this comment.
Please add a test covering this path.
| lv.nextValue = std::move(*valueRes); | ||
| } | ||
|
|
||
| if (LLVM_UNLIKELY(k == UINT32_MAX)) { |
There was a problem hiding this comment.
This is always false since k < length and length is uint32_t.
|
addressed all comments @lavenzg . Added a deleted-element test covering the empty-slot copy branch, mapper receiving
Can you pls take another look and let me know if it looks good now? Thanks! |
|
@lavenzg has imported this pull request. If you are a Meta employee, you can view this in D126991206. |
Thanks! All tests still pass without the loop of setting holes to undefined. I'll amend it directly after import. |
Summary: Add a guarded fast path for creating typed arrays from plain `JSArray` values when both the array iterator and its `next` method are unchanged. For arrays that pass `arrayFastPathCheck()`, copy the indexed storage and normalize empty slots to `undefined`. Other guarded arrays use an indexed loop that preserves dynamic length and indexed getters. Inputs with customized iterator behavior continue through the generic iterable path. Add regression coverage for array growth and shrinkage, conversion and mapping side effects, customized iterator methods, and a deleted interior element. The deleted-element case checks floating-point and integer conversion, mapper input, and source preservation. Fixes #2215 X-link: #2219 Reviewed By: avp Differential Revision: D126991206 Pulled By: lavenzg fbshipit-source-id: e5d47db0028d2c0c43d124cc6f6553c107be0f20
Summary
Add a guarded fast path for creating typed arrays from plain
JSArrayvalues when both the array iterator and itsnextmethod are unchanged.For arrays that pass
arrayFastPathCheck(), copy the indexed storage and normalize empty slots toundefined. Other guarded arrays use an indexed loop that preserves dynamic length and indexed getters. Inputs with customized iterator behavior continue through the generic iterable path.Add regression coverage for array growth and shrinkage, conversion and mapping side effects, customized iterator methods, and a deleted interior element. The deleted-element case checks floating-point and integer conversion, mapper input, and source preservation.
Fixes #2215
Test Plan
./utils/format.shtest/hermes/TypedArray.jsLIT test: passed under both Hermes and Static Hermes.LIT_OPTS=-j4 cmake --build cmake-build-release --target check-hermes -j 8Benchmark
Reran the script from #2215 with matched Release builds of the baseline (
ade4a2b20) and this branch. Both used-O3 -DNDEBUGwith-gline-tables-onlyand the same compiler.Each run uses 10 warmups, 30 samples, a 200-element source array, and 20,000 copies per sample. Ran baseline then updated, followed by updated then baseline. Ranges below span the two run medians, in milliseconds:
Float64Array.from(array): baseline 518-755; updated 40-48.new Float64Array(array): baseline 505-557; updated 39 in both runs.Uint8Array.from(array): baseline 612-644; updated 43-44.