Chrome's classes should have their padding checked on 64-bit |
|||||
Issue description
Structure (struct/class) layout is important in order to conserve memory by minimizing padding. Some structs have excessive padding in 32-bit builds, but 64-bit builds are much more prone to problems due to the increased size and alignment requirements of pointer and size_t types. A classic example is the Windows.h URL_COMPONENETS which alternates DWORD and pointer types. This is perfectly packed in 32-bit but is 25% padding in 64-bit.
Padding can most easily be investigated by using recent versions of llvm-pdbdump.exe, like this:
llvm-pdbdump.exe pretty -class-definitions=layout -classes -only-padding-classes chrome.pdb
This will dump all of the types that have padding, showing their layout and some summary data. A bit of scripting can generate reports of the worst padding violations, and then this list can be searched for types that are heavily allocated.
When iterating on fixes the best workflow is to use the undocumented /d1reportSingleClassLayout option, like this:
/d1reportSingleClassLayoutIncrementalMarking
Just put a class name substring after /d1reportSingleClassLayout and compile a test file and VC++ will dump the layout of all struct/class that match the pattern. This is much faster than waiting for a full link and then dumping the PDB.
Be aware that llvm-pdbdump may show anomalously large padding blocks for some types due to complex multiple inheritance.
Finding the most allocated types is crucial for finding valuable changes to make.
Understanding structure padding is important when working on this bug. The bit-field packing rules used by VC++ are documented here:
https://randomascii.wordpress.com/2010/06/06/bit-field-packing-with-visual-c/
Some other packing anomalies are discussed here:
https://randomascii.wordpress.com/2013/12/01/vc-2013-class-layout-change-and-wasted-space/
and a search for struct padding will find many other articles.
In general the VC++ rule is that all built-in types are aligned to a multiple of their size. For member variables that are of struct/class type the alignment is (logically enough) that of the type in the child struct with the largest alignment requirement.
Don't forget that the size of a struct will (logically enough) always be a multiple of its largest alignment requirement. This is necessary so that an array of this type of struct will leave each one properly aligned. Therefore a struct that is just int64_t/char will have seven bytes of padding at the end.
Finally, the significance of savings depends on how a struct/class is used. When used as a global or local the bytes saved are, more or less, pure savings. When allocated as a single item it is necessary to consider the heap's behavior. In 32-bit processes on Windows all allocations are rounded up to a multiple of eight bytes, and in 64-bit processes on Windows all allocations are rounded up to a multiple of sixteen bytes. Therefore shrinking a struct from 36 bytes down to 32 is more valuable than shrinking it from 32 to 28.
While this bug is focused on Windows and the investigation is all cast in terms of Windows the benefits of any fixes will usually be cross-platform.
,
Apr 15 2017
,
Apr 17 2017
,
Apr 20 2017
For structs and classes that contain enums there is another strategy to consider for saving space - typed enums. enum class Foo : uint8_t; or: enum Foo : uint8_t; This lets us specify that an enum should be stored as a smaller type. This needs to be used with care if the enum is stored in a bit-field due to the crazy bit-field packing rules, but for other cases this is a straightforward way to reduce the size of enum fields, and their alignment requirements. This is an approved C++ feature per https://chromium-cpp.appspot.com/
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f80504b4bad121fb16e3e7dad008555baf44905 commit 3f80504b4bad121fb16e3e7dad008555baf44905 Author: stanisc <stanisc@chromium.org> Date: Fri Apr 21 18:12:26 2017 Reduce StyleRule class size by eliminating padding in the layout. This change improves the class size from 40 bytes to 32 byte. Considering 16-byte allocation granularity on 64-bit Windows this saves 16 bytes per instance (48 bytes to 32 bytes). When I tested this with cnn.com there were about 7.2K instances of StyleRule after loading the page (14K instances at the peak). That means this change will reduce the memory usage by ~112 KB and lower the peak memory usage by ~219 KB. Layout before the change: class blink::StyleRule [sizeof = 40] : public blink::StyleRuleBase { <padding> (4 bytes) [sizeof=8] blink::CSSSelectorList selector_list_ [sizeof=8] blink::Member<blink::StylePropertySet> properties_ [sizeof=8] blink::Member<blink::CSSLazyPropertyParser> lazy_property_parser_ [sizeof=4] blink::StyleRule::ConsiderForMatching should_consider_for_matching_rules_ <padding> (4 bytes) } Layout after the change: class blink::StyleRule [sizeof = 32] : public blink::StyleRuleBase { [sizeof=4] blink::StyleRule::ConsiderForMatching should_consider_for_matching_rules_ [sizeof=8] blink::CSSSelectorList selector_list_ [sizeof=8] blink::Member<blink::StylePropertySet> properties_ [sizeof=8] blink::Member<blink::CSSLazyPropertyParser> lazy_property_parser_ } BUG= 710933 Review-Url: https://codereview.chromium.org/2833883002 Cr-Commit-Position: refs/heads/master@{#466394} [modify] https://crrev.com/3f80504b4bad121fb16e3e7dad008555baf44905/third_party/WebKit/Source/core/css/StyleRule.cpp [modify] https://crrev.com/3f80504b4bad121fb16e3e7dad008555baf44905/third_party/WebKit/Source/core/css/StyleRule.h
,
Apr 21 2017
Nice fix! Check out base::OptionalStorage<> as well. That one looked like it has potential to be used very frequently and you can knock out quite a few bytes by moving the bool down.
,
Apr 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a9eb48c6d24472e6812811ece2bf4a29d2c3747 commit 8a9eb48c6d24472e6812811ece2bf4a29d2c3747 Author: stanisc <stanisc@chromium.org> Date: Mon Apr 24 18:51:25 2017 Optimize sizeof(InlineBox) by eliminating padding Considering that on 64-bit Windows allocations are done with 16 byte granularity this reduces the per-instance memory usage of InlineBox by 16 bytes - from 80 bytes to 64 bytes. My estimate is that on a typical page where are 1K to 2K instances of InlineBox and derived classes. And on text heavy page like Reddit topic discussion I counted 6.7K instances. That means that the estimated memory saving is anywhere from 16 KB to 100 KB per Renderer processes. Class layout before the change: class blink::InlineBox [sizeof = 72] : public blink::DisplayItemClient { <padding> (4 bytes) [sizeof=4] blink::InlineBox::InlineBoxBitfields bitfields_ <padding> (4 bytes) [sizeof=8] blink::InlineBox* next_ [sizeof=8] blink::InlineBox* prev_ [sizeof=8] blink::InlineFlowBox* parent_ [sizeof=8] blink::LineLayoutItem line_layout_item_ [sizeof=8] blink::LayoutPoint location_ [sizeof=4] blink::LayoutUnit logical_width_ <padding> (4 bytes) } class layout after the change: class blink::InlineBox [sizeof = 64] : public blink::DisplayItemClient { <padding> (4 bytes) [sizeof=8] blink::InlineBox* next_ [sizeof=8] blink::InlineBox* prev_ [sizeof=8] blink::InlineFlowBox* parent_ [sizeof=8] blink::LineLayoutItem line_layout_item_ [sizeof=8] blink::LayoutPoint location_ [sizeof=4] blink::LayoutUnit logical_width_ [sizeof=4] blink::InlineBox::InlineBoxBitfields bitfields_ } BUG= 710933 Review-Url: https://codereview.chromium.org/2829683010 Cr-Commit-Position: refs/heads/master@{#466699} [modify] https://crrev.com/8a9eb48c6d24472e6812811ece2bf4a29d2c3747/third_party/WebKit/Source/core/layout/line/InlineBox.cpp [modify] https://crrev.com/8a9eb48c6d24472e6812811ece2bf4a29d2c3747/third_party/WebKit/Source/core/layout/line/InlineBox.h
,
Apr 24 2017
It would be nice to get a padding compiler intrinsic that would let us go: static_assert(__padding(blink::InlineBox) <= 4, "Padding is too large - please fix if possible");
,
Apr 24 2017
That would be pretty tricky, because there's no sane value you could choose that wouldn't give false positives from time to time, and a static_assert would break your build if you couldn't fix it. It seems like it could be possible to have a builtin warning which would trigger if you laid out your record poorly though.
,
Apr 24 2017
I was thinking of adding it on a class-by-class basis. The only time it would break would be when somebody was changing the class (or a class that the class includes or inherits from), at which point they could either adjust the assert conditions or fix their layout. But, just brainstorming here. The benefit might be too small. It just seems a shame to carefully fix this padding and then have no way to ensure that it doesn't revert next week.
,
Apr 25 2017
Next candidate is v8::internal::compiler::Constant. Swapping fields reduces the size from 24 to 16. I have hard time figuring out the number of instances, but it looks like there may be at least a few thousands. This class is copy constructed and stored in std::map and std::vector.
Before:
class v8::internal::compiler::Constant [sizeof = 24] {
[sizeof=4] v8::internal::compiler::Constant::Type type_
<padding> (4 bytes)
[sizeof=8] __int64 value_
[sizeof=4] v8::internal::RelocInfo::Mode rmode_
<padding> (4 bytes)
}
After:
class v8::internal::compiler::Constant [sizeof = 16] {
[sizeof=4] v8::internal::compiler::Constant::Type type_
[sizeof=4] v8::internal::RelocInfo::Mode rmode_
[sizeof=8] __int64 value_
}
,
Apr 25 2017
Some more words on tools:
1. clang has -Wpadded -- you could add that to the build flags and remove -Werror, and then grep for the largest "with N bytes" messages in the build output:
thakis@thakis:~/src/llvm-build-nolibcxx$ cat test.cc
struct S {
int a;
void* v;
} s;
thakis@thakis:~/src/llvm-build-nolibcxx$ bin/clang -Wpadded -c test.cc
test.cc:3:9: warning: padding struct 'S' with 4 bytes to align 'v' [-Wpadded]
void* v;
^
2. clang also has `-Xclang -fdump-record-layouts` but that's more difficult to use (https://www.hdevalence.ca/blog/2013-12-22-better-living-through-clangistry)
Not sure if that's useful; I thought I'd mention it.
,
Apr 27 2017
For V8, I'd suggest to look at classes defined in: - v8/src/compiler/node.h - v8/src/compiler/operator.h - v8/src/compiler/instruction.h - v8/src/ast/ast.h (The various AstNode subclasses have already gone through several rounds of manual object layout tuning to minimize padding, taking subclassing into account; but maybe there's potential for further improvement.) (If you go looking for other things, please ignore everything in v8/src/full-codegen/ and v8/src/crankshaft/, as those directories are slated to go away.)
,
Apr 27 2017
@13: A fix for v8::internal::compiler::Constant is coming today and I am looking at other classes too. https://codereview.chromium.org/2841343002/
,
Apr 27 2017
Re #12: Apparently cl.exe has a /d1reportAllClassLayout switch that does roughly the same as clang's -fdump-record-layouts (seen in https://bugs.llvm.org/show_bug.cgi?id=32826)
,
Apr 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/6408032e6164f7f1b2f9edab75d1b14e4cbb32e4 commit 6408032e6164f7f1b2f9edab75d1b14e4cbb32e4 Author: stanisc <stanisc@chromium.org> Date: Fri Apr 28 20:58:53 2017 Optimized layout padding in 4 classes in ast.h This reduces sizeof of these classes by 8 bytes on 64-bit (16 bytes considering allocation size granularity for some of these classes). I don't know how many instances remain at the end of loading a page. These objects are Zone objects which makes it more difficult to count the number of instances. But looking at allocations only on cnn.com I've got 70K for BinaryOperation, 20K for CompareOperation, 1.5K for CaseClause. There aren't not many allocations of NativeFunctionLiteral but I decided to fix it too to keep the same layout pattern. Before: class v8::internal::CaseClause [sizeof = 56] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression <padding> (4 bytes) [sizeof=8] v8::internal::Expression* label_ [sizeof=8] v8::internal::Label body_target_ [sizeof=8] v8::internal::ZoneList<v8::internal::Statement *>* statements_ [sizeof=8] v8::internal::AstType* compare_type_ [sizeof=4] v8::internal::FeedbackSlot feedback_slot_ <padding> (4 bytes) } After: class v8::internal::CaseClause [sizeof = 48] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression [sizeof=4] v8::internal::FeedbackSlot feedback_slot_ [sizeof=8] v8::internal::Expression* label_ [sizeof=8] v8::internal::Label body_target_ [sizeof=8] v8::internal::ZoneList<v8::internal::Statement *>* statements_ [sizeof=8] v8::internal::AstType* compare_type_ } Before: class v8::internal::BinaryOperation [sizeof = 56] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression [sizeof=1] bool has_fixed_right_arg_ <padding> (3 bytes) [sizeof=4] int fixed_right_arg_value_ <padding> (4 bytes) [sizeof=8] v8::internal::Expression* left_ [sizeof=8] v8::internal::Expression* right_ [sizeof=8] v8::internal::Handle<v8::internal::AllocationSite> allocation_site_ [sizeof=4] v8::internal::FeedbackSlot feedback_slot_ <padding> (4 bytes) } After: class v8::internal::BinaryOperation [sizeof = 48] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression [sizeof=4] v8::internal::FeedbackSlot feedback_slot_ [sizeof=8] v8::internal::Expression* left_ [sizeof=8] v8::internal::Expression* right_ [sizeof=8] v8::internal::Handle<v8::internal::AllocationSite> allocation_site_ [sizeof=1] bool has_fixed_right_arg_ <padding> (3 bytes) [sizeof=4] int fixed_right_arg_value_ } Before: class v8::internal::CompareOperation [sizeof = 48] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression <padding> (4 bytes) [sizeof=8] v8::internal::Expression* left_ [sizeof=8] v8::internal::Expression* right_ [sizeof=8] v8::internal::AstType* combined_type_ [sizeof=4] v8::internal::FeedbackSlot feedback_slot_ <padding> (4 bytes) } After: class v8::internal::CompareOperation [sizeof = 40] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression [sizeof=4] v8::internal::FeedbackSlot feedback_slot_ [sizeof=8] v8::internal::Expression* left_ [sizeof=8] v8::internal::Expression* right_ [sizeof=8] v8::internal::AstType* combined_type_ } Before: class v8::internal::NativeFunctionLiteral [sizeof = 40] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression <padding> (4 bytes) [sizeof=8] v8::internal::AstRawString* name_ [sizeof=8] v8::Extension* extension_ [sizeof=4] v8::internal::FeedbackSlot literal_feedback_slot_ <padding> (4 bytes) } After: class v8::internal::NativeFunctionLiteral [sizeof = 32] : public v8::internal::Expression { [sizeof=12] v8::internal::Expression [sizeof=4] v8::internal::FeedbackSlot literal_feedback_slot_ [sizeof=8] v8::internal::AstRawString* name_ [sizeof=8] v8::Extension* extension_ } BUG= chromium:710933 Review-Url: https://codereview.chromium.org/2843293003 Cr-Commit-Position: refs/heads/master@{#44989} [modify] https://crrev.com/6408032e6164f7f1b2f9edab75d1b14e4cbb32e4/src/ast/ast.h
,
May 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/9ede481a8c873058f76a315a89c8803868403913 commit 9ede481a8c873058f76a315a89c8803868403913 Author: stanisc <stanisc@chromium.org> Date: Mon May 01 04:05:00 2017 Remove padding in v8::internal::compiler::Constant class. This makes the size of the class smaller by 8 bytes on 64-bit. I looked at the usage pattern. Even though it seems the number of instances doesn't get higher than a few thousand, this class is still very hot because it is constructed and passed by value a lot. So perhaps reducing the size would make passing this class by value or growing arrays more optimal and might save some cycles. Before: class v8::internal::compiler::Constant [sizeof = 24] { [sizeof=4] v8::internal::compiler::Constant::Type type_ <padding> (4 bytes) [sizeof=8] __int64 value_ [sizeof=4] v8::internal::RelocInfo::Mode rmode_ <padding> (4 bytes) } After: class v8::internal::compiler::Constant [sizeof = 16] { [sizeof=4] v8::internal::compiler::Constant::Type type_ [sizeof=4] v8::internal::RelocInfo::Mode rmode_ [sizeof=8] __int64 value_ } BUG= chromium:710933 Review-Url: https://codereview.chromium.org/2841343002 Cr-Commit-Position: refs/heads/master@{#45001} [modify] https://crrev.com/9ede481a8c873058f76a315a89c8803868403913/src/compiler/instruction.h
,
May 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4f54bee9b89dd593c338dbc684f806bb73b48256 commit 4f54bee9b89dd593c338dbc684f806bb73b48256 Author: stanisc <stanisc@chromium.org> Date: Tue May 09 17:47:56 2017 Blink class layout optimization to reduce memory usage This change optimizes class layout on 64-bit in 4 blink classes, all related to images. Considering the 16 bit allocation granularity this saves 16 bytes per instance of each class. It looks like a typical web page might have up to a few hundred instances of each class so the total memory saving ~ 16-32 KB per Renderer process, potentially more for image heavy pages. UPDATE: crrev.com/2746343002 added a new field to ImageResourceContent which further increased it size by 8 bytes. I've moved the fields around again to still make it squeeze into 80 bytes. Layout details: Before: class blink::Image [sizeof = 40] : public WTF::ThreadSafeRefCounted<blink::Image> { vfptr +0x00 [sizeof=8] base +0x08 [sizeof=4] WTF::ThreadSafeRefCounted<blink::Image> <padding> (4 bytes) data +0x10 [sizeof=8] WTF::RefPtr<blink::SharedBuffer> encoded_image_data_ data +0x18 [sizeof=8] blink::UntracedMember<blink::ImageObserver> image_observer_ data +0x20 [sizeof=1] bool image_observer_disabled_ <padding> (7 bytes) } After: class blink::Image [sizeof = 32] : public WTF::ThreadSafeRefCounted<blink::Image> { vfptr +0x00 [sizeof=8] base +0x08 [sizeof=4] WTF::ThreadSafeRefCounted<blink::Image> data +0x0c [sizeof=1] bool image_observer_disabled_ <padding> (3 bytes) data +0x10 [sizeof=8] WTF::RefPtr<blink::SharedBuffer> encoded_image_data_ data +0x18 [sizeof=8] blink::UntracedMember<blink::ImageObserver> image_observer_ } Before: class blink::BitmapImage [sizeof = 280] : public blink::Image { base +0x00 [sizeof=33] blink::Image <padding> (7 bytes) data +0x28 [sizeof=112] blink::ImageSource source_ data +0x98 [sizeof=8] blink::IntSize size_ data +0xa0 [sizeof=8] blink::IntSize size_respecting_orientation_ data +0xa8 [sizeof=8] unsigned __int64 current_frame_ data +0xb0 [sizeof=40] WTF::Vector<blink::FrameData,1,WTF::PartitionAllocator> frames_ data +0xd8 [sizeof=8] sk_sp<SkImage> cached_frame_ data +0xe0 [sizeof=8] unsigned __int64 cached_frame_index_ data +0xe8 [sizeof=8] std::unique_ptr<...> frame_timer_ data +0xf0 [sizeof=4] int repetition_count_ data +0xf4 [sizeof=4] blink::BitmapImage::RepetitionCountStatus repetition_count_status_ data +0xf8 [sizeof=4] int repetitions_complete_ <padding> (4 bytes) data +0x100 [sizeof=8] double desired_frame_start_time_ data +0x108 [sizeof=8] unsigned __int64 frame_count_ data +0x110 [sizeof=4] blink::ImageAnimationPolicy animation_policy_ data +0x114 [sizeof=1] bool animation_finished_ : 1 data +0x114 [sizeof=1] bool all_data_received_ : 1 data +0x114 [sizeof=1] bool have_size_ : 1 data +0x114 [sizeof=1] bool size_available_ : 1 data +0x114 [sizeof=1] bool have_frame_count_ : 1 <padding> (3 bytes) } After: class blink::BitmapImage [sizeof = 264] : public blink::Image { base +0x00 [sizeof=32] blink::Image data +0x20 [sizeof=112] blink::ImageSource source_ data +0x90 [sizeof=8] blink::IntSize size_ data +0x98 [sizeof=8] blink::IntSize size_respecting_orientation_ data +0xa0 [sizeof=8] unsigned __int64 current_frame_ data +0xa8 [sizeof=40] WTF::Vector<blink::FrameData,1,WTF::PartitionAllocator> frames_ data +0xd0 [sizeof=8] sk_sp<SkImage> cached_frame_ data +0xd8 [sizeof=8] unsigned __int64 cached_frame_index_ data +0xe0 [sizeof=8] std::unique_ptr<...> frame_timer_ data +0xe8 [sizeof=4] blink::ImageAnimationPolicy animation_policy_ data +0xec [sizeof=1] bool animation_finished_ : 1 data +0xec [sizeof=1] bool all_data_received_ : 1 data +0xec [sizeof=1] bool have_size_ : 1 data +0xec [sizeof=1] bool size_available_ : 1 data +0xec [sizeof=1] bool have_frame_count_ : 1 data +0xed [sizeof=1] blink::BitmapImage::RepetitionCountStatus repetition_count_status_ <padding> (2 bytes) data +0xf0 [sizeof=4] int repetition_count_ data +0xf4 [sizeof=4] int repetitions_complete_ data +0xf8 [sizeof=8] double desired_frame_start_time_ data +0x100 [sizeof=8] unsigned __int64 frame_count_ } Before: class blink::ImageResourceContent [sizeof = 96] : public blink::GarbageCollectedFinalized<blink::ImageResourceContent> , public blink::ImageObserver { base +0x00 [sizeof=8] blink::ImageObserver base +0x08 [sizeof=1] blink::GarbageCollectedFinalized<blink::ImageResourceContent> <padding> (7 bytes) data +0x10 [sizeof=8] blink::Member<blink::ImageResourceInfo> info_ data +0x18 [sizeof=4] blink::ResourceStatus content_status_ <padding> (4 bytes) data +0x20 [sizeof=8] WTF::RefPtr<blink::Image> image_ data +0x28 [sizeof=24] WTF::HashCountedSet<...> observers_ data +0x40 [sizeof=24] WTF::HashCountedSet<...> finished_observers_ data +0x58 [sizeof=4] blink::Image::SizeAvailability size_available_ data +0x5C [sizeof=1] bool is_refetchable_data_from_disk_cache_ data +0x5D [sizeof=1] bool is_add_remove_observer_prohibited_ <padding> (2 bytes) } After: class blink::ImageResourceContent [sizeof = 80] : public blink::GarbageCollectedFinalized<blink::ImageResourceContent> , public blink::ImageObserver { base +0x00 [sizeof=8] blink::ImageObserver base +0x08 [sizeof=1] blink::GarbageCollectedFinalized<blink::ImageResourceContent> data +0x09 [sizeof=1] blink::ResourceStatus content_status_ data +0x0a [sizeof=1] bool is_refetchable_data_from_disk_cache_ data +0x0b [sizeof=1] bool is_add_remove_observer_prohibited_ data +0x0c [sizeof=4] blink::Image::SizeAvailability size_available_ data +0x10 [sizeof=8] blink::Member<blink::ImageResourceInfo> info_ data +0x18 [sizeof=8] WTF::RefPtr<blink::Image> image_ data +0x20 [sizeof=24] WTF::HashCountedSet<...> observers_ data +0x38 [sizeof=24] WTF::HashCountedSet<...> finished_observers_ } Before: class blink::DeferredImageDecoder [sizeof = 96] { data +0x00 [sizeof=8] std::unique_ptr<...> rw_buffer_ data +0x08 [sizeof=1] bool all_data_received_ <padding> (7 bytes) data +0x10 [sizeof=8] std::unique_ptr<...> actual_decoder_ data +0x18 [sizeof=8] WTF::String filename_extension_ data +0x20 [sizeof=8] blink::IntSize size_ data +0x28 [sizeof=4] int repetition_count_ data +0x2c [sizeof=1] bool has_embedded_color_space_ <padding> (3 bytes) data +0x30 [sizeof=8] sk_sp<SkColorSpace> color_space_for_sk_images_ data +0x38 [sizeof=1] bool can_yuv_decode_ data +0x39 [sizeof=1] bool has_hot_spot_ <padding> (2 bytes) data +0x3c [sizeof=8] blink::IntPoint hot_spot_ <padding> (4 bytes) data +0x48 [sizeof=16] WTF::Vector<...> frame_data_ data +0x58 [sizeof=8] WTF::RefPtr<...> frame_generator_ } After: class blink::DeferredImageDecoder [sizeof = 80] { data +0x00 [sizeof=8] std::unique_ptr<...> rw_buffer_ data +0x08 [sizeof=8] std::unique_ptr<...> actual_decoder_ data +0x10 [sizeof=8] WTF::String filename_extension_ data +0x18 [sizeof=8] blink::IntSize size_ data +0x20 [sizeof=4] int repetition_count_ data +0x24 [sizeof=1] bool has_embedded_color_space_ data +0x25 [sizeof=1] bool all_data_received_ data +0x26 [sizeof=1] bool can_yuv_decode_ data +0x27 [sizeof=1] bool has_hot_spot_ data +0x28 [sizeof=8] sk_sp<SkColorSpace> color_space_for_sk_images_ data +0x30 [sizeof=8] blink::IntPoint hot_spot_ data +0x38 [sizeof=16] WTF::Vector<...> frame_data_ data +0x48 [sizeof=8] WTF::RefPtr<...> frame_generator_ } BUG= 710933 Review-Url: https://codereview.chromium.org/2859313003 Cr-Commit-Position: refs/heads/master@{#470373} [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/graphics/BitmapImage.h [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/graphics/Image.cpp [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/graphics/Image.h [modify] https://crrev.com/4f54bee9b89dd593c338dbc684f806bb73b48256/third_party/WebKit/Source/platform/loader/fetch/ResourceStatus.h
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a8599ca28499cb5770adc9219857b3a0dff4b90b commit a8599ca28499cb5770adc9219857b3a0dff4b90b Author: stanisc <stanisc@chromium.org> Date: Thu Jun 15 19:55:28 2017 Class/struct layout optimization for blink Resource related classes Reduces sizeof(Resource) by 160 bytes from 1632 bytes to 1472 bytes. Also the following nested classes are optimized: - ResourceRequest - 416 to 368 bytes - ResourceResponse - 696 to 624 bytes - ResourceLoaderOptions 112 to 88 bytes The optimization is achieved by a combination of the following methods: 1) Making enums based on uint8_t or int8_t which reduces their size from 4 bytes to 1 byte. 2) Packing smaller sizing fields together forming 8 byte groups of fields to avoid padding. 3) Grouping bitfields together - disperse bitfields waste a lot of space due to padding The total number of Resource instances is typically 200-500 depending on site so the expected memory improvement is may be up to 100 KB per tab. But the change is straightforward and low risk. Here is an example of optimization. Before: class blink::ResourceResponse [sizeof = 696] { data +0x00 [sizeof=112] blink::KURL url_ data +0x70 [sizeof=8] WTF::AtomicString mime_type_ data +0x78 [sizeof=8] __int64 expected_content_length_ data +0x80 [sizeof=8] WTF::AtomicString text_encoding_name_ data +0x88 [sizeof=4] int http_status_code_ <padding> (4 bytes) data +0x90 [sizeof=8] WTF::AtomicString http_status_text_ data +0x98 [sizeof=24] blink::HTTPHeaderMap http_header_fields_ data +0xb0 [sizeof=1] bool was_cached_ : 1 <padding> (3 bytes) data +0xb4 [sizeof=4] unsigned int connection_id_ data +0xb8 [sizeof=1] bool connection_reused_ : 1 <padding> (7 bytes) data +0xc0 [sizeof=8] WTF::RefPtr<blink::ResourceLoadTiming> resource_load_timing_ data +0xc8 [sizeof=8] WTF::RefPtr<blink::ResourceLoadInfo> resource_load_info_ data +0xd0 [sizeof=1] bool is_null_ : 1 <padding> (7 bytes) data +0xd8 [sizeof=16] blink::CacheControlHeader cache_control_header_ data +0xe8 [sizeof=1] bool have_parsed_age_header_ : 1 data +0xe8 [sizeof=1] bool have_parsed_date_header_ : 1 data +0xe8 [sizeof=1] bool have_parsed_expires_header_ : 1 data +0xe8 [sizeof=1] bool have_parsed_last_modified_header_ : 1 <padding> (7 bytes) data +0xf0 [sizeof=8] double age_ data +0xf8 [sizeof=8] double date_ data +0x100 [sizeof=8] double expires_ data +0x108 [sizeof=8] double last_modified_ data +0x110 [sizeof=1] bool has_major_certificate_errors_ <padding> (3 bytes) data +0x114 [sizeof=4] blink::ResourceResponse::SecurityStyle security_style_ data +0x118 [sizeof=120] blink::ResourceResponse::SecurityDetails security_details_ data +0x190 [sizeof=4] blink::ResourceResponse::HTTPVersion http_version_ <padding> (4 bytes) data +0x198 [sizeof=8] __int64 app_cache_id_ data +0x1a0 [sizeof=112] blink::KURL app_cache_manifest_url_ data +0x210 [sizeof=16] WTF::Vector<char,0,WTF::PartitionAllocator> multipart_boundary_ data +0x220 [sizeof=1] bool was_fetched_via_spdy_ data +0x221 [sizeof=1] bool was_fetched_via_proxy_ data +0x222 [sizeof=1] bool was_fetched_via_service_worker_ data +0x223 [sizeof=1] bool was_fetched_via_foreign_fetch_ data +0x224 [sizeof=1] bool was_fallback_required_by_service_worker_ <padding> (3 bytes) data +0x228 [sizeof=4] blink::WebServiceWorkerResponseType service_worker_response_type_ <padding> (4 bytes) data +0x230 [sizeof=16] WTF::Vector<blink::KURL,0,WTF::PartitionAllocator> url_list_via_service_worker_ data +0x240 [sizeof=8] WTF::String cache_storage_cache_name_ data +0x248 [sizeof=16] WTF::Vector<WTF::String,0,WTF::PartitionAllocator> cors_exposed_header_names_ data +0x258 [sizeof=1] bool did_service_worker_navigation_preload_ <padding> (7 bytes) data +0x260 [sizeof=8] __int64 response_time_ data +0x268 [sizeof=8] WTF::AtomicString remote_ip_address_ data +0x270 [sizeof=2] unsigned short remote_port_ <padding> (6 bytes) data +0x278 [sizeof=8] __int64 encoded_data_length_ data +0x280 [sizeof=8] __int64 encoded_body_length_ data +0x288 [sizeof=8] __int64 decoded_body_length_ data +0x290 [sizeof=8] WTF::String downloaded_file_path_ data +0x298 [sizeof=8] WTF::RefPtr<blink::BlobDataHandle> downloaded_file_handle_ data +0x2a0 [sizeof=8] WTF::RefPtr<blink::ResourceResponse::ExtraData> extra_data_ data +0x2a8 [sizeof=16] WTF::Vector<blink::ResourceResponse,0,WTF::PartitionAllocator> redirect_responses_ } After: class blink::ResourceResponse [sizeof = 624] { data +0x00 [sizeof=112] blink::KURL url_ data +0x70 [sizeof=8] WTF::AtomicString mime_type_ data +0x78 [sizeof=8] __int64 expected_content_length_ data +0x80 [sizeof=8] WTF::AtomicString text_encoding_name_ data +0x88 [sizeof=4] unsigned int connection_id_ data +0x8c [sizeof=4] int http_status_code_ data +0x90 [sizeof=8] WTF::AtomicString http_status_text_ data +0x98 [sizeof=24] blink::HTTPHeaderMap http_header_fields_ data +0xb0 [sizeof=8] WTF::AtomicString remote_ip_address_ data +0xb8 [sizeof=2] unsigned short remote_port_ data +0xba [sizeof=1] bool was_cached_ : 1 data +0xba [sizeof=1] bool connection_reused_ : 1 data +0xba [sizeof=1] bool is_null_ : 1 data +0xba [sizeof=1] bool have_parsed_age_header_ : 1 data +0xba [sizeof=1] bool have_parsed_date_header_ : 1 data +0xba [sizeof=1] bool have_parsed_expires_header_ : 1 data +0xba [sizeof=1] bool have_parsed_last_modified_header_ : 1 data +0xba [sizeof=1] bool has_major_certificate_errors_ : 1 data +0xbb [sizeof=1] bool was_fetched_via_spdy_ : 1 data +0xbb [sizeof=1] bool was_fetched_via_proxy_ : 1 data +0xbb [sizeof=1] bool was_fetched_via_service_worker_ : 1 data +0xbb [sizeof=1] bool was_fetched_via_foreign_fetch_ : 1 data +0xbb [sizeof=1] bool was_fallback_required_by_service_worker_ : 1 data +0xbb [sizeof=1] bool did_service_worker_navigation_preload_ : 1 data +0xbc [sizeof=1] blink::WebServiceWorkerResponseType service_worker_response_type_ data +0xbd [sizeof=1] blink::ResourceResponse::HTTPVersion http_version_ data +0xbe [sizeof=1] blink::ResourceResponse::SecurityStyle security_style_ <padding> (1 bytes) data +0xc0 [sizeof=120] blink::ResourceResponse::SecurityDetails security_details_ data +0x138 [sizeof=8] WTF::RefPtr<blink::ResourceLoadTiming> resource_load_timing_ data +0x140 [sizeof=8] WTF::RefPtr<blink::ResourceLoadInfo> resource_load_info_ data +0x148 [sizeof=16] blink::CacheControlHeader cache_control_header_ data +0x158 [sizeof=8] double age_ data +0x160 [sizeof=8] double date_ data +0x168 [sizeof=8] double expires_ data +0x170 [sizeof=8] double last_modified_ data +0x178 [sizeof=8] __int64 app_cache_id_ data +0x180 [sizeof=112] blink::KURL app_cache_manifest_url_ data +0x1f0 [sizeof=16] WTF::Vector<char,0,WTF::PartitionAllocator> multipart_boundary_ data +0x200 [sizeof=16] WTF::Vector<blink::KURL,0,WTF::PartitionAllocator> url_list_via_service_worker_ data +0x210 [sizeof=8] WTF::String cache_storage_cache_name_ data +0x218 [sizeof=16] WTF::Vector<WTF::String,0,WTF::PartitionAllocator> cors_exposed_header_names_ data +0x228 [sizeof=8] __int64 response_time_ data +0x230 [sizeof=8] __int64 encoded_data_length_ data +0x238 [sizeof=8] __int64 encoded_body_length_ data +0x240 [sizeof=8] __int64 decoded_body_length_ data +0x248 [sizeof=8] WTF::String downloaded_file_path_ data +0x250 [sizeof=8] WTF::RefPtr<blink::BlobDataHandle> downloaded_file_handle_ data +0x258 [sizeof=8] WTF::RefPtr<blink::ResourceResponse::ExtraData> extra_data_ data +0x260 [sizeof=16] WTF::Vector<blink::ResourceResponse,0,WTF::PartitionAllocator> redirect_responses_ } BUG= 710933 Review-Url: https://codereview.chromium.org/2920663002 Cr-Commit-Position: refs/heads/master@{#479798} [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/content/public/renderer/associated_resource_fetcher.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/content/renderer/fetchers/associated_resource_fetcher_impl.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/content/renderer/fetchers/multi_resolution_image_resource_fetcher.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/content/renderer/internal_document_state_data.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/content/renderer/render_frame_impl.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/core/frame/FrameTestHelpers.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/core/loader/HistoryItem.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/FetchContext.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/IntegrityMetadata.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/Resource.cpp [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/Resource.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/ResourceLoadPriority.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/ResourceLoaderOptions.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.cpp [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/loader/fetch/ResourceResponse.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/Source/platform/weborigin/ReferrerPolicy.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/public/platform/WebCachePolicy.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/public/platform/WebURLRequest.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponseType.h [modify] https://crrev.com/a8599ca28499cb5770adc9219857b3a0dff4b90b/third_party/WebKit/public/web/WebLocalFrame.h
,
Oct 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfa838d8751d3a163457e01cf744a75115d04309 commit cfa838d8751d3a163457e01cf744a75115d04309 Author: Rob Buis <rob.buis@samsung.com> Date: Fri Oct 20 18:17:32 2017 Reduce HTMLAnchorElement class size We can reduce HTMLAnchorElement class size by introducing two bitfields. On my system this reduces the size from 128 to 120 bytes. Bug: 710933 Change-Id: Idda84b12870581b30de63aae950a47589fec59ad Reviewed-on: https://chromium-review.googlesource.com/728639 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Rob Buis <rob.buis@samsung.com> Cr-Commit-Position: refs/heads/master@{#510496} [modify] https://crrev.com/cfa838d8751d3a163457e01cf744a75115d04309/third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp [modify] https://crrev.com/cfa838d8751d3a163457e01cf744a75115d04309/third_party/WebKit/Source/core/html/HTMLAnchorElement.h
,
Oct 22
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 24
Unless we find a better way to detect excessive padding (i.e.; frequently used classes that have a lot of padding) we don't have a way to make significant forward progress on this, so closing it makes sense. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by brucedaw...@chromium.org
, Apr 14 2017