Skip to content

Commit 1b49f72

Browse files
hashseedCommit Bot
hashseed
authored and
Commit Bot
committed
Revert "Reimplement Object.entries/values as CSA to optimize performance."
This reverts commit d30a8fa. Reason for revert: no-snap test failures here https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20debug/builds/17068 You need to update the whitelist in src/debug/debug-evaluate.cc. I'm a bit surprised this only happens in no-snap builds. Original change's description: > Reimplement Object.entries/values as CSA to optimize performance. > > This implementation based on runtime implementation. > > Bug: v8:6804 > Change-Id: Ib8bfcc4648e44a999789237effc0275c5e4d9936 > Reviewed-on: https://chromium-review.googlesource.com/810504 > Commit-Queue: Camillo Bruni <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Jakob Gruber <[email protected]> > Cr-Commit-Position: refs/heads/master@{#50477} [email protected],[email protected],[email protected],[email protected] Change-Id: I1a0c8e3c054a57ca4d15f7a064ff4b28ca133b16 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:6804 Reviewed-on: https://chromium-review.googlesource.com/859937 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#50478}
1 parent d30a8fa commit 1b49f72

11 files changed

Lines changed: 67 additions & 416 deletions

‎AUTHORS‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,6 @@ Sanjoy Das <[email protected]>
135135
Seo Sanghyeon <[email protected]>
136136
Stefan Penner <[email protected]>
137137
Sylvestre Ledru <[email protected]>
138-
Taketoshi Aono <[email protected]>
139138
Tiancheng "Timothy" Gu <[email protected]>
140139
Tobias Burnus <[email protected]>
141140
Victor Costan <[email protected]>

‎src/bootstrapper.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1506,9 +1506,9 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
15061506
object_function, "keys", Builtins::kObjectKeys, 1, true);
15071507
native_context()->set_object_keys(*object_keys);
15081508
SimpleInstallFunction(object_function, factory->entries_string(),
1509-
Builtins::kObjectEntries, 1, true);
1509+
Builtins::kObjectEntries, 1, false);
15101510
SimpleInstallFunction(object_function, factory->values_string(),
1511-
Builtins::kObjectValues, 1, true);
1511+
Builtins::kObjectValues, 1, false);
15121512

