Commit 4d39e342 authored by Georg Neis's avatar Georg Neis Committed by Commit Bot

[turbofan] Don't access heap in ReduceElementAccess

This CL builds on top of feedback preprocessing. It brokerizes
all parts of element access reduction and disallows heap access there
(except for debug tracing).

To make this work without breaking tests (when concurrent inlining is
enabled):
- We don't inline functions that weren't serialized for compilation.
- We don't optimize for constant typed-array receivers when the typed
  array wasn't serialized.

This means that from now on --concurrent-inlining (and thus --future)
may result in less optimization than the default configuration.

Bug: v8:7790
Change-Id: I22685258b7d841fc9183bf99775d3f09cd272927
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1495556
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: 's avatarBenedikt Meurer <bmeurer@chromium.org>
Reviewed-by: 's avatarMaya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60061}
parent f044f91d
......@@ -8,6 +8,7 @@
#include <stdint.h>
#include "src/base/macros.h"
#include "src/base/optional.h"
#include "src/globals.h"
#include "src/pointer-with-payload.h"
......@@ -167,10 +168,20 @@ typedef PerThreadAssertScopeDebugOnly<CODE_DEPENDENCY_CHANGE_ASSERT, true>
AllowCodeDependencyChange;
class DisallowHeapAccess {
DisallowHeapAllocation no_heap_allocation_;
DisallowCodeDependencyChange no_dependency_change_;
DisallowHandleAllocation no_handle_allocation_;
DisallowHandleDereference no_handle_dereference_;
DisallowCodeDependencyChange no_dependency_change_;
DisallowHeapAllocation no_heap_allocation_;
};
class DisallowHeapAccessIf {
public:
explicit DisallowHeapAccessIf(bool condition) {
if (condition) maybe_disallow_.emplace();
}
private:
base::Optional<DisallowHeapAccess> maybe_disallow_;
};
// Per-isolate assert scopes.
......
......@@ -249,8 +249,9 @@ bool AccessInfoFactory::ComputeElementAccessInfo(
Handle<Map> map, AccessMode access_mode,
ElementAccessInfo* access_info) const {
// Check if it is safe to inline element access for the {map}.
if (!CanInlineElementAccess(map)) return false;
ElementsKind const elements_kind = map->elements_kind();
MapRef map_ref(broker(), map);
if (!CanInlineElementAccess(map_ref)) return false;
ElementsKind const elements_kind = map_ref.elements_kind();
*access_info = ElementAccessInfo(MapHandles{map}, elements_kind);
return true;
}
......@@ -259,14 +260,22 @@ bool AccessInfoFactory::ComputeElementAccessInfos(
FeedbackNexus nexus, MapHandles const& maps, AccessMode access_mode,
ZoneVector<ElementAccessInfo>* access_infos) const {
ProcessedFeedback processed(broker()->zone());
ProcessFeedbackMapsForElementAccess(isolate(), maps, &processed);
if (FLAG_concurrent_inlining) {
if (broker()->HasFeedback(nexus)) {
// TODO(neis): When concurrent inlining is ready,
// - change the printing below to not look into the heap,
// - remove the call to ProcessFeedbackMapsForElementAccess,
// - remove the Allow* scopes,
AllowCodeDependencyChange dependency_change_;
AllowHandleAllocation handle_allocation_;
AllowHandleDereference handle_dereference_;
AllowHeapAllocation heap_allocation_;
// We have already processed the feedback for this nexus during
// serialization. Use that data instead of the data computed above.
ProcessedFeedback const& preprocessed =
broker()->GetOrCreateFeedback(nexus);
// serialization. Use that data! We still process the incoming {maps} (even
// though we don't use them) so that we can print a comparison.
ProcessFeedbackMapsForElementAccess(broker(), maps, &processed);
ProcessedFeedback const& preprocessed = broker()->GetFeedback(nexus);
TRACE_BROKER(broker(),
"ComputeElementAccessInfos: using preprocessed feedback "
<< "(slot " << nexus.slot() << " of "
......@@ -278,11 +287,7 @@ bool AccessInfoFactory::ComputeElementAccessInfos(
processed.receiver_maps = preprocessed.receiver_maps;
processed.transitions = preprocessed.transitions;
} else {
TRACE_BROKER(broker(),
"ComputeElementAccessInfos: missing preprocessed feedback "
<< "(slot " << nexus.slot() << " of "
<< Brief(*nexus.vector_handle()) << ").\n");
}
ProcessFeedbackMapsForElementAccess(broker(), maps, &processed);
}
if (processed.receiver_maps.empty()) return false;
......@@ -308,7 +313,7 @@ bool AccessInfoFactory::ComputeElementAccessInfos(
// Collect the possible transitions for the {receiver_map}.
for (auto transition : processed.transitions) {
if (transition.second.is_identical_to(receiver_map)) {
if (transition.second.equals(receiver_map)) {
access_info.AddTransitionSource(transition.first);
}
}
......@@ -619,36 +624,22 @@ Maybe<ElementsKind> GeneralizeElementsKind(ElementsKind this_kind,
bool AccessInfoFactory::ConsolidateElementLoad(
ProcessedFeedback const& processed, ElementAccessInfo* access_info) const {
CHECK(!processed.receiver_maps.empty());
// We want to look at each map but the maps are split across
// {processed.receiver_maps} and {processed.transitions}.
InstanceType instance_type = processed.receiver_maps.front()->instance_type();
ElementsKind elements_kind = processed.receiver_maps.front()->elements_kind();
auto processMap = [&](Handle<Map> map) {
if (!CanInlineElementAccess(map) || map->instance_type() != instance_type) {
ProcessedFeedback::MapIterator it = processed.all_maps(broker());
MapRef first_map = it.current();
InstanceType instance_type = first_map.instance_type();
ElementsKind elements_kind = first_map.elements_kind();
MapHandles maps;
for (; !it.done(); it.advance()) {
MapRef map = it.current();
if (map.instance_type() != instance_type || !CanInlineElementAccess(map)) {
return false;
}
if (!GeneralizeElementsKind(elements_kind, map->elements_kind())
if (!GeneralizeElementsKind(elements_kind, map.elements_kind())
.To(&elements_kind)) {
return false;
}
return true;
};
for (Handle<Map> map : processed.receiver_maps) {
if (!processMap(map)) return false;
}
MapHandles maps(processed.receiver_maps.begin(),
processed.receiver_maps.end());
for (auto& pair : processed.transitions) {
if (!processMap(pair.first) || !processMap(pair.second)) return false;
maps.push_back(pair.first);
maps.push_back(pair.second);
maps.push_back(map.object());
}
// {maps} may now contain duplicate entries, but that shouldn't matter.
*access_info = ElementAccessInfo(maps, elements_kind);
return true;
......
This diff is collapsed.
......@@ -439,7 +439,9 @@ class MapRef : public HeapObjectRef {
bool IsPrimitiveMap() const;
bool is_undetectable() const;
bool is_callable() const;
bool has_indexed_interceptor() const;
bool has_hidden_prototype() const;
bool is_migration_target() const;
bool supports_fast_array_iteration() const;
bool supports_fast_array_resize() const;
......@@ -448,13 +450,17 @@ class MapRef : public HeapObjectRef {
#undef DEF_TESTER
ObjectRef GetConstructor() const;
OddballType oddball_type() const;
base::Optional<MapRef> AsElementsKind(ElementsKind kind) const;
void SerializePrototype();
ObjectRef prototype() const;
OddballType oddball_type() const;
void SerializeForElementLoad();
base::Optional<MapRef> AsElementsKind(ElementsKind kind) const;
void SerializeForElementStore();
bool HasOnlyStablePrototypesWithFastElements(
ZoneVector<MapRef>* prototype_maps);
// Concerning the underlying instance_descriptors:
void SerializeOwnDescriptors();
......@@ -573,6 +579,7 @@ class JSTypedArrayRef : public JSObjectRef {
void* elements_external_pointer() const;
void Serialize();
bool serialized() const;
HeapObjectRef buffer() const;
};
......@@ -617,11 +624,32 @@ class InternalizedStringRef : public StringRef {
};
struct ProcessedFeedback {
explicit ProcessedFeedback(Zone* zone);
// No transition sources appear in {receiver_maps}.
// All transition targets appear in {receiver_maps}.
ZoneVector<Handle<Map>> receiver_maps;
ZoneVector<std::pair<Handle<Map>, Handle<Map>>> transitions;
explicit ProcessedFeedback(Zone* zone)
: receiver_maps(zone), transitions(zone) {}
class MapIterator {
public:
bool done() const;
void advance();
MapRef current() const;
private:
friend struct ProcessedFeedback;
explicit MapIterator(ProcessedFeedback const& processed,
JSHeapBroker* broker);
ProcessedFeedback const& processed_;
JSHeapBroker* const broker_;
size_t index_ = 0;
};
// Iterator over all maps: first {receiver_maps}, then transition sources.
MapIterator all_maps(JSHeapBroker* broker) const;
};
class V8_EXPORT_PRIVATE JSHeapBroker : public NON_EXPORTED_BASE(ZoneObject) {
......@@ -654,8 +682,9 @@ class V8_EXPORT_PRIVATE JSHeapBroker : public NON_EXPORTED_BASE(ZoneObject) {
// %ObjectPrototype%.
bool IsArrayOrObjectPrototype(const JSObjectRef& object) const;
ProcessedFeedback& CreateEmptyFeedback(FeedbackNexus const& nexus);
bool HasFeedback(FeedbackNexus const& nexus) const;
ProcessedFeedback& GetOrCreateFeedback(FeedbackNexus const& nexus);
ProcessedFeedback& GetFeedback(FeedbackNexus const& nexus);
std::ostream& Trace();
void IncrementTracingIndentation();
......@@ -671,7 +700,7 @@ class V8_EXPORT_PRIVATE JSHeapBroker : public NON_EXPORTED_BASE(ZoneObject) {
struct FeedbackNexusHash {
size_t operator()(FeedbackNexus const& nexus) const {
return base::hash_combine(nexus.vector_handle().location(), nexus.slot());
return base::hash_combine(nexus.vector_handle().address(), nexus.slot());
}
};
struct FeedbackNexusEqual {
......@@ -714,8 +743,8 @@ Reduction NoChangeBecauseOfMissingData(JSHeapBroker* broker,
// Miscellaneous definitions that should be moved elsewhere once concurrent
// compilation is finished.
bool CanInlineElementAccess(Handle<Map> map);
void ProcessFeedbackMapsForElementAccess(Isolate* isolate,
bool CanInlineElementAccess(MapRef const& map);
void ProcessFeedbackMapsForElementAccess(JSHeapBroker* broker,
MapHandles const& maps,
ProcessedFeedback* processed);
......
......@@ -453,10 +453,6 @@ Reduction JSInliner::ReduceJSCall(Node* node) {
SharedFunctionInfo::EnsureSourcePositionsAvailable(isolate(), shared_info);
}
// ----------------------------------------------------------------
// After this point, we've made a decision to inline this function.
// We shall not bailout from inlining if we got here.
TRACE("Inlining %s into %s%s\n", shared_info->DebugName()->ToCString().get(),
info_->shared_info()->DebugName()->ToCString().get(),
(exception_target != nullptr) ? " (inside try-block)" : "");
......@@ -470,13 +466,17 @@ Reduction JSInliner::ReduceJSCall(Node* node) {
SharedFunctionInfoRef sfi(broker(), shared_info);
FeedbackVectorRef feedback(broker(), feedback_vector);
if (!sfi.IsSerializedForCompilation(feedback)) {
TRACE_BROKER(broker(),
"Would have missed opportunity to inline a function ("
TRACE_BROKER(broker(), "Missed opportunity to inline a function ("
<< Brief(*sfi.object()) << " with "
<< Brief(*feedback.object()) << ")");
return NoChange();
}
}
// ----------------------------------------------------------------
// After this point, we've made a decision to inline this function.
// We shall not bailout from inlining if we got here.
Handle<BytecodeArray> bytecode_array =
handle(shared_info->GetBytecodeArray(), isolate());
......
......@@ -32,28 +32,31 @@ SimplifiedOperatorBuilder* PropertyAccessBuilder::simplified() const {
return jsgraph()->simplified();
}
bool HasOnlyStringMaps(MapHandles const& maps) {
bool HasOnlyStringMaps(JSHeapBroker* broker, MapHandles const& maps) {
for (auto map : maps) {
if (!map->IsStringMap()) return false;
MapRef map_ref(broker, map);
if (!map_ref.IsStringMap()) return false;
}
return true;
}
namespace {
bool HasOnlyNumberMaps(MapHandles const& maps) {
bool HasOnlyNumberMaps(JSHeapBroker* broker, MapHandles const& maps) {
for (auto map : maps) {
if (map->instance_type() != HEAP_NUMBER_TYPE) return false;
MapRef map_ref(broker, map);
if (map_ref.instance_type() != HEAP_NUMBER_TYPE) return false;
}
return true;
}
} // namespace
bool PropertyAccessBuilder::TryBuildStringCheck(MapHandles const& maps,
bool PropertyAccessBuilder::TryBuildStringCheck(JSHeapBroker* broker,
MapHandles const& maps,
Node** receiver, Node** effect,
Node* control) {
if (HasOnlyStringMaps(maps)) {
if (HasOnlyStringMaps(broker, maps)) {
// Monormorphic string access (ignoring the fact that there are multiple
// String maps).
*receiver = *effect =
......@@ -64,10 +67,11 @@ bool PropertyAccessBuilder::TryBuildStringCheck(MapHandles const& maps,
return false;
}
bool PropertyAccessBuilder::TryBuildNumberCheck(MapHandles const& maps,
bool PropertyAccessBuilder::TryBuildNumberCheck(JSHeapBroker* broker,
MapHandles const& maps,
Node** receiver, Node** effect,
Node* control) {
if (HasOnlyNumberMaps(maps)) {
if (HasOnlyNumberMaps(broker, maps)) {
// Monomorphic number access (we also deal with Smis here).
*receiver = *effect =
graph()->NewNode(simplified()->CheckNumber(VectorSlotPair()), *receiver,
......@@ -139,16 +143,16 @@ Node* PropertyAccessBuilder::BuildCheckHeapObject(Node* receiver, Node** effect,
return receiver;
}
void PropertyAccessBuilder::BuildCheckMaps(
Node* receiver, Node** effect, Node* control,
std::vector<Handle<Map>> const& receiver_maps) {
void PropertyAccessBuilder::BuildCheckMaps(Node* receiver, Node** effect,
Node* control,
MapHandles const& receiver_maps) {
HeapObjectMatcher m(receiver);
if (m.HasValue()) {
Handle<Map> receiver_map(m.Value()->map(), isolate());
if (receiver_map->is_stable()) {
MapRef receiver_map = m.Ref(broker()).AsHeapObject().map();
if (receiver_map.is_stable()) {
for (Handle<Map> map : receiver_maps) {
if (map.is_identical_to(receiver_map)) {
dependencies()->DependOnStableMap(MapRef(broker(), receiver_map));
if (MapRef(broker(), map).equals(receiver_map)) {
dependencies()->DependOnStableMap(receiver_map);
return;
}
}
......@@ -157,8 +161,9 @@ void PropertyAccessBuilder::BuildCheckMaps(
ZoneHandleSet<Map> maps;
CheckMapsFlags flags = CheckMapsFlag::kNone;
for (Handle<Map> map : receiver_maps) {
maps.insert(map, graph()->zone());
if (map->is_migration_target()) {
MapRef receiver_map(broker(), map);
maps.insert(receiver_map.object(), graph()->zone());
if (receiver_map.is_migration_target()) {
flags |= CheckMapsFlag::kTryMigrateInstance;
}
}
......
......@@ -32,11 +32,11 @@ class PropertyAccessBuilder {
// Builds the appropriate string check if the maps are only string
// maps.
bool TryBuildStringCheck(MapHandles const& maps, Node** receiver,
Node** effect, Node* control);
bool TryBuildStringCheck(JSHeapBroker* broker, MapHandles const& maps,
Node** receiver, Node** effect, Node* control);
// Builds a number check if all maps are number maps.
bool TryBuildNumberCheck(MapHandles const& maps, Node** receiver,
Node** effect, Node* control);
bool TryBuildNumberCheck(JSHeapBroker* broker, MapHandles const& maps,
Node** receiver, Node** effect, Node* control);
Node* BuildCheckHeapObject(Node* receiver, Node** effect, Node* control);
void BuildCheckMaps(Node* receiver, Node** effect, Node* control,
......@@ -71,7 +71,7 @@ class PropertyAccessBuilder {
CompilationDependencies* dependencies_;
};
bool HasOnlyStringMaps(MapHandles const& maps);
bool HasOnlyStringMaps(JSHeapBroker* broker, MapHandles const& maps);
} // namespace compiler
} // namespace internal
......
......@@ -684,56 +684,79 @@ void SerializerForBackgroundCompilation::VisitConstructWithSpread(
}
void SerializerForBackgroundCompilation::ProcessFeedbackForKeyedPropertyAccess(
BytecodeArrayIterator* iterator) {
interpreter::Bytecode bytecode = iterator->current_bytecode();
DCHECK(bytecode == interpreter::Bytecode::kLdaKeyedProperty ||
bytecode == interpreter::Bytecode::kStaKeyedProperty ||
bytecode == interpreter::Bytecode::kStaInArrayLiteral);
if (environment()->function().feedback_vector.is_null()) return;
FeedbackSlot slot = iterator->GetSlotOperand(
bytecode == interpreter::Bytecode::kLdaKeyedProperty ? 1 : 2);
FeedbackSlot slot, AccessMode mode) {
if (slot.IsInvalid()) return;
if (environment()->function().feedback_vector.is_null()) return;
FeedbackNexus nexus(environment()->function().feedback_vector, slot);
if (broker()->HasFeedback(nexus)) return;
Handle<Name> name(nexus.GetName(), broker()->isolate());
CHECK_IMPLIES(nexus.GetKeyType() == ELEMENT, name->is_null());
if (!name->is_null() || nexus.GetKeyType() == PROPERTY) {
CHECK_NE(bytecode, interpreter::Bytecode::kStaInArrayLiteral);
if (nexus.ic_state() == MEGAMORPHIC) return;
if (nexus.GetKeyType() == PROPERTY) {
CHECK_NE(mode, AccessMode::kStoreInLiteral);
return; // TODO(neis): Support named access.
}
if (nexus.ic_state() == MEGAMORPHIC) {
return;
}
DCHECK_EQ(nexus.GetKeyType(), ELEMENT);
CHECK(nexus.GetName().is_null());
ProcessedFeedback& processed = broker()->GetOrCreateFeedback(nexus);
MapHandles maps;
nexus.ExtractMaps(&maps);
ProcessFeedbackMapsForElementAccess(broker()->isolate(), maps, &processed);
// TODO(neis): Have something like MapRef::SerializeForElementStore() and call
// it for every receiver map in case of an element store.
ProcessedFeedback& processed = broker()->CreateEmptyFeedback(nexus);
ProcessFeedbackMapsForElementAccess(broker(), maps, &processed);
for (ProcessedFeedback::MapIterator it = processed.all_maps(broker());
!it.done(); it.advance()) {
switch (mode) {
case AccessMode::kHas:
case AccessMode::kLoad:
it.current().SerializeForElementLoad();
break;
case AccessMode::kStore:
it.current().SerializeForElementStore();
break;
case AccessMode::kStoreInLiteral:
// This operation is fairly local and simple, nothing to serialize.
break;
}
}
}
void SerializerForBackgroundCompilation::VisitLdaKeyedProperty(
BytecodeArrayIterator* iterator) {
FeedbackSlot slot = iterator->GetSlotOperand(1);
ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kLoad);
environment()->accumulator_hints().Clear();
}
void SerializerForBackgroundCompilation::VisitTestIn(
BytecodeArrayIterator* iterator) {
FeedbackSlot slot = iterator->GetSlotOperand(1);
ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kHas);
environment()->accumulator_hints().Clear();
ProcessFeedbackForKeyedPropertyAccess(iterator);
}
void SerializerForBackgroundCompilation::VisitStaKeyedProperty(
BytecodeArrayIterator* iterator) {
const Hints& receiver =
environment()->register_hints(iterator->GetRegisterOperand(0));
FeedbackSlot slot = iterator->GetSlotOperand(2);
ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kStore);
// Process hints about constants.
for (Handle<Object> object : receiver.constants()) {
if (object->IsJSTypedArray()) JSTypedArrayRef(broker(), object).Serialize();
}
environment()->accumulator_hints().Clear();
ProcessFeedbackForKeyedPropertyAccess(iterator);
}
void SerializerForBackgroundCompilation::VisitStaInArrayLiteral(
BytecodeArrayIterator* iterator) {
FeedbackSlot slot = iterator->GetSlotOperand(2);
ProcessFeedbackForKeyedPropertyAccess(slot, AccessMode::kStoreInLiteral);
environment()->accumulator_hints().Clear();
ProcessFeedbackForKeyedPropertyAccess(iterator);
}
#define DEFINE_CLEAR_ENVIRONMENT(name, ...) \
......
......@@ -6,6 +6,7 @@
#define V8_COMPILER_SERIALIZER_FOR_BACKGROUND_COMPILATION_H_
#include "src/base/optional.h"
#include "src/compiler/access-info.h"
#include "src/handles.h"
#include "src/maybe-handles.h"
#include "src/utils.h"
......@@ -95,7 +96,6 @@ namespace compiler {
V(TestGreaterThanOrEqual) \
V(TestReferenceEqual) \
V(TestInstanceOf) \
V(TestIn) \
V(TestUndetectable) \
V(TestNull) \
V(TestUndefined) \
......@@ -134,6 +134,7 @@ namespace compiler {
V(StaInArrayLiteral) \
V(StaKeyedProperty) \
V(Star) \
V(TestIn) \
V(Wide) \
CLEAR_ENVIRONMENT_LIST(V) \
CLEAR_ACCUMULATOR_LIST(V) \
......@@ -244,8 +245,8 @@ class SerializerForBackgroundCompilation {
base::Optional<Hints> new_target,
const HintsVector& arguments, bool with_spread);
void ProcessFeedbackForKeyedPropertyAccess(
interpreter::BytecodeArrayIterator* iterator);
void ProcessFeedbackForKeyedPropertyAccess(FeedbackSlot slot,
AccessMode mode);
JSHeapBroker* broker() const { return broker_; }
Zone* zone() const { return zone_; }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment