From 9905c0b34e59270323b23fcb2565a0fb46cd17c8 Mon Sep 17 00:00:00 2001 From: Mike Stanton Date: Fri, 14 May 2021 12:53:20 +0200 Subject: [PATCH] [compiler] Mark HeapNumberRef as never serialized This CL simplifies the approach to HeapNumbers in concurrent compilation. We'll only create a HeapNumberRef for immutable HeapNumbers -- this means that we don't need to validate the read of the value with a compilation dependency check. Mutable HeapNumbers are handled differently (the value is read for constant folding, and protected with a constant field dependency). This CL includes 2 reverts: Revert "[compiler] Make HeapNumberRef background serialized" Revert "[compiler] Fix endianness issue when reading HeapNumber" Bug: v8:7790 Change-Id: I24e65583b787c214b917e96e789d711c2a7c9694 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2891576 Commit-Queue: Michael Stanton Reviewed-by: Georg Neis Cr-Commit-Position: refs/heads/master@{#74567} --- src/compiler/compilation-dependencies.cc | 21 ---------------- src/compiler/compilation-dependencies.h | 4 --- src/compiler/heap-refs.cc | 32 ++++-------------------- src/compiler/heap-refs.h | 7 +++++- src/compiler/js-heap-broker.h | 11 -------- src/compiler/pipeline.cc | 1 - src/objects/heap-number-inl.h | 14 ----------- src/objects/heap-number.h | 1 - 8 files changed, 11 insertions(+), 80 deletions(-) diff --git a/src/compiler/compilation-dependencies.cc b/src/compiler/compilation-dependencies.cc index ab475cff6a..8c73a759ff 100644 --- a/src/compiler/compilation-dependencies.cc +++ b/src/compiler/compilation-dependencies.cc @@ -227,22 +227,6 @@ class ConstantInDictionaryPrototypeChainDependency final PropertyKind kind_; }; -class HeapNumberValueDependency final : public CompilationDependency { - public: - HeapNumberValueDependency(Handle number, uint64_t value) - : number_(number), value_(value) {} - - bool IsValid() const override { return number_->value_as_bits() == value_; } - - void Install(const MaybeObjectHandle& code) const override { - SLOW_DCHECK(IsValid()); - } - - private: - Handle number_; - const uint64_t value_; -}; - class TransitionDependency final : public CompilationDependency { public: explicit TransitionDependency(const MapRef& map) : map_(map) { @@ -764,11 +748,6 @@ CompilationDependencies::DependOnInitialMapInstanceSizePrediction( return SlackTrackingPrediction(initial_map, instance_size); } -void CompilationDependencies::DependOnHeapNumberValue(Handle number, - uint64_t value) { - RecordDependency(zone_->New(number, value)); -} - CompilationDependency const* CompilationDependencies::TransitionDependencyOffTheRecord( const MapRef& target_map) const { diff --git a/src/compiler/compilation-dependencies.h b/src/compiler/compilation-dependencies.h index c3de70e92e..d2acc35261 100644 --- a/src/compiler/compilation-dependencies.h +++ b/src/compiler/compilation-dependencies.h @@ -97,10 +97,6 @@ class V8_EXPORT_PRIVATE CompilationDependencies : public ZoneObject { // Record the assumption that {site}'s {ElementsKind} doesn't change. void DependOnElementsKind(const AllocationSiteRef& site); - // HeapNumber::value() when read from the background thread may be - // invalid, and must be checked at the end of compilation. - void DependOnHeapNumberValue(Handle number, uint64_t value); - // For each given map, depend on the stability of (the maps of) all prototypes // up to (and including) the {last_prototype}. template diff --git a/src/compiler/heap-refs.cc b/src/compiler/heap-refs.cc index ba3a878eb7..40a1aa21c9 100644 --- a/src/compiler/heap-refs.cc +++ b/src/compiler/heap-refs.cc @@ -11,7 +11,6 @@ #include "src/api/api-inl.h" #include "src/ast/modules.h" #include "src/codegen/code-factory.h" -#include "src/compiler/compilation-dependencies.h" #include "src/compiler/graph-reducer.h" #include "src/compiler/js-heap-broker.h" #include "src/execution/protectors-inl.h" @@ -73,10 +72,8 @@ namespace { bool IsReadOnlyHeapObject(Object object) { DisallowGarbageCollection no_gc; - // HeapNumber is excluded because we have a uniform background - // serialization treatment for it. return (object.IsCode() && Code::cast(object).is_builtin()) || - (object.IsHeapObject() && !object.IsHeapNumber() && + (object.IsHeapObject() && ReadOnlyHeap::Contains(HeapObject::cast(object))); } @@ -757,29 +754,18 @@ class RegExpBoilerplateDescriptionData : public HeapObjectData { int flags_; }; -// for HeapNumber, we should always read from the Data object, which records -// the one time we read the value, possibly on main thread (in the case of -// bytecode serialization), or possibly on the background thread. The read of -// this value has no guarantee of being correct. Therefore, instantiation of -// a HeapNumberData must be associated with the creation of a compilation -// dependency which will check later on the main thread if the bits of the -// heap number are exactly correct. class HeapNumberData : public HeapObjectData { public: HeapNumberData(JSHeapBroker* broker, ObjectData** storage, Handle object, ObjectDataKind kind = ObjectDataKind::kSerializedHeapObject) - : HeapObjectData(broker, storage, object, kind) { - const uint64_t value_as_bits = object->value_as_bits_relaxed(); - STATIC_ASSERT(sizeof(value_) == sizeof(value_as_bits)); - value_ = *reinterpret_cast(&value_as_bits); - broker->dependencies()->DependOnHeapNumberValue(object, value_as_bits); - } + : HeapObjectData(broker, storage, object, kind), + value_(object->value()) {} double value() const { return value_; } private: - double value_; + double const value_; }; class ContextData : public HeapObjectData { @@ -3299,15 +3285,7 @@ BIMODAL_ACCESSOR_C(FeedbackVector, double, invocation_count) BIMODAL_ACCESSOR(HeapObject, Map, map) -double HeapNumberRef::value() const { - if (data_->should_access_heap()) { - // TODO(mvstanton): it would make things simpler to eliminate the - // kUnserializedHeapObject case, even for OSR compiles. - CHECK_EQ(data_->kind(), kUnserializedHeapObject); - return object()->value(); - } - return ObjectRef::data()->AsHeapNumber()->value(); -} +BIMODAL_ACCESSOR_C(HeapNumber, double, value) // These JSBoundFunction fields are immutable after initialization. Moreover, // as long as JSObjects are still serialized on the main thread, all diff --git a/src/compiler/heap-refs.h b/src/compiler/heap-refs.h index fa838a49bf..e11d05aaef 100644 --- a/src/compiler/heap-refs.h +++ b/src/compiler/heap-refs.h @@ -123,7 +123,7 @@ enum class RefSerializationKind { V(FeedbackVector, RefSerializationKind::kNeverSerialized) \ V(FixedArrayBase, RefSerializationKind::kBackgroundSerialized) \ V(FunctionTemplateInfo, RefSerializationKind::kNeverSerialized) \ - V(HeapNumber, RefSerializationKind::kBackgroundSerialized) \ + V(HeapNumber, RefSerializationKind::kNeverSerialized) \ V(JSReceiver, RefSerializationKind::kBackgroundSerialized) \ V(Map, RefSerializationKind::kBackgroundSerialized) \ V(Name, RefSerializationKind::kNeverSerialized) \ @@ -429,6 +429,11 @@ class RegExpBoilerplateDescriptionRef : public HeapObjectRef { int flags() const; }; +// HeapNumberRef is only created for immutable HeapNumbers. Mutable +// HeapNumbers (those owned by in-object or backing store fields with +// representation type Double are not exposed to the compiler through +// HeapNumberRef. Instead, we read their value, and protect that read +// with a field-constness Dependency. class HeapNumberRef : public HeapObjectRef { public: DEFINE_REF_CONSTRUCTOR(HeapNumber, HeapObjectRef) diff --git a/src/compiler/js-heap-broker.h b/src/compiler/js-heap-broker.h index e4ab14d193..d684596351 100644 --- a/src/compiler/js-heap-broker.h +++ b/src/compiler/js-heap-broker.h @@ -348,16 +348,6 @@ class V8_EXPORT_PRIVATE JSHeapBroker { RootIndexMap const& root_index_map() { return root_index_map_; } - CompilationDependencies* dependencies() const { - DCHECK_NOT_NULL(dependencies_); - return dependencies_; - } - - void set_dependencies(CompilationDependencies* dependencies) { - DCHECK_NULL(dependencies_); - dependencies_ = dependencies; - } - class MapUpdaterMutexDepthScope final { public: explicit MapUpdaterMutexDepthScope(JSHeapBroker* ptr) @@ -451,7 +441,6 @@ class V8_EXPORT_PRIVATE JSHeapBroker { Isolate* const isolate_; Zone* const zone_ = nullptr; - CompilationDependencies* dependencies_ = nullptr; base::Optional target_native_context_; RefsMap* refs_; RootIndexMap root_index_map_; diff --git a/src/compiler/pipeline.cc b/src/compiler/pipeline.cc index d480f885f8..1d42111b6d 100644 --- a/src/compiler/pipeline.cc +++ b/src/compiler/pipeline.cc @@ -183,7 +183,6 @@ class PipelineData { : nullptr; dependencies_ = info_->zone()->New(broker_, info_->zone()); - broker_->set_dependencies(dependencies_); } #if V8_ENABLE_WEBASSEMBLY diff --git a/src/objects/heap-number-inl.h b/src/objects/heap-number-inl.h index 4ac6beac81..97db52a58c 100644 --- a/src/objects/heap-number-inl.h +++ b/src/objects/heap-number-inl.h @@ -25,20 +25,6 @@ uint64_t HeapNumber::value_as_bits() const { return base::ReadUnalignedValue(field_address(kValueOffset)); } -uint64_t HeapNumber::value_as_bits_relaxed() const { -#if defined(V8_TARGET_BIG_ENDIAN) - return (static_cast(RELAXED_READ_UINT32_FIELD(*this, kValueOffset)) - << 32) | - static_cast( - RELAXED_READ_UINT32_FIELD(*this, kValueOffset + kUInt32Size)); -#else - return static_cast(RELAXED_READ_UINT32_FIELD(*this, kValueOffset)) | - (static_cast( - RELAXED_READ_UINT32_FIELD(*this, kValueOffset + kUInt32Size)) - << 32); -#endif -} - void HeapNumber::set_value_as_bits(uint64_t bits) { base::WriteUnalignedValue(field_address(kValueOffset), bits); } diff --git a/src/objects/heap-number.h b/src/objects/heap-number.h index 97c359afb3..311f1437be 100644 --- a/src/objects/heap-number.h +++ b/src/objects/heap-number.h @@ -20,7 +20,6 @@ namespace internal { class HeapNumber : public TorqueGeneratedHeapNumber { public: - inline uint64_t value_as_bits_relaxed() const; inline uint64_t value_as_bits() const; inline void set_value_as_bits(uint64_t bits);