15131513
SimpleInstallFunction(isolate->initial_object_prototype(),
15141514
"__defineGetter__", Builtins::kObjectDefineGetter, 2,

‎src/builtins/builtins-definitions.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -754,7 +754,7 @@ namespace internal {
754754
CPP(ObjectDefineProperties) \
755755
CPP(ObjectDefineProperty) \
756756
CPP(ObjectDefineSetter) \
757-
TFJ(ObjectEntries, 1, kObject) \
757+
CPP(ObjectEntries) \
758758
CPP(ObjectFreeze) \
759759
TFJ(ObjectGetOwnPropertyDescriptor, \
760760
SharedFunctionInfo::kDontAdaptArgumentsSentinel) \
@@ -784,7 +784,7 @@ namespace internal {
784784
/* ES #sec-object.prototype.tolocalestring */ \
785785
TFJ(ObjectPrototypeToLocaleString, 0) \
786786
CPP(ObjectSeal) \
787-
TFJ(ObjectValues, 1, kObject) \
787+
CPP(ObjectValues) \
788788
\
789789
/* instanceof */ \
790790
TFC(OrdinaryHasInstance, Compare, 1) \

‎src/builtins/builtins-object-gen.cc‎

Lines changed: 0 additions & 301 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@ namespace internal {
1616
// ES6 section 19.1 Object Objects
1717

1818
typedef compiler::Node Node;
19-
template <class T>
20-
using TNode = CodeStubAssembler::TNode<T>;
2119

2220
class ObjectBuiltinsAssembler : public CodeStubAssembler {
2321
public:
@@ -36,46 +34,6 @@ class ObjectBuiltinsAssembler : public CodeStubAssembler {
3634
Node* ConstructDataDescriptor(Node* context, Node* value, Node* writable,
3735
Node* enumerable, Node* configurable);
3836
Node* GetAccessorOrUndefined(Node* accessor, Label* if_bailout);
39-
40-
Node* IsSpecialReceiverMap(SloppyTNode<Map> map);
41-
};
42-
43-
class ObjectEntriesValuesBuiltinsAssembler : public ObjectBuiltinsAssembler {
44-
public:
45-
explicit ObjectEntriesValuesBuiltinsAssembler(
46-
compiler::CodeAssemblerState* state)
47-
: ObjectBuiltinsAssembler(state) {}
48-
49-
protected:
50-
enum CollectType { kEntries, kValues };
51-
52-
TNode<Word32T> IsStringWrapperElementsKind(TNode<Map> map);
53-
54-
TNode<BoolT> IsPropertyEnumerable(TNode<Uint32T> details);
55-
56-
TNode<BoolT> IsPropertyKindAccessor(TNode<Uint32T> kind);
57-
58-
TNode<BoolT> IsPropertyKindData(TNode<Uint32T> kind);
59-
60-
TNode<Uint32T> HasHiddenPrototype(TNode<Map> map);
61-
62-
TNode<Uint32T> LoadPropertyKind(TNode<Uint32T> details) {
63-
return DecodeWord32<PropertyDetails::KindField>(details);
64-
}
65-
66-
void GetOwnValuesOrEntries(TNode<Context> context, TNode<Object> maybe_object,
67-
CollectType collect_type);
68-
69-
void GotoIfMapHasSlowProperties(TNode<Map> map, Label* if_slow);
70-
71-
TNode<JSArray> FastGetOwnValuesOrEntries(
72-
TNode<Context> context, TNode<JSObject> object,
73-
Label* if_call_runtime_with_fast_path, Label* if_no_properties,
74-
CollectType collect_type);
75-
76-
TNode<JSArray> FinalizeValuesOrEntriesJSArray(
77-
TNode<Context> context, TNode<FixedArray> values_or_entries,
78-
TNode<IntPtrT> size, TNode<Map> array_map, Label* if_empty);
7937
};
8038

8139
void ObjectBuiltinsAssembler::ReturnToStringFormat(Node* context,
@@ -139,249 +97,6 @@ Node* ObjectBuiltinsAssembler::ConstructDataDescriptor(Node* context,
13997
return js_desc;
14098
}
14199

142-
Node* ObjectBuiltinsAssembler::IsSpecialReceiverMap(SloppyTNode<Map> map) {
143-
CSA_SLOW_ASSERT(this, IsMap(map));
144-
Node* is_special = IsSpecialReceiverInstanceType(LoadMapInstanceType(map));
145-
uint32_t mask =
146-
Map::HasNamedInterceptorBit::kMask | Map::IsAccessCheckNeededBit::kMask;
147-
USE(mask);
148-
// Interceptors or access checks imply special receiver.
149-
CSA_ASSERT(this,
150-
SelectConstant(IsSetWord32(LoadMapBitField(map), mask), is_special,
151-
Int32Constant(1), MachineRepresentation::kWord32));
152-
return is_special;
153-
}
154-
155-
TNode<Word32T>
156-
ObjectEntriesValuesBuiltinsAssembler::IsStringWrapperElementsKind(
157-
TNode<Map> map) {
158-
Node* kind = LoadMapElementsKind(map);
159-
return Word32Or(
160-
Word32Equal(kind, Int32Constant(FAST_STRING_WRAPPER_ELEMENTS)),
161-
Word32Equal(kind, Int32Constant(SLOW_STRING_WRAPPER_ELEMENTS)));
162-
}
163-
164-
TNode<BoolT> ObjectEntriesValuesBuiltinsAssembler::IsPropertyEnumerable(
165-
TNode<Uint32T> details) {
166-
TNode<Uint32T> attributes =
167-
DecodeWord32<PropertyDetails::AttributesField>(details);
168-
return IsNotSetWord32(attributes, PropertyAttributes::DONT_ENUM);
169-
}
170-
171-
TNode<BoolT> ObjectEntriesValuesBuiltinsAssembler::IsPropertyKindAccessor(
172-
TNode<Uint32T> kind) {
173-
return Word32Equal(kind, Int32Constant(PropertyKind::kAccessor));
174-
}
175-
176-
TNode<BoolT> ObjectEntriesValuesBuiltinsAssembler::IsPropertyKindData(
177-
TNode<Uint32T> kind) {
178-
return Word32Equal(kind, Int32Constant(PropertyKind::kData));
179-
}
180-
181-
TNode<Uint32T> ObjectEntriesValuesBuiltinsAssembler::HasHiddenPrototype(
182-
TNode<Map> map) {
183-
TNode<Uint32T> bit_field3 = LoadMapBitField3(map);
184-
return DecodeWord32<Map::HasHiddenPrototypeBit>(bit_field3);
185-
}
186-
187-
void ObjectEntriesValuesBuiltinsAssembler::GetOwnValuesOrEntries(
188-
TNode<Context> context, TNode<Object> maybe_object,
189-
CollectType collect_type) {
190-
TNode<JSObject> object = TNode<JSObject>::UncheckedCast(
191-
CallBuiltin(Builtins::kToObject, context, maybe_object));
192-
193-
Label if_call_runtime_with_fast_path(this, Label::kDeferred),
194-
if_call_runtime(this, Label::kDeferred),
195-
if_no_properties(this, Label::kDeferred);
196-
197-
TNode<Map> map = LoadMap(object);
198-
GotoIfNot(IsJSObjectMap(map), &if_call_runtime);
199-
GotoIfMapHasSlowProperties(map, &if_call_runtime);
200-
201-
TNode<FixedArrayBase> elements = LoadElements(object);
202-
// If the object has elements, we treat it as slow case.
203-
// So, we go to runtime call.
204-
GotoIfNot(IsEmptyFixedArray(elements), &if_call_runtime_with_fast_path);
205-
206-
TNode<JSArray> result = FastGetOwnValuesOrEntries(
207-
context, object, &if_call_runtime_with_fast_path, &if_no_properties,
208-
collect_type);
209-
Return(result);
210-
211-
BIND(&if_no_properties);
212-
{
213-
Node* native_context = LoadNativeContext(context);
214-
Node* array_map = LoadJSArrayElementsMap(PACKED_ELEMENTS, native_context);
215-
Node* empty_array = AllocateJSArray(PACKED_ELEMENTS, array_map,
216-
IntPtrConstant(0), SmiConstant(0));
217-
Return(empty_array);
218-
}
219-
220-
BIND(&if_call_runtime_with_fast_path);
221-
{
222-
// In slow case, we simply call runtime.
223-
if (collect_type == CollectType::kEntries) {
224-
Return(CallRuntime(Runtime::kObjectEntries, context, object));
225-
} else {
226-
DCHECK(collect_type == CollectType::kValues);
227-
Return(CallRuntime(Runtime::kObjectValues, context, object));
228-
}
229-
}
230-
231-
BIND(&if_call_runtime);
232-
{
233-
// In slow case, we simply call runtime.
234-
if (collect_type == CollectType::kEntries) {
235-
Return(CallRuntime(Runtime::kObjectEntriesSkipFastPath, context, object));
236-
} else {
237-
DCHECK(collect_type == CollectType::kValues);
238-
Return(CallRuntime(Runtime::kObjectValuesSkipFastPath, context, object));
239-
}
240-
}
241-
}
242-
243-
void ObjectEntriesValuesBuiltinsAssembler::GotoIfMapHasSlowProperties(
244-
TNode<Map> map, Label* if_slow) {
245-
GotoIf(IsStringWrapperElementsKind(map), if_slow);
246-
GotoIf(IsSpecialReceiverMap(map), if_slow);
247-
GotoIf(HasHiddenPrototype(map), if_slow);
248-
GotoIf(IsDictionaryMap(map), if_slow);
249-
}
250-
251-
TNode<JSArray> ObjectEntriesValuesBuiltinsAssembler::FastGetOwnValuesOrEntries(
252-
TNode<Context> context, TNode<JSObject> object,
253-
Label* if_call_runtime_with_fast_path, Label* if_no_properties,
254-
CollectType collect_type) {
255-
Node* native_context = LoadNativeContext(context);
256-
TNode<Map> array_map =
257-
LoadJSArrayElementsMap(PACKED_ELEMENTS, native_context);
258-
TNode<Map> map = LoadMap(object);
259-
TNode<Uint32T> bit_field3 = LoadMapBitField3(map);
260-
261-
Label if_has_enum_cache(this), if_not_has_enum_cache(this),
262-
collect_entries(this);
263-
Node* object_enum_length =
264-
DecodeWordFromWord32<Map::EnumLengthBits>(bit_field3);
265-
Node* has_enum_cache = WordNotEqual(
266-
object_enum_length, IntPtrConstant(kInvalidEnumCacheSentinel));
267-
268-
// In case, we found enum_cache in object,
269-
// we use it as array_length becuase it has same size for
270-
// Object.(entries/values) result array object length.
271-
// So object_enum_length use less memory space than
272-
// NumberOfOwnDescriptorsBits value.
273-
// And in case, if enum_cache_not_found,
274-
// we call runtime and initialize enum_cache for subsequent call of
275-
// CSA fast path.
276-
Branch(has_enum_cache, &if_has_enum_cache, if_call_runtime_with_fast_path);
277-
278-
BIND(&if_has_enum_cache);
279-
{
280-
GotoIf(WordEqual(object_enum_length, IntPtrConstant(0)), if_no_properties);
281-
TNode<FixedArray> values_or_entries = TNode<FixedArray>::UncheckedCast(
282-
AllocateFixedArray(PACKED_ELEMENTS, object_enum_length,
283-
INTPTR_PARAMETERS, kAllowLargeObjectAllocation));
284-
285-
// If in case we have enum_cache,
286-
// we can't detect accessor of object until loop through descritpros.
287-
// So if object might have accessor,
288-
// we will remain invalid addresses of FixedArray.
289-
// Because in that case, we need to jump to runtime call.
290-
// So the array filled by the-hole even if enum_cache exists.
291-
FillFixedArrayWithValue(PACKED_ELEMENTS, values_or_entries,
292-
IntPtrConstant(0), object_enum_length,
293-
Heap::kTheHoleValueRootIndex);
294-
295-
TVARIABLE(IntPtrT, var_result_index, IntPtrConstant(0));
296-
TVARIABLE(IntPtrT, var_descriptor_index, IntPtrConstant(0));
297-
Variable* vars[] = {&var_descriptor_index, &var_result_index};
298-
// Let desc be ? O.[[GetOwnProperty]](key).
299-
TNode<DescriptorArray> descriptors = LoadMapDescriptors(map);
300-
Label loop(this, 2, vars), after_loop(this), loop_condition(this);
301-
Branch(IntPtrEqual(var_descriptor_index, object_enum_length), &after_loop,
302-
&loop);
303-
304-
// We dont use BuildFastLoop.
305-
// Instead, we use hand-written loop
306-
// because of we need to use 'continue' functionality.
307-
BIND(&loop);
308-
{
309-
// Currently, we will not invoke getters,
310-
// so, map will not be changed.
311-
CSA_ASSERT(this, WordEqual(map, LoadMap(object)));
312-
TNode<Uint32T> descriptor_index = TNode<Uint32T>::UncheckedCast(
313-
TruncateWordToWord32(var_descriptor_index));
314-
Node* next_key = DescriptorArrayGetKey(descriptors, descriptor_index);
315-
316-
// Skip Symbols.
317-
GotoIf(IsSymbol(next_key), &loop_condition);
318-
319-
TNode<Uint32T> details = TNode<Uint32T>::UncheckedCast(
320-
DescriptorArrayGetDetails(descriptors, descriptor_index));
321-
TNode<Uint32T> kind = LoadPropertyKind(details);
322-
323-
// If property is accessor, we escape fast path and call runtime.
324-
GotoIf(IsPropertyKindAccessor(kind), if_call_runtime_with_fast_path);
325-
CSA_ASSERT(this, IsPropertyKindData(kind));
326-
327-
// If desc is not undefined and desc.[[Enumerable]] is true, then
328-
GotoIfNot(IsPropertyEnumerable(details), &loop_condition);
329-
330-
VARIABLE(var_property_value, MachineRepresentation::kTagged,
331-
UndefinedConstant());
332-
Node* descriptor_name_index = DescriptorNumberToIndex(descriptor_index);
333-
334-
// Let value be ? Get(O, key).
335-
LoadPropertyFromFastObject(object, map, descriptors,
336-
descriptor_name_index, details,
337-
&var_property_value);
338-
339-
// If kind is "value", append value to properties.
340-
Node* value = var_property_value.value();
341-
342-
if (collect_type == CollectType::kEntries) {
343-
// Let entry be CreateArrayFromList(« key, value »).
344-
Node* array = nullptr;
345-
Node* elements = nullptr;
346-
std::tie(array, elements) = AllocateUninitializedJSArrayWithElements(
347-
PACKED_ELEMENTS, array_map, SmiConstant(2), nullptr,
348-
IntPtrConstant(2));
349-
StoreFixedArrayElement(elements, 0, next_key, SKIP_WRITE_BARRIER);
350-
StoreFixedArrayElement(elements, 1, value, SKIP_WRITE_BARRIER);
351-
value = array;
352-
}
353-
354-
StoreFixedArrayElement(values_or_entries, var_result_index, value);
355-
Increment(&var_result_index, 1);
356-
Goto(&loop_condition);
357-
358-
BIND(&loop_condition);
359-
{
360-
Increment(&var_descriptor_index, 1);
361-
Branch(IntPtrEqual(var_descriptor_index, object_enum_length),
362-
&after_loop, &loop);
363-
}
364-
}
365-
BIND(&after_loop);
366-
return FinalizeValuesOrEntriesJSArray(context, values_or_entries,
367-
var_result_index, array_map,
368-
if_no_properties);
369-
}
370-
}
371-
372-
TNode<JSArray>
373-
ObjectEntriesValuesBuiltinsAssembler::FinalizeValuesOrEntriesJSArray(
374-
TNode<Context> context, TNode<FixedArray> result, TNode<IntPtrT> size,
375-
TNode<Map> array_map, Label* if_empty) {
376-
CSA_ASSERT(this, IsJSArrayMap(array_map));
377-
378-
GotoIf(IntPtrEqual(size, IntPtrConstant(0)), if_empty);
379-
Node* array = AllocateUninitializedJSArrayWithoutElements(
380-
array_map, SmiTag(size), nullptr);
381-
StoreObjectField(array, JSArray::kElementsOffset, result);
382-
return TNode<JSArray>::UncheckedCast(array);
383-
}
384-
385100
TF_BUILTIN(ObjectPrototypeToLocaleString, CodeStubAssembler) {
386101
TNode<Context> context = CAST(Parameter(Descriptor::kContext));
387102
TNode<Object> receiver = CAST(Parameter(Descriptor::kReceiver));
@@ -553,22 +268,6 @@ TF_BUILTIN(ObjectKeys, ObjectBuiltinsAssembler) {
553268
}
554269
}
555270

556-
TF_BUILTIN(ObjectValues, ObjectEntriesValuesBuiltinsAssembler) {
557-
TNode<JSObject> object =
558-
TNode<JSObject>::UncheckedCast(Parameter(Descriptor::kObject));
559-
TNode<Context> context =
560-
TNode<Context>::UncheckedCast(Parameter(Descriptor::kContext));
561-
GetOwnValuesOrEntries(context, object, CollectType::kValues);
562-
}
563-
564-
TF_BUILTIN(ObjectEntries, ObjectEntriesValuesBuiltinsAssembler) {
565-
TNode<JSObject> object =
566-
TNode<JSObject>::UncheckedCast(Parameter(Descriptor::kObject));
567-
TNode<Context> context =
568-
TNode<Context>::UncheckedCast(Parameter(Descriptor::kContext));
569-
GetOwnValuesOrEntries(context, object, CollectType::kEntries);
570-
}
571-
572271
// ES #sec-object.prototype.isprototypeof
573272
TF_BUILTIN(ObjectPrototypeIsPrototypeOf, ObjectBuiltinsAssembler) {
574273
Node* receiver = Parameter(Descriptor::kReceiver);

‎src/builtins/builtins-object.cc‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,31 @@ BUILTIN(ObjectIsSealed) {
395395
return isolate->heap()->ToBoolean(result.FromJust());
396396
}
397397

398+
BUILTIN(ObjectValues) {
399+
HandleScope scope(isolate);
400+
Handle<Object> object = args.atOrUndefined(isolate, 1);
401+
Handle<JSReceiver> receiver;
402+
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
403+
Object::ToObject(isolate, object));
404+
Handle<FixedArray> values;
405+
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
406+
isolate, values, JSReceiver::GetOwnValues(receiver, ENUMERABLE_STRINGS));
407+
return *isolate->factory()->NewJSArrayWithElements(values);
408+
}
409+
410+
BUILTIN(ObjectEntries) {
411+
HandleScope scope(isolate);
412+
Handle<Object> object = args.atOrUndefined(isolate, 1);
413+
Handle<JSReceiver> receiver;
414+
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
415+
Object::ToObject(isolate, object));
416+
Handle<FixedArray> entries;
417+
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
418+
isolate, entries,
419+
JSReceiver::GetOwnEntries(receiver, ENUMERABLE_STRINGS));
420+
return *isolate->factory()->NewJSArrayWithElements(entries);
421+
}
422+
398423
BUILTIN(ObjectGetOwnPropertyDescriptors) {
399424
HandleScope scope(isolate);
400425
Handle<Object> object = args.atOrUndefined(isolate, 1);

0 commit comments

Comments
 (0)