Bug 187455 - Make WebCore::Timer more space-efficient
Summary: Make WebCore::Timer more space-efficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-08 19:20 PDT by Simon Fraser (smfr)
Modified: 2018-08-04 15:58 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2018-08-04 11:32 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2018-07-08 19:20:37 PDT
Timer member variables often introduce 7 bytes of padding because of the bool at the end.

+120 < 72>     WebCore::Timer m_timeoutTimer
+120 < 64>         WebCore::TimerBase WebCore::TimerBase
+120 <  8>            __vtbl_ptr_type * _vptr
+128 <  8>             WTF::MonotonicTime m_nextFireTime
+128 <  8>               double m_value
+136 <  8>             WTF::MonotonicTime m_unalignedNextFireTime
+136 <  8>               double m_value
+144 <  8>             WTF::Seconds m_repeatInterval
+144 <  8>               double m_value
+152 <  4>           int m_heapIndex
+156 <  4>           unsigned int m_heapInsertionOrder
+160 <  1>           bool m_wasDeleted
+161 <  7>           <PADDING: 7 bytes>

Can we steal a bit from m_heapIndex or m_heapInsertionOrder for this bool?
Comment 1 Simon Fraser (smfr) 2018-07-08 19:27:38 PDT
Here's the full layout:

 4$ $ ./Tools/Scripts/dump-class-layout -c Release WebCore Timer
  +0 < 72> Timer
  +0 < 64>     WebCore::TimerBase WebCore::TimerBase
  +0 <  8>        __vtbl_ptr_type * _vptr
  +8 <  8>         WTF::MonotonicTime m_nextFireTime
  +8 <  8>           double m_value
 +16 <  8>         WTF::MonotonicTime m_unalignedNextFireTime
 +16 <  8>           double m_value
 +24 <  8>         WTF::Seconds m_repeatInterval
 +24 <  8>           double m_value
 +32 <  4>       int m_heapIndex
 +36 <  4>       unsigned int m_heapInsertionOrder
 +40 <  1>       bool m_wasDeleted
 +41 <  7>       <PADDING: 7 bytes>
 +48 <  8>       WTF::Vector<WebCore::TimerBase *, 0, WTF::CrashOnOverflow, 16> * m_cachedThreadGlobalTimerHeap
 +56 <  8>         WTF::Ref<WTF::Thread, WTF::DumbPtrTraits<WTF::Thread> > m_thread
 +56 <  8>           WTF::DumbPtrTraits<WTF::Thread>::StorageType m_ptr
 +64 <  8>     WTF::Function<void ()> m_function
 +64 <  8>         std::__1::unique_ptr<WTF::Function<void ()>::CallableWrapperBase, std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> > m_callableWrapper
 +64 <  8>             std::__1::__compressed_pair<WTF::Function<void ()>::CallableWrapperBase *, std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> > __ptr_
 +64 <  8>                 std::__1::__compressed_pair_elem<WTF::Function<void ()>::CallableWrapperBase *, 0, false> std::__1::__compressed_pair_elem<WTF::Function<void ()>::CallableWrapperBase *, 0, false>
 +64 <  8>                   WTF::Function<void ()>::CallableWrapperBase * __value_
 +64 <  1>                 std::__1::__compressed_pair_elem<std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>, 1, true> std::__1::__compressed_pair_elem<std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>, 1, true>
 +64 <  1>                     std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>
Total byte size: 72
Total pad bytes: 7
Padding percentage: 9.72 %
Comment 2 Simon Fraser (smfr) 2018-07-09 18:01:27 PDT
I think we can safely steal a bit from m_heapIndex. m_heapInsertionOrder just keeps increasing and rolls over, but stealing a bit would make that happen more often (but maybe still not often enough to care).
Comment 3 Simon Fraser (smfr) 2018-08-04 11:32:44 PDT
Created attachment 346592 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-08-04 11:33:46 PDT
Layout after:

 12$ $ ./Tools/Scripts/dump-class-layout -c Release WebCore Timer
  +0 < 64> Timer
  +0 < 56>     WebCore::TimerBase WebCore::TimerBase
  +0 <  8>        __vtbl_ptr_type * _vptr
  +8 <  8>         WTF::MonotonicTime m_nextFireTime
  +8 <  8>           double m_value
 +16 <  8>         WTF::MonotonicTime m_unalignedNextFireTime
 +16 <  8>           double m_value
 +24 <  8>         WTF::Seconds m_repeatInterval
 +24 <  8>           double m_value
 +32 < :31>       int m_heapIndex : 31
 +35 < :1>       bool m_wasDeleted : 1
 +36 <  4>       unsigned int m_heapInsertionOrder
 +40 <  8>       WTF::Vector<WebCore::TimerBase *, 0, WTF::CrashOnOverflow, 16> * m_cachedThreadGlobalTimerHeap
 +48 <  8>         WTF::Ref<WTF::Thread, WTF::DumbPtrTraits<WTF::Thread> > m_thread
 +48 <  8>           WTF::DumbPtrTraits<WTF::Thread>::StorageType m_ptr
 +56 <  8>     WTF::Function<void ()> m_function
 +56 <  8>         std::__1::unique_ptr<WTF::Function<void ()>::CallableWrapperBase, std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> > m_callableWrapper
 +56 <  8>             std::__1::__compressed_pair<WTF::Function<void ()>::CallableWrapperBase *, std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> > __ptr_
 +56 <  8>                 std::__1::__compressed_pair_elem<WTF::Function<void ()>::CallableWrapperBase *, 0, false> std::__1::__compressed_pair_elem<WTF::Function<void ()>::CallableWrapperBase *, 0, false>
 +56 <  8>                   WTF::Function<void ()>::CallableWrapperBase * __value_
 +56 <  1>                 std::__1::__compressed_pair_elem<std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>, 1, true> std::__1::__compressed_pair_elem<std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>, 1, true>
 +56 <  1>                     std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>
Total byte size: 64
Total pad bytes: 0
Comment 5 Brent Fulgham 2018-08-04 15:30:30 PDT
Comment on attachment 346592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346592&action=review

> Source/WebCore/platform/Timer.h:101
> +    signed int m_heapIndex : 31; // -1 if not in heap

We basically lose one bit for sign, I wonder if we could pack the “not in heap” state as a bool, too, to avoid magic number code,

> Source/WebCore/platform/Timer.h:102
> +    bool m_wasDeleted : 1;

Nice!
Comment 6 WebKit Commit Bot 2018-08-04 15:57:20 PDT
Comment on attachment 346592 [details]
Patch

Clearing flags on attachment: 346592

Committed r234581: <https://trac.webkit.org/changeset/234581>
Comment 7 WebKit Commit Bot 2018-08-04 15:57:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-08-04 15:58:17 PDT
<rdar://problem/42937768>