Bug 188713 – Pack booleans in Event into a bitfield

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Make Event object slightly smaller.

[Ryosuke Niwa]

Created attachment 347419 [details]
Cleanup

[EWS Watchlist]

Attachment 347419 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Event.cpp:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 347419 [details]
Cleanup

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

> Source/WebCore/dom/Event.cpp:37
> +ALWAYS_INLINE Event::Event(MonotonicTime createTime, const AtomicString& type, IsTrusted isTrusted, CanBubble canBubble, IsCancelable cancelable, IsComposed composed)

This feels like a step backwards from using default initializers. I am assuming it is a memory footprint win. Once we move to C++20 we can use default initalizers for bitfields.

> Source/WebCore/dom/Event.h:171
> +    unsigned m_eventPhase : 2; // 13-bits

I wish we had a good solution for this. Very easy for a person to add a new every phase and forget to update the width (if needed). This problem is not unique to this code. One tactic I have seen is to define a constexpr for the width above the enum in the hope that a person sees it. Another tactic is to add placeholder enum after last enumerator then compute width.

[Simon Fraser (smfr)]

Comment on attachment 347419 [details]
Cleanup

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

>> Source/WebCore/dom/Event.h:171
>> +    unsigned m_eventPhase : 2; // 13-bits
> 
> I wish we had a good solution for this. Very easy for a person to add a new every phase and forget to update the width (if needed). This problem is not unique to this code. One tactic I have seen is to define a constexpr for the width above the enum in the hope that a person sees it. Another tactic is to add placeholder enum after last enumerator then compute width.

It took me a few seconds to realize that the "13 bits" didn't mean that m_eventPhase needs 13 bits. I think this comment is confusing, and dump-class-layout can show bit counts so there's no need for the comment now.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 347419 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347419&action=review
> 
> > Source/WebCore/dom/Event.cpp:37
> > +ALWAYS_INLINE Event::Event(MonotonicTime createTime, const AtomicString& type, IsTrusted isTrusted, CanBubble canBubble, IsCancelable cancelable, IsComposed composed)
> 
> This feels like a step backwards from using default initializers. I am
> assuming it is a memory footprint win. Once we move to C++20 we can use
> default initalizers for bitfields.

Yeah, it would be nice to be able to use default initializers with bitfields.

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 347419 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347419&action=review
> 
> >> Source/WebCore/dom/Event.h:171
> >> +    unsigned m_eventPhase : 2; // 13-bits
> > 
> > I wish we had a good solution for this. Very easy for a person to add a new every phase and forget to update the width (if needed). This problem is not unique to this code. One tactic I have seen is to define a constexpr for the width above the enum in the hope that a person sees it. Another tactic is to add placeholder enum after last enumerator then compute width.
> 
> It took me a few seconds to realize that the "13 bits" didn't mean that
> m_eventPhase needs 13 bits. I think this comment is confusing, and
> dump-class-layout can show bit counts so there's no need for the comment now.

Okay, will remove the comment.

[Ryosuke Niwa]

Committed r235009: <https://trac.webkit.org/changeset/235009>

[Radar WebKit Bug Importer]

<rdar://problem/43454338>

[Yusuke Suzuki]

Nice!

[Lucas Forschler]

Mass move bugs into the DOM component.