New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 710933 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 24
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 711101



Sign in to add a comment

Chrome's classes should have their padding checked on 64-bit

Project Member Reported by brucedaw...@chromium.org, Apr 12 2017

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.

 
See crrev.com/2816143003 which fixed generation of inefficiently padded structs, tracked by the more specific  bug 711101 .
Blockedon: 711101
Cc: stanisc@chromium.org
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/

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Comment 6 by zturner@google.com, 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.
Project Member

Comment 7 by bugdroid1@chromium.org, 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

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");

Comment 9 by zturner@google.com, 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.  
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.

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_
    }
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.
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.)
@13: A fix for v8::internal::compiler::Constant is coming today and I am looking at other classes too.
https://codereview.chromium.org/2841343002/

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)
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Project Member

Comment 17 by bugdroid1@chromium.org, 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

Project Member

Comment 18 by bugdroid1@chromium.org, 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

Project Member

Comment 19 by bugdroid1@chromium.org, 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

Project Member

Comment 20 by bugdroid1@chromium.org, 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

Project Member

Comment 21 by sheriffbot@chromium.org, Oct 22

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
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
Status: Fixed (was: Untriaged)
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