AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

[const-tracking] Generalize constness when delete properties

Browse Source 
When fast deleting properties generalize all outgoing transitions
to mutable instead of generalizing when property is reconfigured.

Bug: chromium:1201938
Change-Id: I080f2f43de1691a742be2a2bec5cd20d02d78dbc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2859960
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74334}
This commit is contained in:
Igor Sheludko 2021-05-03 18:26:15 +02:00 committed by V8 LUCI CQ
parent 620da72cef
commit d570bbe0c7
11 changed files with 274 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/builtins/builtins-internal-gen.cc
View File

@ -544,8 +544,8 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) {
Label dictionary(this), dont_delete(this);
GotoIf(IsDictionaryMap(receiver_map), &dictionary);
// Fast properties need to clear recorded slots, which can only be done
// in C++.
// Fast properties need to clear recorded slots and mark the deleted
// property as mutable, which can only be done in C++.
Goto(&slow);
BIND(&dictionary);

4
src/objects/map-inl.h
View File

@ -190,6 +190,10 @@ bool Map::TooManyFastProperties(StoreOrigin store_origin) const {
}
}
Name Map::GetLastDescriptorName(Isolate* isolate) const {
return instance_descriptors(isolate).GetKey(LastAdded());
}
PropertyDetails Map::GetLastDescriptorDetails(Isolate* isolate) const {
return instance_descriptors(isolate).GetDetails(LastAdded());
}

45
src/objects/map-updater.cc
View File

@ -192,49 +192,6 @@ Handle<Map> MapUpdater::ReconfigureToDataField(InternalIndex descriptor,
PropertyDetails old_details =
old_descriptors_->GetDetails(modified_descriptor_);
// If the {descriptor} was "const" data field so far, we need to update the
// {old_map_} here, otherwise we could get the constants wrong, i.e.
//
// o.x = 1;
// change o.x's attributes to something else
// delete o.x;
// o.x = 2;
//
// could trick V8 into thinking that `o.x` is still 1 even after the second
// assignment.
// This situation is similar to what might happen with property deletion.
if (old_details.constness() == PropertyConstness::kConst &&
old_details.location() == kField &&
old_details.attributes() != new_attributes_) {
// Ensure we'll be updating constness of the up-to-date version of old_map_.
Handle<Map> old_map = UpdateMapNoLock(isolate_, old_map_);
PropertyDetails details =
old_map->instance_descriptors(isolate_).GetDetails(descriptor);
Handle<FieldType> field_type(
old_map->instance_descriptors(isolate_).GetFieldType(descriptor),
isolate_);
GeneralizeField(isolate_, old_map, descriptor, PropertyConstness::kMutable,
details.representation(), field_type);
DCHECK_EQ(PropertyConstness::kMutable,
old_map->instance_descriptors(isolate_)
.GetDetails(descriptor)
.constness());
// The old_map_'s property must become mutable.
// Note, that the {old_map_} and {old_descriptors_} are not expected to be
// updated by the generalization if the map is already deprecated.
DCHECK_IMPLIES(
!old_map_->is_deprecated(),
PropertyConstness::kMutable ==
old_descriptors_->GetDetails(modified_descriptor_).constness());
// Although the property in the old map is marked as mutable we still
// treat it as constant when merging with the new path in transition tree.
// This is fine because up until this reconfiguration the field was
// known to be constant, so it's fair to proceed treating it as such
// during this reconfiguration session. The issue is that after the
// reconfiguration the original field might become mutable (see the delete
// example above).
}
// If property kind is not reconfigured merge the result with
// representation/field type from the old descriptor.
if (old_details.kind() == new_kind_) {
@ -1051,6 +1008,8 @@ void MapUpdater::GeneralizeField(Isolate* isolate, Handle<Map> map,
PropertyConstness new_constness,
Representation new_representation,
Handle<FieldType> new_field_type) {
DCHECK(!map->is_deprecated());
// Check if we actually need to generalize the field type at all.
Handle<DescriptorArray> old_descriptors(map->instance_descriptors(isolate),
isolate);

1
src/objects/map.h
View File

@ -611,6 +611,7 @@ class Map : public HeapObject {
// chain state.
inline bool IsPrototypeValidityCellValid() const;
inline Name GetLastDescriptorName(Isolate* isolate) const;
inline PropertyDetails GetLastDescriptorDetails(Isolate* isolate) const;
inline InternalIndex LastAdded() const;

11
src/objects/property-details.h
View File

@ -32,6 +32,12 @@ enum PropertyAttributes {
// a non-existent property.
};
// Number of distinct bits in PropertyAttributes.
static const int kPropertyAttributesBitsCount = 3;
static const int kPropertyAttributesCombinationsCount =
1 << kPropertyAttributesBitsCount;
enum PropertyFilter {
ALL_PROPERTIES = 0,
ONLY_WRITABLE = 1,
@ -63,6 +69,11 @@ STATIC_ASSERT(SKIP_STRINGS ==
STATIC_ASSERT(SKIP_SYMBOLS ==
static_cast<PropertyFilter>(v8::PropertyFilter::SKIP_SYMBOLS));
// Assert that kPropertyAttributesBitsCount value matches the definition of
// ALL_ATTRIBUTES_MASK.
STATIC_ASSERT((ALL_ATTRIBUTES_MASK == (READ_ONLY | DONT_ENUM | DONT_DELETE)) ==
(kPropertyAttributesBitsCount == 3));
class Smi;
class TypeInfo;

43
src/objects/transitions.cc
View File

@ -270,6 +270,34 @@ MaybeHandle<Map> TransitionsAccessor::FindTransitionToDataProperty(
return Handle<Map>(target, isolate_);
}
void TransitionsAccessor::ForEachTransitionTo(
Name name, const ForEachTransitionCallback& callback,
DisallowGarbageCollection* no_gc) {
DCHECK(name.IsUniqueName());
switch (encoding()) {
case kPrototypeInfo:
case kUninitialized:
case kMigrationTarget:
return;
case kWeakRef: {
Map target = Map::cast(raw_transitions_->GetHeapObjectAssumeWeak());
InternalIndex descriptor = target.LastAdded();
DescriptorArray descriptors = target.instance_descriptors(kRelaxedLoad);
Name key = descriptors.GetKey(descriptor);
if (key == name) {
callback(target);
}
return;
}
case kFullTransitionArray: {
base::SharedMutexGuardIf<base::kShared> scope(
isolate_->full_transition_array_access(), concurrent_access_);
return transitions().ForEachTransitionTo(name, callback);
}
}
UNREACHABLE();
}
bool TransitionsAccessor::CanHaveMoreTransitions() {
if (map_.is_dictionary_map()) return false;
if (encoding() == kFullTransitionArray) {
@ -613,6 +641,21 @@ Map TransitionArray::SearchAndGetTarget(PropertyKind kind, Name name,
return SearchDetailsAndGetTarget(transition, kind, attributes);
}
void TransitionArray::ForEachTransitionTo(
Name name, const ForEachTransitionCallback& callback) {
int transition = SearchName(name, nullptr);
if (transition == kNotFound) return;
int nof_transitions = number_of_transitions();
DCHECK(transition < nof_transitions);
Name key = GetKey(transition);
for (; transition < nof_transitions && GetKey(transition) == key;
transition++) {
Map target = GetTarget(transition);
callback(target);
}
}
void TransitionArray::Sort() {
DisallowGarbageCollection no_gc;
// In-place insertion sort.

15
src/objects/transitions.h
View File

@ -19,6 +19,9 @@
namespace v8 {
namespace internal {
// Find all transitions with given name and calls the callback.
using ForEachTransitionCallback = std::function<void(Map)>;
// TransitionsAccessor is a helper class to encapsulate access to the various
// ways a Map can store transitions to other maps in its respective field at
// Map::kTransitionsOrPrototypeInfo.
@ -68,6 +71,14 @@ class V8_EXPORT_PRIVATE TransitionsAccessor {
return FindTransitionToDataProperty(name, kFieldOnly);
}
// Find all transitions with given name and calls the callback.
// Neither GCs nor operations requiring Isolate::full_transition_array_access
// lock are allowed inside the callback.
// If any of the GC- or lock-requiring processing is necessary, it has to be
// done outside of the callback.
void ForEachTransitionTo(Name name, const ForEachTransitionCallback& callback,
DisallowGarbageCollection* no_gc);
inline Handle<String> ExpectedTransitionKey();
inline Handle<Map> ExpectedTransitionTarget();
@ -320,6 +331,10 @@ class TransitionArray : public WeakFixedArray {
Map SearchDetailsAndGetTarget(int transition, PropertyKind kind,
PropertyAttributes attributes);
// Find all transitions with given name and calls the callback.
void ForEachTransitionTo(Name name,
const ForEachTransitionCallback& callback);
inline int number_of_transitions() const;
static bool CompactPrototypeTransitionArray(Isolate* isolate,

92
src/runtime/runtime-object.cc
View File

@ -19,6 +19,7 @@
#include "src/objects/map-updater.h"
#include "src/objects/property-descriptor-object.h"
#include "src/objects/property-descriptor.h"
#include "src/objects/property-details.h"
#include "src/objects/swiss-name-dictionary-inl.h"
#include "src/runtime/runtime-utils.h"
#include "src/runtime/runtime.h"
@ -118,6 +119,51 @@ inline void ClearField(Isolate* isolate, JSObject object, FieldIndex index) {
}
}
void GeneralizeAllTransitionsToFieldAsMutable(Isolate* isolate, Handle<Map> map,
Handle<Name> name) {
InternalIndex descriptor(map->NumberOfOwnDescriptors());
Handle<Map> target_maps[kPropertyAttributesCombinationsCount];
int target_maps_count = 0;
// Collect all outgoing field transitions.
{
DisallowGarbageCollection no_gc;
TransitionsAccessor transitions(isolate, *map, &no_gc);
transitions.ForEachTransitionTo(
*name,
[&](Map target) {
DCHECK_EQ(descriptor, target.LastAdded());
DCHECK_EQ(*name, target.GetLastDescriptorName(isolate));
PropertyDetails details = target.GetLastDescriptorDetails(isolate);
// Currently, we track constness only for fields.
if (details.kind() == kData &&
details.constness() == PropertyConstness::kConst) {
target_maps[target_maps_count++] = handle(target, isolate);
}
DCHECK_IMPLIES(details.kind() == kAccessor,
details.constness() == PropertyConstness::kConst);
},
&no_gc);
CHECK_LE(target_maps_count, kPropertyAttributesCombinationsCount);
}
for (int i = 0; i < target_maps_count; i++) {
Handle<Map> target = target_maps[i];
PropertyDetails details =
target->instance_descriptors(isolate).GetDetails(descriptor);
Handle<FieldType> field_type(
target->instance_descriptors(isolate).GetFieldType(descriptor),
isolate);
MapUpdater::GeneralizeField(isolate, target, descriptor,
PropertyConstness::kMutable,
details.representation(), field_type);
DCHECK_EQ(PropertyConstness::kMutable, target->instance_descriptors(isolate)
.GetDetails(descriptor)
.constness());
}
}
bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
Handle<Object> raw_key) {
// This implements a special case for fast property deletion: when the
@ -127,6 +173,8 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
// (1) The receiver must be a regular object and the key a unique name.
Handle<Map> receiver_map(receiver->map(), isolate);
if (receiver_map->IsSpecialReceiverMap()) return false;
DCHECK(receiver_map->IsJSObjectMap());
if (!raw_key->IsUniqueName()) return false;
Handle<Name> key = Handle<Name>::cast(raw_key);
// (2) The property to be deleted must be the last property.
@ -149,26 +197,6 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
// Preconditions successful. No more bailouts after this point.
// If the {descriptor} was "const" so far, we need to update the
// {receiver_map} here, otherwise we could get the constants wrong, i.e.
//
// o.x = 1;
// delete o.x;
// o.x = 2;
//
// could trick V8 into thinking that `o.x` is still 1 even after the second
// assignment.
if (details.constness() == PropertyConstness::kConst &&
details.location() == kField) {
Handle<FieldType> field_type(descriptors->GetFieldType(descriptor),
isolate);
MapUpdater::GeneralizeField(isolate, receiver_map, descriptor,
PropertyConstness::kMutable,
details.representation(), field_type);
DCHECK_EQ(PropertyConstness::kMutable,
descriptors->GetDetails(descriptor).constness());
}
// Zap the property to avoid keeping objects alive. Zapping is not necessary
// for properties stored in the descriptor array.
if (details.location() == kField) {
@ -216,6 +244,30 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
receiver->HeapObjectVerify(isolate);
receiver->property_array().PropertyArrayVerify(isolate);
#endif
// If the {descriptor} was "const" so far, we need to update the
// {receiver_map} here, otherwise we could get the constants wrong, i.e.
//
// o.x = 1;
// [change o.x's attributes or reconfigure property kind]
// delete o.x;
// o.x = 2;
//
// could trick V8 into thinking that `o.x` is still 1 even after the second
// assignment.
// Step 1: Migrate object to an up-to-date shape.
if (parent_map->is_deprecated()) {
JSObject::MigrateInstance(isolate, Handle<JSObject>::cast(receiver));
parent_map = handle(receiver->map(), isolate);
}
// Step 2: Mark outgoing transitions from the up-to-date version of the
// parent_map to same property name of any kind or attributes as mutable.
// Also migrate object to the up-to-date map to make the object shapes
// converge sooner.
GeneralizeAllTransitionsToFieldAsMutable(isolate, parent_map, key);
return true;
}

6
src/runtime/runtime.h
View File

@ -777,9 +777,9 @@ class Runtime : public AllStatic {
// Get the runtime intrinsic function table.
static const Function* RuntimeFunctionTable(Isolate* isolate);
V8_WARN_UNUSED_RESULT static Maybe<bool> DeleteObjectProperty(
Isolate* isolate, Handle<JSReceiver> receiver, Handle<Object> key,
LanguageMode language_mode);
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe<bool>
DeleteObjectProperty(Isolate* isolate, Handle<JSReceiver> receiver,
Handle<Object> key, LanguageMode language_mode);
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
SetObjectProperty(Isolate* isolate, Handle<Object> object, Handle<Object> key,

129
test/cctest/test-field-type-tracking.cc
View File

@ -14,8 +14,10 @@
#include "src/init/v8.h"
#include "src/objects/field-type.h"
#include "src/objects/heap-number-inl.h"
#include "src/objects/internal-index.h"
#include "src/objects/map-updater.h"
#include "src/objects/objects-inl.h"
#include "src/objects/property-details.h"
#include "src/objects/property.h"
#include "src/objects/struct-inl.h"
#include "src/objects/transitions.h"
@ -1122,16 +1124,7 @@ void TestReconfigureDataFieldAttribute_GeneralizeField(
CHECK(!map2->is_stable());
CHECK(!map2->is_deprecated());
CHECK_NE(*map2, *new_map);
// If the "source" property was const then update constness expectations for
// "source" map and ensure the deoptimization dependency was triggered.
if (to.constness == PropertyConstness::kConst) {
expectations2.SetDataField(kSplitProp, READ_ONLY,
PropertyConstness::kMutable, to.representation,
to.type);
CHECK(code_src_field_const->marked_for_deoptimization());
} else {
CHECK(!code_src_field_const->marked_for_deoptimization());
}
CHECK(!code_src_field_const->marked_for_deoptimization());
CHECK(expectations2.Check(*map2));
for (int i = kSplitProp; i < kPropCount; i++) {
@ -3045,6 +3038,122 @@ TEST(RepresentationPredicatesAreInSync) {
}
}
TEST(DeletePropertyGeneralizesConstness) {
CcTest::InitializeVM();
v8::HandleScope scope(CcTest::isolate());
Isolate* isolate = CcTest::i_isolate();
Handle<FieldType> any_type = FieldType::Any(isolate);
// Create a map with some properties.
Handle<Map> initial_map = Map::Create(isolate, kPropCount + 3);
Handle<Map> map = initial_map;
for (int i = 0; i < kPropCount; i++) {
Handle<String> name = CcTest::MakeName("prop", i);
map = Map::CopyWithField(isolate, map, name, any_type, NONE,
PropertyConstness::kConst, Representation::Smi(),
INSERT_TRANSITION)
.ToHandleChecked();
}
Handle<Map> parent_map = map;
CHECK(!map->is_deprecated());
Handle<String> name_x = CcTest::MakeString("x");
Handle<String> name_y = CcTest::MakeString("y");
map = Map::CopyWithField(isolate, parent_map, name_x, any_type, NONE,
PropertyConstness::kConst, Representation::Smi(),
INSERT_TRANSITION)
.ToHandleChecked();
// Create an object, initialize its properties and add a couple of clones.
Handle<JSObject> object1 = isolate->factory()->NewJSObjectFromMap(map);
for (int i = 0; i < kPropCount; i++) {
FieldIndex index = FieldIndex::ForDescriptor(*map, InternalIndex(i));
object1->FastPropertyAtPut(index, Smi::FromInt(i));
}
Handle<JSObject> object2 = isolate->factory()->CopyJSObject(object1);
CHECK(!map->is_deprecated());
CHECK(!parent_map->is_deprecated());
// Transition to Double must deprecate m1.
CHECK(!Representation::Smi().CanBeInPlaceChangedTo(Representation::Double()));
// Reconfigure one of the first properties to make the whole transition tree
// deprecated (including |parent_map| and |map|).
Handle<Map> new_map =
ReconfigureProperty(isolate, map, InternalIndex(0), PropertyKind::kData,
NONE, Representation::Double(), any_type);
CHECK(map->is_deprecated());
CHECK(parent_map->is_deprecated());
CHECK(!new_map->is_deprecated());
// The "x" property is still kConst.
CHECK_EQ(new_map->GetLastDescriptorDetails(isolate).constness(),
PropertyConstness::kConst);
Handle<Map> new_parent_map = Map::Update(isolate, parent_map);
CHECK(!new_parent_map->is_deprecated());
// |new_parent_map| must have exactly one outgoing transition to |new_map|.
{
TransitionsAccessor ta(isolate, new_parent_map);
CHECK_EQ(ta.NumberOfTransitions(), 1);
CHECK_EQ(ta.GetTarget(0), *new_map);
}
// Deletion of the property from |object1| must migrate it to |new_parent_map|
// which is an up-to-date version of the |parent_map|. The |new_map|'s "x"
// property should be marked as mutable.
CHECK_EQ(object1->map(isolate), *map);
CHECK(Runtime::DeleteObjectProperty(isolate, object1, name_x,
LanguageMode::kSloppy)
.ToChecked());
CHECK_EQ(object1->map(isolate), *new_parent_map);
CHECK_EQ(new_map->GetLastDescriptorDetails(isolate).constness(),
PropertyConstness::kMutable);
// Now add transitions to "x" and "y" properties from |new_parent_map|.
std::vector<Handle<Map>> transitions;
Handle<Object> value = handle(Smi::FromInt(0), isolate);
for (int i = 0; i < kPropertyAttributesCombinationsCount; i++) {
PropertyAttributes attributes = static_cast<PropertyAttributes>(i);
Handle<Map> tmp;
// Add some transitions to "x" and "y".
tmp = Map::TransitionToDataProperty(isolate, new_parent_map, name_x, value,
attributes, PropertyConstness::kConst,
StoreOrigin::kNamed);
CHECK(!tmp->map(isolate).is_dictionary_map());
transitions.push_back(tmp);
tmp = Map::TransitionToDataProperty(isolate, new_parent_map, name_y, value,
attributes, PropertyConstness::kConst,
StoreOrigin::kNamed);
CHECK(!tmp->map(isolate).is_dictionary_map());
transitions.push_back(tmp);
}
// Deletion of the property from |object2| must migrate it to |new_parent_map|
// which is an up-to-date version of the |parent_map|.
// All outgoing transitions from |new_map| that add "x" must be marked as
// mutable, transitions to other properties must remain const.
CHECK_EQ(object2->map(isolate), *map);
CHECK(Runtime::DeleteObjectProperty(isolate, object2, name_x,
LanguageMode::kSloppy)
.ToChecked());
CHECK_EQ(object2->map(isolate), *new_parent_map);
for (Handle<Map> m : transitions) {
if (m->GetLastDescriptorName(isolate) == *name_x) {
CHECK_EQ(m->GetLastDescriptorDetails(isolate).constness(),
PropertyConstness::kMutable);
} else {
CHECK_EQ(m->GetLastDescriptorDetails(isolate).constness(),
PropertyConstness::kConst);
}
}
}
} // namespace test_field_type_tracking
} // namespace compiler
} // namespace internal

4
test/mjsunit/regress/regress-crbug-1195331.js
View File

@ -27,9 +27,9 @@ assertFalse(%HasOwnConstDataProperty(o3, "b"));
Object.defineProperty(o2, "a", {
value:2, enumerable: false, configurable: true, writable: true,
});
assertFalse(%HasOwnConstDataProperty(o1, "a"));
assertTrue(%HasOwnConstDataProperty(o1, "a"));
assertFalse(%HasOwnConstDataProperty(o1, "b"));
assertFalse(%HasOwnConstDataProperty(o3, "a"));
assertTrue(%HasOwnConstDataProperty(o3, "a"));
assertFalse(%HasOwnConstDataProperty(o3, "b"));
assertFalse(%HasOwnConstDataProperty(o2, "a"));