Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521043005:
[elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE (Closed)
|
Created:
4 years ago by Camillo Bruni Modified:
4 years ago Reviewers:
Jakob Kummerow CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE
Make apply calls with double array arguments such as
Math.min.apply(Math, [1.1, 2.2]) 1.6x faster.
Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl.
BUG=v8:4826
Committed: https://crrev.com/4ec41c355cde0e73f5dcbcf97eb2e6bc0615255a
Cr-Commit-Position: refs/heads/master@{#41268}
Patch Set 1 #Patch Set 2 : avoid unnecessary writes #
Total comments: 3
Patch Set 3 : addressing nits #Patch Set 4 : merge with master #
Messages
Total messages: 27 (19 generated)
The CQ bit was checked by cbruni@chromium.org 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: This issue passed the CQ dry run.
Description was changed from ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE BUG=chromium:4826 ========== to ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=chromium:4826 ==========
cbruni@chromium.org changed reviewers: + jkummerow@chromium.org
PTAL
Description was changed from ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=chromium:4826 ========== to ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=v8:4826 ==========
lgtm https://codereview.chromium.org/2521043005/diff/20001/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2521043005/diff/20001/src/elements.cc#newcode... src/elements.cc:2281: for (int i = 0; i < elements->length(); i++) { s/elements->length()/length/ for a minor gain in efficiency. (And s/int/uint32_t/ to match.) https://codereview.chromium.org/2521043005/diff/20001/src/elements.h File src/elements.h (right): https://codereview.chromium.org/2521043005/diff/20001/src/elements.h#newcode178 src/elements.h:178: virtual Handle<FixedArray> CreateListFromArrayLike(Isolate* isolate, nit: bit of a misnomer. Since it takes a JSArray (and not any JSObject with a "length" property), it should be called "CreateListFromArray".
The CQ bit was checked by cbruni@chromium.org 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2521043005/diff/20001/src/elements.h File src/elements.h (right): https://codereview.chromium.org/2521043005/diff/20001/src/elements.h#newcode178 src/elements.h:178: virtual Handle<FixedArray> CreateListFromArrayLike(Isolate* isolate, On 2016/11/24 at 10:55:16, Jakob Kummerow wrote: > nit: bit of a misnomer. Since it takes a JSArray (and not any JSObject with a "length" property), it should be called "CreateListFromArray". ack.
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2521043005/#ps40001 (title: "addressing nits")
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
Failed to apply patch for src/elements.cc:
While running git apply --index -p1;
error: patch failed: src/elements.cc:3338
error: src/elements.cc: patch does not apply
Patch: src/elements.cc
Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
index
10fc1be97b165430e40f833f54deb2f8f2d8ecef..a736a7d1f007b1489d44cdcda2be6553de838ed8
100644
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -603,7 +603,7 @@ class ElementsAccessorBase : public ElementsAccessor {
static bool HasElementImpl(Isolate* isolate, Handle<JSObject> holder,
uint32_t index,
Handle<FixedArrayBase> backing_store,
- PropertyFilter filter) {
+ PropertyFilter filter = ALL_PROPERTIES) {
return Subclass::GetEntryForIndexImpl(isolate, *holder, *backing_store,
index, filter) != kMaxUInt32;
}
@@ -618,15 +618,16 @@ class ElementsAccessorBase : public ElementsAccessor {
}
Handle<Object> Get(Handle<JSObject> holder, uint32_t entry) final {
- return Subclass::GetImpl(holder, entry);
+ return Subclass::GetInternalImpl(holder, entry);
}
- static Handle<Object> GetImpl(Handle<JSObject> holder, uint32_t entry) {
- return Subclass::GetImpl(holder->elements(), entry);
+ static Handle<Object> GetInternalImpl(Handle<JSObject> holder,
+ uint32_t entry) {
+ return Subclass::GetImpl(holder->GetIsolate(), holder->elements(), entry);
}
- static Handle<Object> GetImpl(FixedArrayBase* backing_store, uint32_t entry)
{
- Isolate* isolate = backing_store->GetIsolate();
+ static Handle<Object> GetImpl(Isolate* isolate, FixedArrayBase*
backing_store,
+ uint32_t entry) {
uint32_t index = GetIndexForEntryImpl(backing_store, entry);
return handle(BackingStore::cast(backing_store)->get(index), isolate);
}
@@ -1033,7 +1034,7 @@ class ElementsAccessorBase : public ElementsAccessor {
PropertyDetails details = Subclass::GetDetailsImpl(*object, entry);
if (details.kind() == kData) {
- value = Subclass::GetImpl(object, entry);
+ value = Subclass::GetImpl(isolate, object->elements(), entry);
} else {
LookupIterator it(isolate, object, index, LookupIterator::OWN);
ASSIGN_RETURN_ON_EXCEPTION_VALUE(
@@ -1258,6 +1259,17 @@ class ElementsAccessorBase : public ElementsAccessor {
return Subclass::GetDetailsImpl(holder, entry);
}
+ Handle<FixedArray> CreateListFromArray(Isolate* isolate,
+ Handle<JSArray> array) final {
+ return Subclass::CreateListFromArrayImpl(isolate, array);
+ };
+
+ static Handle<FixedArray> CreateListFromArrayImpl(Isolate* isolate,
+ Handle<JSArray> array) {
+ UNREACHABLE();
+ return Handle<FixedArray>();
+ }
+
private:
DISALLOW_COPY_AND_ASSIGN(ElementsAccessorBase);
};
@@ -1383,12 +1395,9 @@ class DictionaryElementsAccessor
return backing_store->ValueAt(entry);
}
- static Handle<Object> GetImpl(Handle<JSObject> holder, uint32_t entry) {
- return GetImpl(holder->elements(), entry);
- }
-
- static Handle<Object> GetImpl(FixedArrayBase* backing_store, uint32_t entry)
{
- return handle(GetRaw(backing_store, entry), backing_store->GetIsolate());
+ static Handle<Object> GetImpl(Isolate* isolate, FixedArrayBase*
backing_store,
+ uint32_t entry) {
+ return handle(GetRaw(backing_store, entry), isolate);
}
static inline void SetImpl(Handle<JSObject> holder, uint32_t entry,
@@ -1771,7 +1780,7 @@ class FastElementsAccessor : public
ElementsAccessorBase<Subclass, KindTraits> {
if (IsHoleyElementsKind(kind)) {
if (BackingStore::cast(*store)->is_the_hole(isolate, i)) continue;
}
- Handle<Object> value = Subclass::GetImpl(*store, i);
+ Handle<Object> value = Subclass::GetImpl(isolate, *store, i);
dictionary = SeededNumberDictionary::AddNumberEntry(
dictionary, i, value, details, used_as_prototype);
j++;
@@ -1932,7 +1941,7 @@ class FastElementsAccessor : public
ElementsAccessorBase<Subclass, KindTraits> {
for (uint32_t i = 0; i < length; i++) {
if (IsFastPackedElementsKind(KindTraits::Kind) ||
HasEntryImpl(isolate, *elements, i)) {
- accumulator->AddKey(Subclass::GetImpl(*elements, i), convert);
+ accumulator->AddKey(Subclass::GetImpl(isolate, *elements, i), convert);
}
}
}
@@ -2073,7 +2082,7 @@ class FastElementsAccessor : public
ElementsAccessorBase<Subclass, KindTraits> {
uint32_t length = elements->length();
for (uint32_t index = 0; index < length; ++index) {
if (!HasEntryImpl(isolate, *elements, index)) continue;
- Handle<Object> value = Subclass::GetImpl(*elements, index);
+ Handle<Object> value = Subclass::GetImpl(isolate, *elements, index);
if (get_entries) {
value = MakeEntryPair(isolate, index, value);
}
@@ -2263,6 +2272,24 @@ class FastElementsAccessor : public
ElementsAccessorBase<Subclass, KindTraits> {
}
}
+ static Handle<FixedArray> CreateListFromArrayImpl(Isolate* isolate,
+ Handle<JSArray> array) {
+ uint32_t length = 0;
+ array->length()->ToArrayLength(&length);
+ Handle<FixedArray> result = isolate->factory()->NewFixedArray(length);
+ Handle<FixedArrayBase> elements(array->elements(), isolate);
+ for (uint32_t i = 0; i < length; i++) {
+ if (!Subclass::HasElementImpl(isolate, array, i, elements)) continue;
+ Handle<Object> value;
+ value = Subclass::GetImpl(isolate, *elements, i);
+ if (value->IsName()) {
+ value = isolate->factory()->InternalizeName(Handle<Name>::cast(value));
+ }
+ result->set(i, *value);
+ }
+ return result;
+ }
+
private:
// SpliceShrinkStep might modify the backing_store.
static void SpliceShrinkStep(Isolate* isolate, Handle<JSArray> receiver,
@@ -2321,7 +2348,8 @@ class FastElementsAccessor : public
ElementsAccessorBase<Subclass, KindTraits> {
DCHECK(length > 0);
int new_length = length - 1;
int remove_index = remove_position == AT_START ? 0 : new_length;
- Handle<Object> result = Subclass::GetImpl(*backing_store, remove_index);
+ Handle<Object> result =
+ Subclass::GetImpl(isolate, *backing_store, remove_index);
if (remove_position == AT_START) {
Subclass::MoveElements(isolate, receiver, backing_store, 0, 1,
new_length,
0, 0);
@@ -2542,12 +2570,8 @@ class FastDoubleElementsAccessor
explicit FastDoubleElementsAccessor(const char* name)
: FastElementsAccessor<Subclass, KindTraits>(name) {}
- static Handle<Object> GetImpl(Handle<JSObject> holder, uint32_t entry) {
- return GetImpl(holder->elements(), entry);
- }
-
- static Handle<Object> GetImpl(FixedArrayBase* backing_store, uint32_t entry)
{
- Isolate* isolate = backing_store->GetIsolate();
+ static Handle<Object> GetImpl(Isolate* isolate, FixedArrayBase*
backing_store,
+ uint32_t entry) {
return FixedDoubleArray::get(FixedDoubleArray::cast(backing_store), entry,
isolate);
}
@@ -2694,11 +2718,8 @@ class TypedElementsAccessor
BackingStore::cast(backing_store)->SetValue(entry, value);
}
- static Handle<Object> GetImpl(Handle<JSObject> holder, uint32_t entry) {
- return GetImpl(holder->elements(), entry);
- }
-
- static Handle<Object> GetImpl(FixedArrayBase* backing_store, uint32_t entry)
{
+ static Handle<Object> GetImpl(Isolate* isolate, FixedArrayBase*
backing_store,
+ uint32_t entry) {
return BackingStore::get(BackingStore::cast(backing_store), entry);
}
@@ -2762,10 +2783,11 @@ class TypedElementsAccessor
static void AddElementsToKeyAccumulatorImpl(Handle<JSObject> receiver,
KeyAccumulator* accumulator,
AddKeyConversion convert) {
+ Isolate* isolate = receiver->GetIsolate();
Handle<FixedArrayBase> elements(receiver->elements());
uint32_t length = AccessorClass::GetCapacityImpl(*receiver, *elements);
for (uint32_t i = 0; i < length; i++) {
- Handle<Object> value = AccessorClass::GetImpl(*elements, i);
+ Handle<Object> value = AccessorClass::GetImpl(isolate, *elements, i);
accumulator->AddKey(value, convert);
}
}
@@ -2779,7 +2801,8 @@ class TypedElementsAccessor
Handle<FixedArrayBase> elements(object->elements());
uint32_t length = AccessorClass::GetCapacityImpl(*object, *elements);
for (uint32_t index = 0; index < length; ++index) {
- Handle<Object> value = AccessorClass::GetImpl(*elements, index);
+ Handle<Object> value =
+ AccessorClass::GetImpl(isolate, *elements, index);
if (get_entries) {
value = MakeEntryPair(isolate, index, value);
}
@@ -2902,12 +2925,8 @@ class SloppyArgumentsElementsAccessor
USE(KindTraits::Kind);
}
- static Handle<Object> GetImpl(Handle<JSObject> holder, uint32_t entry) {
- return GetImpl(holder->elements(), entry);
- }
-
- static Handle<Object> GetImpl(FixedArrayBase* parameters, uint32_t entry) {
- Isolate* isolate = parameters->GetIsolate();
+ static Handle<Object> GetImpl(Isolate* isolate, FixedArrayBase* parameters,
+ uint32_t entry) {
Handle<FixedArray> parameter_map(FixedArray::cast(parameters), isolate);
uint32_t length = parameter_map->length() - 2;
if (entry < length) {
@@ -2920,7 +2939,7 @@ class SloppyArgumentsElementsAccessor
} else {
// Object is not mapped, defer to the arguments.
Handle<Object> r…
(message too large)
The CQ bit was checked by cbruni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2521043005/#ps60001 (title: "merge with master")
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": 60001, "attempt_start_ts": 1479997346364670,
"parent_rev": "d2f1602af65a3d6e39b3e1acb6ce72ee67b9663d", "commit_rev":
"0f415beca340c222b98dde8a8ae2316e1a72147f"}
Message was sent while issue was closed.
Description was changed from ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=v8:4826 ========== to ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=v8:4826 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=v8:4826 ========== to ========== [elements] Add CreateFromArrayLike fast-path for JS_ARRAY_TYPE Make apply calls with double array arguments such as Math.min.apply(Math, [1.1, 2.2]) 1.6x faster. Drive-by-fix: pass in the isolate to ElementsAccessor::GetImpl. BUG=v8:4826 Committed: https://crrev.com/4ec41c355cde0e73f5dcbcf97eb2e6bc0615255a Cr-Commit-Position: refs/heads/master@{#41268} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4ec41c355cde0e73f5dcbcf97eb2e6bc0615255a Cr-Commit-Position: refs/heads/master@{#41268} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
