From a2bb7301c5795e6b28089a8dc96e6ab5ca798e22 Mon Sep 17 00:00:00 2001 From: Teagan Strickland Date: Tue, 1 Oct 2019 09:12:22 +0000 Subject: [PATCH] [vm/compiler] Lift PC offsets out of StackMaps. Instead of storing the PC offset inside the StackMap object, store it instead in the `Array` containing `StackMap`s in `Code` objects. That is, the `Array` provided by `Code::stackmaps()` no longer contains just `StackMap` objects, but instead contains `Smi`s and `StackMap` objects in alternating fashion. Each `Smi` is the PC offset for the `StackMap` object that follows. This ends up changing very little code outside of `StackMap`, `Code::GetStackMap`, and `StackMapTableBuilder`, as there are only two types of `StackMap` users: * Users that call `Code::GetStackMap` already have the PC offset. * Users that call Code::stackmaps() can just fetch the PC offset from the returned `Array` instead. On 64-bit architectures, we will use more space to represent the PC offset as a Smi in the Array than the old uint32_t field. However, the drop in total number of StackMap objects due to an increased ability to canonicalize them should offset this. On 32-bit architectures, we can only represent 30 bit PC offsets, not 32 bit PC offsets, but that shouldn't be a problem in practice except for pathological cases. _Numbers from building the Flutter gallery in android_release mode_ Since PC offsets are no longer in the `StackMap` objects, this enables a lot more canonicalization than before. Previously, we generated 49379 `StackMap`s, but now we only generate 16139 `StackMap`s, just under a third of the original number. This is because there were a lot of `StackMap`s that differed only in their PC offset, and now they can be canonicalized into the same `StackMap` object. When building the Flutter gallery with android_release, the app.so size drops from 11276896 bytes to 10908256 bytes, a difference of 368640 bytes, or 3.27%. Using the AOT snapshot profiling support, we see the following drops: Heap snapshot size drops from 10.7 MB to 10.4 MB. `Code` | Before | After | Difference --------------------------------------------------------- Shallow size | 352363 | 352363 | 0 Retained size | 9114801 | 8796064 | -318737 Percent of snapshot | 81.2% | 80.7% | -0.5% `(RO)StackMap` | Before | After | Difference --------------------------------------------------------- Shallow size | 49381 | 16141 | -33240 Retained size | 893965 | 286613 | -607352 Percent of snapshot | 7.97% | 2.63% | -5.34% `StackMap` | Before | After | Difference --------------------------------------------------------- Shallow size | 844584 | 270472 | -574112 Retained size | 844584 | 270472 | -574112 Percent of snapshot | 7.53% | 2.48% | -5.05% As we'd expect from the `StackMap` numbers above, we end up using a little under a third of the space for `StackMap`s. We actually use even less space (32.0% of the original) than the drop in `StackMap` numbers (32.7%), because each `RawStackMap` instance is 32 bits smaller due to dropping the `pc_offset_` field. Note that even though these PC offsets now show up in the `Code::stackmaps()` `Array`, we can see this is still a net drop in space used by looking at the retained size of `Code` objects. Bug: https://github.com/dart-lang/sdk/issues/35274 Change-Id: I0910a43e7a5a7e2e721676209196be1884c5a71c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119147 Commit-Queue: Teagan Strickland Reviewed-by: Ryan Macnak --- runtime/vm/bitmap_test.cc | 4 +- runtime/vm/code_descriptors.cc | 27 +++--- runtime/vm/code_descriptors.h | 7 +- runtime/vm/compiler/assembler/disassembler.cc | 8 +- runtime/vm/image_snapshot.cc | 3 +- runtime/vm/object.cc | 90 +++++-------------- runtime/vm/object.h | 14 +-- runtime/vm/program_visitor.cc | 8 +- runtime/vm/raw_object.h | 6 +- 9 files changed, 62 insertions(+), 105 deletions(-) diff --git a/runtime/vm/bitmap_test.cc b/runtime/vm/bitmap_test.cc index b48a2047dbf..a9a1addc683 100644 --- a/runtime/vm/bitmap_test.cc +++ b/runtime/vm/bitmap_test.cc @@ -37,7 +37,7 @@ ISOLATE_UNIT_TEST_CASE(BitmapBuilder) { value = !value; } // Create a StackMap object from the builder and verify its contents. - const StackMap& stackmap1 = StackMap::Handle(StackMap::New(0, builder1, 0)); + const StackMap& stackmap1 = StackMap::Handle(StackMap::New(builder1, 0)); EXPECT_EQ(1024, stackmap1.Length()); OS::PrintErr("%s\n", stackmap1.ToCString()); value = true; @@ -62,7 +62,7 @@ ISOLATE_UNIT_TEST_CASE(BitmapBuilder) { for (int32_t i = 1025; i <= 2048; i++) { EXPECT(!builder1->Get(i)); } - const StackMap& stackmap2 = StackMap::Handle(StackMap::New(0, builder1, 0)); + const StackMap& stackmap2 = StackMap::Handle(StackMap::New(builder1, 0)); EXPECT_EQ(2049, stackmap2.Length()); for (int32_t i = 0; i <= 256; i++) { EXPECT(!stackmap2.IsObject(i)); diff --git a/runtime/vm/code_descriptors.cc b/runtime/vm/code_descriptors.cc index 0d4bd00b5f6..fd5fd33bbf6 100644 --- a/runtime/vm/code_descriptors.cc +++ b/runtime/vm/code_descriptors.cc @@ -49,21 +49,22 @@ RawPcDescriptors* DescriptorList::FinalizePcDescriptors(uword entry_point) { void StackMapTableBuilder::AddEntry(intptr_t pc_offset, BitmapBuilder* bitmap, intptr_t register_bit_count) { - stack_map_ = StackMap::New(pc_offset, bitmap, register_bit_count); + ASSERT(Smi::IsValid(pc_offset)); + pc_offset_ = Smi::New(pc_offset); + stack_map_ = StackMap::New(bitmap, register_bit_count); + list_.Add(pc_offset_, Heap::kOld); list_.Add(stack_map_, Heap::kOld); } bool StackMapTableBuilder::Verify() { intptr_t num_entries = Length(); - StackMap& map1 = StackMap::Handle(); - StackMap& map2 = StackMap::Handle(); for (intptr_t i = 1; i < num_entries; i++) { - map1 = MapAt(i - 1); - map2 = MapAt(i); + pc_offset_ = OffsetAt(i - 1); + auto const offset1 = pc_offset_.Value(); + pc_offset_ = OffsetAt(i); + auto const offset2 = pc_offset_.Value(); // Ensure there are no duplicates and the entries are sorted. - if (map1.PcOffset() >= map2.PcOffset()) { - return false; - } + if (offset1 >= offset2) return false; } return true; } @@ -77,10 +78,14 @@ RawArray* StackMapTableBuilder::FinalizeStackMaps(const Code& code) { return Array::MakeFixedLength(list_); } +RawSmi* StackMapTableBuilder::OffsetAt(intptr_t index) const { + pc_offset_ ^= list_.At(2 * index); + return pc_offset_.raw(); +} + RawStackMap* StackMapTableBuilder::MapAt(intptr_t index) const { - StackMap& map = StackMap::Handle(); - map ^= list_.At(index); - return map.raw(); + stack_map_ ^= list_.At(2 * index + 1); + return stack_map_.raw(); } RawExceptionHandlers* ExceptionHandlerList::FinalizeExceptionHandlers( diff --git a/runtime/vm/code_descriptors.h b/runtime/vm/code_descriptors.h index 5e2a52f6b11..e77a651b74a 100644 --- a/runtime/vm/code_descriptors.h +++ b/runtime/vm/code_descriptors.h @@ -47,7 +47,8 @@ class DescriptorList : public ZoneAllocated { class StackMapTableBuilder : public ZoneAllocated { public: StackMapTableBuilder() - : stack_map_(StackMap::ZoneHandle()), + : pc_offset_(Smi::ZoneHandle()), + stack_map_(StackMap::ZoneHandle()), list_(GrowableObjectArray::ZoneHandle( GrowableObjectArray::New(Heap::kOld))) {} ~StackMapTableBuilder() {} @@ -61,9 +62,11 @@ class StackMapTableBuilder : public ZoneAllocated { RawArray* FinalizeStackMaps(const Code& code); private: - intptr_t Length() const { return list_.Length(); } + intptr_t Length() const { return list_.Length() / 2; } + RawSmi* OffsetAt(intptr_t index) const; RawStackMap* MapAt(intptr_t index) const; + Smi& pc_offset_; StackMap& stack_map_; GrowableObjectArray& list_; DISALLOW_COPY_AND_ASSIGN(StackMapTableBuilder); diff --git a/runtime/vm/compiler/assembler/disassembler.cc b/runtime/vm/compiler/assembler/disassembler.cc index b7aa53dc2a2..64cd1affb1b 100644 --- a/runtime/vm/compiler/assembler/disassembler.cc +++ b/runtime/vm/compiler/assembler/disassembler.cc @@ -281,10 +281,12 @@ void Disassembler::DisassembleCodeHelper(const char* function_fullname, THR_Print("StackMaps for function '%s' {\n", function_fullname); if (code.stackmaps() != Array::null()) { const Array& stackmap_table = Array::Handle(zone, code.stackmaps()); + auto& offset = Smi::Handle(zone); StackMap& map = StackMap::Handle(zone); - for (intptr_t i = 0; i < stackmap_table.Length(); ++i) { - map ^= stackmap_table.At(i); - THR_Print("%s\n", map.ToCString()); + for (intptr_t i = 0; i < stackmap_table.Length(); i += 2) { + offset ^= stackmap_table.At(i); + map ^= stackmap_table.At(i + 1); + THR_Print("0x%08" Px ": %s\n", offset.Value(), map.ToCString()); } } THR_Print("}\n"); diff --git a/runtime/vm/image_snapshot.cc b/runtime/vm/image_snapshot.cc index a926493316a..2649ca0353d 100644 --- a/runtime/vm/image_snapshot.cc +++ b/runtime/vm/image_snapshot.cc @@ -189,7 +189,7 @@ static intptr_t StackMapSizeInSnapshot(intptr_t len_in_bits) { const intptr_t len_in_bytes = Utils::RoundUp(len_in_bits, kBitsPerByte) / kBitsPerByte; const intptr_t unrounded_size_in_bytes = - 3 * compiler::target::kWordSize + len_in_bytes; + 2 * compiler::target::kWordSize + len_in_bytes; return Utils::RoundUp(unrounded_size_in_bytes, compiler::target::ObjectAlignment::kObjectAlignment); } @@ -443,7 +443,6 @@ void ImageWriter::WriteROData(WriteStream* stream) { marked_tags = RawObject::SizeTag::update(size_in_bytes * 2, marked_tags); stream->WriteTargetWord(marked_tags); - stream->WriteFixed(map.PcOffset()); stream->WriteFixed(map.Length()); stream->WriteFixed(map.SlowPathBitCount()); stream->WriteBytes(map.raw()->ptr()->data(), len_in_bytes); diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index e06b85aa356..e5cdec34670 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -12831,9 +12831,7 @@ void StackMap::SetBit(intptr_t bit_index, bool value) const { } } -RawStackMap* StackMap::New(intptr_t pc_offset, - BitmapBuilder* bmap, - intptr_t slow_path_bit_count) { +RawStackMap* StackMap::New(BitmapBuilder* bmap, intptr_t slow_path_bit_count) { ASSERT(Object::stackmap_class() != Class::null()); ASSERT(bmap != NULL); StackMap& result = StackMap::Handle(); @@ -12860,8 +12858,6 @@ RawStackMap* StackMap::New(intptr_t pc_offset, result ^= raw; result.SetLength(length); } - ASSERT(pc_offset >= 0); - result.SetPcOffset(pc_offset); if (payload_size > 0) { // Ensure leftover bits are deterministic. result.raw()->ptr()->data()[payload_size - 1] = 0; @@ -12873,64 +12869,23 @@ RawStackMap* StackMap::New(intptr_t pc_offset, return result.raw(); } -RawStackMap* StackMap::New(intptr_t length, - intptr_t slow_path_bit_count, - intptr_t pc_offset) { - ASSERT(Object::stackmap_class() != Class::null()); - StackMap& result = StackMap::Handle(); - // Guard against integer overflow of the instance size computation. - intptr_t payload_size = Utils::RoundUp(length, kBitsPerByte) / kBitsPerByte; - if ((length < 0) || (length > kMaxUint16) || - (payload_size > kMaxLengthInBytes)) { - // This should be caught before we reach here. - FATAL1("Fatal error in StackMap::New: invalid length %" Pd "\n", length); - } - if ((slow_path_bit_count < 0) || (slow_path_bit_count > kMaxUint16)) { - // This should be caught before we reach here. - FATAL1("Fatal error in StackMap::New: invalid slow_path_bit_count %" Pd - "\n", - slow_path_bit_count); - } - - { - // StackMap data objects are associated with a code object, allocate them - // in old generation. - RawObject* raw = Object::Allocate( - StackMap::kClassId, StackMap::InstanceSize(length), Heap::kOld); - NoSafepointScope no_safepoint; - result ^= raw; - result.SetLength(length); - } - ASSERT(pc_offset >= 0); - result.SetPcOffset(pc_offset); - result.SetSlowPathBitCount(slow_path_bit_count); - return result.raw(); -} - const char* StackMap::ToCString() const { -#define FORMAT "%#05x: " - if (IsNull()) { - return "{null}"; - } else { - intptr_t fixed_length = Utils::SNPrint(NULL, 0, FORMAT, PcOffset()) + 1; - Thread* thread = Thread::Current(); - // Guard against integer overflow in the computation of alloc_size. - // - // TODO(kmillikin): We could just truncate the string if someone - // tries to print a 2 billion plus entry stackmap. - if (Length() > (kIntptrMax - fixed_length)) { - FATAL1("Length() is unexpectedly large (%" Pd ")", Length()); - } - intptr_t alloc_size = fixed_length + Length(); - char* chars = thread->zone()->Alloc(alloc_size); - intptr_t index = Utils::SNPrint(chars, alloc_size, FORMAT, PcOffset()); - for (intptr_t i = 0; i < Length(); i++) { - chars[index++] = IsObject(i) ? '1' : '0'; - } - chars[index] = '\0'; - return chars; + if (IsNull()) return "{null}"; + // Guard against integer overflow in the computation of alloc_size. + // + // TODO(kmillikin): We could just truncate the string if someone + // tries to print a 2 billion plus entry stackmap. + if (Length() > kIntptrMax) { + FATAL1("Length() is unexpectedly large (%" Pd ")", Length()); } -#undef FORMAT + Thread* thread = Thread::Current(); + intptr_t alloc_size = Length() + 1; + char* chars = thread->zone()->Alloc(alloc_size); + for (intptr_t i = 0; i < Length(); i++) { + chars[i] = IsObject(i) ? '1' : '0'; + } + chars[alloc_size - 1] = '\0'; + return chars; } RawString* LocalVarDescriptors::GetName(intptr_t var_index) const { @@ -15178,12 +15133,15 @@ RawStackMap* Code::GetStackMap(uint32_t pc_offset, // frame slots which are marked as having objects. *maps = stackmaps(); *map = StackMap::null(); - for (intptr_t i = 0; i < maps->Length(); i++) { - *map ^= maps->At(i); + for (intptr_t i = 0; i < maps->Length(); i += 2) { + // The reinterpret_cast from Smi::RawCast is inlined here because in + // debug mode, it creates Handles due to the ASSERT. + const uint32_t offset = + ValueFromRawSmi(reinterpret_cast(maps->At(i))); + if (offset != pc_offset) continue; + *map ^= maps->At(i + 1); ASSERT(!map->IsNull()); - if (map->PcOffset() == pc_offset) { - return map->raw(); // We found a stack map for this frame. - } + return map->raw(); } // If we are missing a stack map, this must either be unoptimized code, or // the entry to an osr function. (In which case all stack slots are diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 81bc5956259..c9daae0bb17 100644 --- a/runtime/vm/object.h +++ b/runtime/vm/object.h @@ -5095,12 +5095,6 @@ class StackMap : public Object { intptr_t Length() const { return raw_ptr()->length_; } - uint32_t PcOffset() const { return raw_ptr()->pc_offset_; } - void SetPcOffset(uint32_t value) const { - ASSERT(value <= kMaxUint32); - StoreNonPointer(&raw_ptr()->pc_offset_, value); - } - intptr_t SlowPathBitCount() const { return raw_ptr()->slow_path_bit_count_; } void SetSlowPathBitCount(intptr_t bit_count) const { ASSERT(bit_count <= kMaxUint16); @@ -5132,13 +5126,7 @@ class StackMap : public Object { static intptr_t InstanceSize(intptr_t length) { return RoundedAllocationSize(UnroundedSize(length)); } - static RawStackMap* New(intptr_t pc_offset, - BitmapBuilder* bmap, - intptr_t register_bit_count); - - static RawStackMap* New(intptr_t length, - intptr_t register_bit_count, - intptr_t pc_offset); + static RawStackMap* New(BitmapBuilder* bmap, intptr_t slow_path_bit_count); private: void SetLength(intptr_t length) const { diff --git a/runtime/vm/program_visitor.cc b/runtime/vm/program_visitor.cc index 91c02679100..55d1d5621af 100644 --- a/runtime/vm/program_visitor.cc +++ b/runtime/vm/program_visitor.cc @@ -212,7 +212,11 @@ class StackMapKeyValueTrait { static Value ValueOf(Pair kv) { return kv; } - static inline intptr_t Hashcode(Key key) { return key->PcOffset(); } + static inline intptr_t Hashcode(Key key) { + intptr_t hash = key->SlowPathBitCount(); + hash = CombineHashes(hash, key->Length()); + return FinalizeHash(hash, kBitsPerWord - 1); + } static inline bool IsKeyEqual(Pair pair, Key key) { return pair->Equals(*key); @@ -242,7 +246,7 @@ void ProgramVisitor::DedupStackMaps() { code_ = function.CurrentCode(); stackmaps_ = code_.stackmaps(); if (stackmaps_.IsNull()) return; - for (intptr_t i = 0; i < stackmaps_.Length(); i++) { + for (intptr_t i = 1; i < stackmaps_.Length(); i += 2) { stackmap_ ^= stackmaps_.At(i); stackmap_ = DedupStackMap(stackmap_); stackmaps_.SetAt(i, stackmap_); diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h index 9a33d1bb719..a3a05942cb2 100644 --- a/runtime/vm/raw_object.h +++ b/runtime/vm/raw_object.h @@ -1367,6 +1367,8 @@ class RawCode : public RawObject { RawTypedData* catch_entry_moves_maps_; RawSmi* variables_; } catch_entry_; + // The stackmaps_ array contains alternating Smi and StackMap values, where + // each Smi value is the PC offset for the following StackMap value. RawArray* stackmaps_; RawArray* inlined_id_to_function_; RawCodeSourceMap* code_source_map_; @@ -1620,10 +1622,6 @@ class RawStackMap : public RawObject { RAW_HEAP_OBJECT_IMPLEMENTATION(StackMap); VISIT_NOTHING(); - // Offset from code entry point corresponding to this stack map - // representation. - uint32_t pc_offset_; - uint16_t length_; // Length of payload, in bits. uint16_t slow_path_bit_count_; // Slow path live values, included in length_. // ARM64 requires register_bit_count_ to be as large as 96.