From bcbb6d9eb492646f11391a9200af513d82386ef1 Mon Sep 17 00:00:00 2001 From: Georg Neis Date: Thu, 27 Sep 2018 11:41:34 +0200 Subject: [PATCH] [turbofan] Prepare broker for the next steps. - Add a new broker mode kRetired, in which the heap can again be accessed. - Change the way modes work. We now always start in kDisabled. If FLAG_concurrent_compiler_frontend is on, we eventually move to kSerializing, then to kSerialized, then to kRetired. - Add an ObjectDataKind to ObjectData that indicates whether the data is just a dummy (i.e. created while broker was in kDisabled mode). This also happens to fix a bug found by clusterfuzz. Bug: v8:7790, chromium:889722 Change-Id: I38833fe7ad26d2d3efb15ba560576defb82f673a Reviewed-on: https://chromium-review.googlesource.com/1245425 Commit-Queue: Georg Neis Reviewed-by: Maya Lekova Reviewed-by: Jaroslav Sevcik Cr-Commit-Position: refs/heads/master@{#56260} --- src/compiler/js-heap-broker.cc | 88 +++++++++++++++---- src/compiler/js-heap-broker.h | 13 +-- src/compiler/pipeline.cc | 11 ++- test/mjsunit/regress/regress-889722.js | 11 +++ test/unittests/compiler/graph-unittest.cc | 3 +- .../compiler/js-create-lowering-unittest.cc | 1 - 6 files changed, 96 insertions(+), 31 deletions(-) create mode 100644 test/mjsunit/regress/regress-889722.js diff --git a/src/compiler/js-heap-broker.cc b/src/compiler/js-heap-broker.cc index f8ab278697..87a5832734 100644 --- a/src/compiler/js-heap-broker.cc +++ b/src/compiler/js-heap-broker.cc @@ -25,11 +25,28 @@ HEAP_BROKER_OBJECT_LIST(FORWARD_DECL) // TODO(neis): It would be nice to share the serialized data for read-only // objects. +// There are three kinds of ObjectData values. +// +// kSmi: The underlying V8 object is a Smi and the data is an instance of the +// base class (ObjectData), i.e. it's basically just the handle. Because the +// object is a Smi, it's safe to access the handle in order to extract the +// number value, and AsSmi() does exactly that. +// +// kSerializedHeapObject: The underlying V8 object is a HeapObject and the +// data is an instance of the corresponding (most-specific) subclass, e.g. +// JSFunctionData, which provides serialized information about the object. +// +// kUnserializedHeapObject: The underlying V8 object is a HeapObject and the +// data is an instance of the base class (ObjectData), i.e. it basically +// carries no information other than the handle. +// +enum ObjectDataKind { kSmi, kSerializedHeapObject, kUnserializedHeapObject }; + class ObjectData : public ZoneObject { public: ObjectData(JSHeapBroker* broker, ObjectData** storage, Handle object, - bool is_smi) - : broker_(broker), object_(object), is_smi_(is_smi) { + ObjectDataKind kind) + : broker_(broker), object_(object), kind_(kind) { *storage = this; broker->Trace("Creating data %p for handle %" V8PRIuPTR " (", this, @@ -49,12 +66,13 @@ class ObjectData : public ZoneObject { JSHeapBroker* broker() const { return broker_; } Handle object() const { return object_; } - bool is_smi() const { return is_smi_; } + ObjectDataKind kind() const { return kind_; } + bool is_smi() const { return kind_ == kSmi; } private: JSHeapBroker* const broker_; Handle const object_; - bool const is_smi_; + ObjectDataKind const kind_; }; class HeapObjectData : public ObjectData { @@ -638,7 +656,7 @@ void AllocationSiteData::SerializeBoilerplate() { HeapObjectData::HeapObjectData(JSHeapBroker* broker, ObjectData** storage, Handle object) - : ObjectData(broker, storage, object, false), + : ObjectData(broker, storage, object, kSerializedHeapObject), boolean_value_(object->BooleanValue(broker->isolate())), // We have to use a raw cast below instead of AsMap() because of // recursion. AsMap() would call IsMap(), which accesses the @@ -1321,10 +1339,7 @@ ObjectRef ContextRef::get(int index) const { } JSHeapBroker::JSHeapBroker(Isolate* isolate, Zone* zone) - : isolate_(isolate), - zone_(zone), - refs_(zone, kInitialRefsBucketCount), - mode_(FLAG_concurrent_compiler_frontend ? kSerializing : kDisabled) { + : isolate_(isolate), zone_(zone), refs_(zone, kInitialRefsBucketCount) { Trace("Constructing heap broker.\n"); } @@ -1339,21 +1354,42 @@ void JSHeapBroker::Trace(const char* format, ...) const { } } +void JSHeapBroker::StartSerializing() { + CHECK_EQ(mode_, kDisabled); + Trace("Starting serialization.\n"); + mode_ = kSerializing; + refs_.clear(); + SetNativeContextRef(); +} + +void JSHeapBroker::StopSerializing() { + CHECK_EQ(mode_, kSerializing); + Trace("Stopping serialization.\n"); + mode_ = kSerialized; +} + +void JSHeapBroker::Retire() { + CHECK_EQ(mode_, kSerialized); + Trace("Retiring.\n"); + mode_ = kRetired; +} + bool JSHeapBroker::SerializingAllowed() const { return mode() == kSerializing || (!FLAG_strict_heap_broker && mode() == kSerialized); } -void JSHeapBroker::SerializeStandardObjects() { - if (!native_context_.has_value()) { - native_context_ = NativeContextRef(this, isolate()->native_context()); - native_context_->Serialize(); - } +void JSHeapBroker::SetNativeContextRef() { + native_context_ = NativeContextRef(this, isolate()->native_context()); +} +void JSHeapBroker::SerializeStandardObjects() { if (mode() == kDisabled) return; TraceScope tracer(this, "JSHeapBroker::SerializeStandardObjects"); + native_context().Serialize(); + Builtins* const b = isolate()->builtins(); Factory* const f = isolate()->factory(); @@ -1473,7 +1509,7 @@ ObjectData* JSHeapBroker::GetOrCreateData(Handle object) { AllowHandleAllocation handle_allocation; AllowHandleDereference handle_dereference; if (object->IsSmi()) { - new (zone()) ObjectData(this, data_storage, object, true); + new (zone()) ObjectData(this, data_storage, object, kSmi); #define CREATE_DATA_IF_MATCH(name) \ } else if (object->Is##name()) { \ new (zone()) name##Data(this, data_storage, Handle::cast(object)); @@ -2026,16 +2062,20 @@ ObjectRef::ObjectRef(JSHeapBroker* broker, Handle object) { case JSHeapBroker::kSerializing: data_ = broker->GetOrCreateData(object); break; - case JSHeapBroker::kDisabled: + case JSHeapBroker::kDisabled: { auto insertion_result = broker->refs_.insert({object.address(), nullptr}); ObjectData** data_storage = &(insertion_result.first->second); if (insertion_result.second) { AllowHandleDereference handle_dereference; new (broker->zone()) - ObjectData(broker, data_storage, object, object->IsSmi()); + ObjectData(broker, data_storage, object, + object->IsSmi() ? kSmi : kUnserializedHeapObject); } data_ = *data_storage; break; + } + case JSHeapBroker::kRetired: + UNREACHABLE(); } CHECK_NOT_NULL(data_); } @@ -2161,7 +2201,19 @@ Handle ObjectRef::object() const { return data()->object(); } JSHeapBroker* ObjectRef::broker() const { return data()->broker(); } -ObjectData* ObjectRef::data() const { return data_; } +ObjectData* ObjectRef::data() const { + switch (data_->broker()->mode()) { + case JSHeapBroker::kDisabled: + CHECK_NE(data_->kind(), kSerializedHeapObject); + return data_; + case JSHeapBroker::kSerializing: + case JSHeapBroker::kSerialized: + CHECK_NE(data_->kind(), kUnserializedHeapObject); + return data_; + case JSHeapBroker::kRetired: + return data_; + } +} Reduction NoChangeBecauseOfMissingData(JSHeapBroker* broker, const char* function, int line) { diff --git a/src/compiler/js-heap-broker.h b/src/compiler/js-heap-broker.h index 3e5717dd26..c623669e12 100644 --- a/src/compiler/js-heap-broker.h +++ b/src/compiler/js-heap-broker.h @@ -472,18 +472,18 @@ class InternalizedStringRef : public StringRef { class V8_EXPORT_PRIVATE JSHeapBroker : public NON_EXPORTED_BASE(ZoneObject) { public: JSHeapBroker(Isolate* isolate, Zone* zone); + void SetNativeContextRef(); void SerializeStandardObjects(); Isolate* isolate() const { return isolate_; } Zone* zone() const { return zone_; } NativeContextRef native_context() const { return native_context_.value(); } - enum BrokerMode { kDisabled, kSerializing, kSerialized }; + enum BrokerMode { kDisabled, kSerializing, kSerialized, kRetired }; BrokerMode mode() const { return mode_; } - void StopSerializing() { - CHECK_EQ(mode_, kSerializing); - mode_ = kSerialized; - } + void StartSerializing(); + void StopSerializing(); + void Retire(); bool SerializingAllowed() const; // Returns nullptr iff handle unknown. @@ -506,7 +506,8 @@ class V8_EXPORT_PRIVATE JSHeapBroker : public NON_EXPORTED_BASE(ZoneObject) { Zone* const zone_; base::Optional native_context_; ZoneUnorderedMap refs_; - BrokerMode mode_; + + BrokerMode mode_ = kDisabled; unsigned tracing_indentation_ = 0; static const size_t kInitialRefsBucketCount = 1000; diff --git a/src/compiler/pipeline.cc b/src/compiler/pipeline.cc index d2d1309ffb..5287bfc8fd 100644 --- a/src/compiler/pipeline.cc +++ b/src/compiler/pipeline.cc @@ -1328,8 +1328,6 @@ struct CopyMetadataForConcurrentCompilePhase { NodeVector cached_nodes(temp_zone); data->jsgraph()->GetCachedNodes(&cached_nodes); for (Node* const node : cached_nodes) graph_reducer.ReduceNode(node); - - data->js_heap_broker()->StopSerializing(); } }; @@ -2011,8 +2009,6 @@ bool PipelineImpl::CreateGraph() { data->node_origins()->AddDecorator(); } - Run(); - Run(); RunPrintAndVerify(GraphBuilderPhase::phase_name(), true); @@ -2040,8 +2036,12 @@ bool PipelineImpl::CreateGraph() { // Run the type-sensitive lowerings and optimizations on the graph. { if (FLAG_concurrent_compiler_frontend) { + data->js_heap_broker()->StartSerializing(); + Run(); Run(); + data->js_heap_broker()->StopSerializing(); } else { + data->js_heap_broker()->SetNativeContextRef(); // Type the graph and keep the Typer running such that new nodes get // automatically typed when they are created. Run(data->CreateTyper()); @@ -2571,6 +2571,9 @@ std::ostream& operator<<(std::ostream& out, const BlockStartsAsJSON& s) { MaybeHandle PipelineImpl::FinalizeCode() { PipelineData* data = this->data_; + if (data->js_heap_broker() && FLAG_concurrent_compiler_frontend) { + data->js_heap_broker()->Retire(); + } Run(); MaybeHandle maybe_code = data->code(); diff --git a/test/mjsunit/regress/regress-889722.js b/test/mjsunit/regress/regress-889722.js new file mode 100644 index 0000000000..c883dbe489 --- /dev/null +++ b/test/mjsunit/regress/regress-889722.js @@ -0,0 +1,11 @@ +// Copyright 2018 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +function getRandomProperty(v, rand) { + var properties = Object.getOwnPropertyNames(v); + return properties[rand % properties.length]; +} +r = Realm.create(); +o = Realm.eval(r, "() => { return Realm.global(-10) instanceof Object }"); +o.__p_211203344 = o[getRandomProperty(o, 211203344)]; diff --git a/test/unittests/compiler/graph-unittest.cc b/test/unittests/compiler/graph-unittest.cc index 2e27ef4a45..4736ddefa2 100644 --- a/test/unittests/compiler/graph-unittest.cc +++ b/test/unittests/compiler/graph-unittest.cc @@ -25,6 +25,7 @@ GraphTest::GraphTest(int num_parameters) node_origins_(&graph_) { graph()->SetStart(graph()->NewNode(common()->Start(num_parameters))); graph()->SetEnd(graph()->NewNode(common()->End(1), graph()->start())); + js_heap_broker()->SetNativeContextRef(); } @@ -70,8 +71,6 @@ Node* GraphTest::HeapConstant(const Handle& value) { Node* node = graph()->NewNode(common()->HeapConstant(value)); Type type = Type::NewConstant(js_heap_broker(), value, zone()); NodeProperties::SetType(node, type); - JSHeapCopyReducer heap_copy_reducer(js_heap_broker()); - heap_copy_reducer.Reduce(node); return node; } diff --git a/test/unittests/compiler/js-create-lowering-unittest.cc b/test/unittests/compiler/js-create-lowering-unittest.cc index 74ae4c8c78..eafd7fa35e 100644 --- a/test/unittests/compiler/js-create-lowering-unittest.cc +++ b/test/unittests/compiler/js-create-lowering-unittest.cc @@ -34,7 +34,6 @@ class JSCreateLoweringTest : public TypedGraphTest { javascript_(zone()), deps_(isolate(), zone()), handle_scope_(isolate()) { - js_heap_broker()->SerializeStandardObjects(); } ~JSCreateLoweringTest() override = default;