Class/struct layout optimization for blink Resource related classes

third_party/WebKit/Source/platform/loader/fetch/Resource.h

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller <mueller@kde.org>
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All
     rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 51 matching lines @@
 // with the loader to obtain the resource from the network.
 class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
                                  public MemoryCoordinatorClient {
   USING_GARBAGE_COLLECTED_MIXIN(Resource);
   WTF_MAKE_NONCOPYABLE(Resource);
 
  public:
   // |Type| enum values are used in UMAs, so do not change the values of
   // existing |Type|. When adding a new |Type|, append it at the end and update
   // |kLastResourceType|.
-  enum Type {
+  enum Type : uint8_t {
     kMainResource,
     kImage,
     kCSSStyleSheet,
     kScript,
     kFont,
     kRaw,
     kSVGDocument,
     kXSLStyleSheet,
     kLinkPrefetch,
     kTextTrack,
@@ skipping 67 matching lines @@
   virtual ResourcePriority PriorityFromObservers() {
     return ResourcePriority();
   }
 
   // The reference policy indicates that the client should not affect whether
   // a preload is considered referenced or not. This allows for "passive"
   // resource clients that simply observe the resource.
   void AddClient(ResourceClient*, PreloadReferencePolicy = kMarkAsReferenced);
   void RemoveClient(ResourceClient*);
 
-  enum PreloadResult {
+  enum PreloadResult : uint8_t {
     kPreloadNotReferenced,
     kPreloadReferenced,
     kPreloadReferencedWhileLoading,
     kPreloadReferencedWhileComplete
   };
   PreloadResult GetPreloadResult() const { return preload_result_; }
 
   ResourceStatus GetStatus() const { return status_; }
   void SetStatus(ResourceStatus status) { status_ = status; }
 
@@ skipping 251 matching lines @@
   void RevalidationSucceeded(const ResourceResponse&);
   void RevalidationFailed();
 
   size_t CalculateOverheadSize() const;
 
   String ReasonNotDeletable() const;
 
   // MemoryCoordinatorClient overrides:
   void OnPurgeMemory() override;
 
+  PreloadResult preload_result_;
+  Type type_;
+  ResourceStatus status_;
+
+  bool needs_synchronous_cache_hit_ : 1;

[dcheng, 2017/06/05 18:15:40]

Is the bitfield packing necessary? We don't pack everything as bitfields, so
it's not clear why some objects are packed and others aren't.

[stanisc, 2017/06/06 22:47:55]

Not necessary in this particular case. I was debating whether to introduce it or
not and decided to introduce it to save another 16 bytes of the class sizeof
(the actual difference would be 8 bytes but the allocation granularity is 16
bytes). 
Bitfields can't be applied to the last two boolean fields because there is some
code that takes address of those field.

I am OK with reverting these back to regular boolean fields if you think this is
sketchy.

[dcheng, 2017/06/07 17:09:22]

If I had written the CL, I probably wouldn't have used bitfields for
consistency, but I don't feel strongly about it.

[stanisc, 2017/06/08 21:34:03]

OK, I've removed bitfields because that has been suggested by another reviewer
too.

+  bool link_preload_ : 1;
+  bool is_revalidating_ : 1;
+  bool is_alive_ : 1;
+
+  bool is_add_remove_client_prohibited_;
+  bool is_revalidation_start_forbidden_ = false;
+
+  ResourceIntegrityDisposition integrity_disposition_;

[dcheng, 2017/06/07 17:09:22]

This IMO is the big disadvantage of reordering fields to optimize class layout:
this was originally next to IntegrityMetadataSet, which makes sense--but now
it's pretty far away. Anything we can do to put it nearby without wasting too
much padding?

[stanisc, 2017/06/08 21:34:03]

OK, moved it back.

+
   Member<CachedMetadataHandlerImpl> cache_handler_;
   RefPtr<SecurityOrigin> fetcher_security_origin_;
 
   ResourceError error_;
 
   double load_finish_time_;
 
   unsigned long identifier_;
 
+  unsigned preload_count_;
+  double preload_discovery_time_;
+
   size_t encoded_size_;
   size_t encoded_size_memory_usage_;
   size_t decoded_size_;
 
   // Resource::calculateOverheadSize() is affected by changes in
   // |m_resourceRequest.url()|, but |m_overheadSize| is not updated after
   // initial |m_resourceRequest| is given, to reduce MemoryCache manipulation
   // and thus potential bugs. crbug.com/594644
   const size_t overhead_size_;
 
-  unsigned preload_count_;
-
-  double preload_discovery_time_;
-
   String cache_identifier_;
 
-  PreloadResult preload_result_;
-  Type type_;
-  ResourceStatus status_;
-
-  bool needs_synchronous_cache_hit_;
-  bool link_preload_;
-  bool is_revalidating_;
-  bool is_alive_;
-  bool is_add_remove_client_prohibited_;
-  bool is_revalidation_start_forbidden_ = false;
-
-  ResourceIntegrityDisposition integrity_disposition_;
   IntegrityMetadataSet integrity_metadata_;
 
   // Ordered list of all redirects followed while fetching this resource.
   Vector<RedirectPair> redirect_chain_;
 
   HeapHashCountedSet<WeakMember<ResourceClient>> clients_;
   HeapHashCountedSet<WeakMember<ResourceClient>> clients_awaiting_callback_;
   HeapHashCountedSet<WeakMember<ResourceClient>> finished_clients_;
 
   ResourceLoaderOptions options_;
@@ skipping 26 matching lines @@
 };
 
 #define DEFINE_RESOURCE_TYPE_CASTS(typeName)                      \
   DEFINE_TYPE_CASTS(typeName##Resource, Resource, resource,       \
                     resource->GetType() == Resource::k##typeName, \
                     resource.GetType() == Resource::k##typeName);
 
 }  // namespace blink
 
 #endif