Bug 206343 – Compact sizeof(HTMLAnchorElement) and sizeof(HTMLLinkElement)

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

[Yusuke Suzuki]

I noticed that HTMLAnchorElement is relatively frequently allocated, while it has not squeezed

[Yusuke Suzuki]

Created attachment 387902 [details]
Patch

[Yusuke Suzuki]

Created attachment 387903 [details]
Patch

[Yusuke Suzuki]

Created attachment 387996 [details]
Patch

Uploading just to ensure that SharedStringHash is not modified

[Yusuke Suzuki]

Created attachment 387998 [details]
Patch

[Yusuke Suzuki]

The first patch is demonstrating that upper 32bit is never used.

[Ryosuke Niwa]

No need to say WebCore. It's pretty obvious from class names.

[Ryosuke Niwa]

Comment on attachment 387998 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WebCore] Compact sizeof(HTMLAnchorElement) and sizeof(HTMLLinkElement)

Please correct the bug title in the change log accordingly.

[Yusuke Suzuki]

Comment on attachment 387998 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [WebCore] Compact sizeof(HTMLAnchorElement) and sizeof(HTMLLinkElement)
> 
> Please correct the bug title in the change log accordingly.

Thanks, fixed.

[Yusuke Suzuki]

Committed r254739: <https://trac.webkit.org/changeset/254739>

[Radar WebKit Bug Importer]

<rdar://problem/58673446>

[Darin Adler]

Comment on attachment 387998 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.h:140
>      String m_type;
>      String m_media;
> +    String m_integrityMetadataForPendingSheetRequest;
>      std::unique_ptr<DOMTokenList> m_sizes;
> +    std::unique_ptr<DOMTokenList> m_relList;

Seems like we could make HTMLinkElement significantly smaller given that it has many different purposes and we have separate data members for each of those purposes. Something like the rare data pattern perhaps? Even unclear why we need members like m_type and m_media at all, just to cache computed attribute values.

> Source/WebCore/html/LinkRelAttribute.cpp:45
> +    : iconType()

Why is this needed? It won’t get initialized unless we specify it explicitly? I kind of wish Markable had a side effect of initializing things to zero when constructed. Not sure we have any use cases for scalars that don’t get initialized.

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 387998 [details]
Patch

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

> Source/WebCore/html/HTMLLinkElement.cpp:305
> +            makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });

FWIW, this doesn't look right. This will instantly run the callback, as it's not kept alive by any variable.

[Chris Dumez]

Comment on attachment 387998 [details]
Patch

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

>> Source/WebCore/html/HTMLLinkElement.cpp:305
>> +            makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
> 
> FWIW, this doesn't look right. This will instantly run the callback, as it's not kept alive by any variable.

lol, good catch! Not the first time we get caught by that. The return value of makeScopeExit should really be marked as never ignored.

[Chris Dumez]

Comment on attachment 387998 [details]
Patch

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

>>> Source/WebCore/html/HTMLLinkElement.cpp:305
>>> +            makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
>> 
>> FWIW, this doesn't look right. This will instantly run the callback, as it's not kept alive by any variable.
> 
> lol, good catch! Not the first time we get caught by that. The return value of makeScopeExit should really be marked as never ignored.

I mean WARN_UNUSED_RETURN.

[Yusuke Suzuki]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 387998 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387998&action=review
> 
> >>> Source/WebCore/html/HTMLLinkElement.cpp:305
> >>> +            makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
> >> 
> >> FWIW, this doesn't look right. This will instantly run the callback, as it's not kept alive by any variable.
> > 
> > lol, good catch! Not the first time we get caught by that. The return value of makeScopeExit should really be marked as never ignored.
> 
> I mean WARN_UNUSED_RETURN.

Oops, nice. I'm creating a patch for this now.

[Yusuke Suzuki]

Comment on attachment 387998 [details]
Patch

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

>>>>> Source/WebCore/html/HTMLLinkElement.cpp:305
>>>>> +            makeScopeExit([&] { m_isHandlingBeforeLoad = previous; });
>>>> 
>>>> FWIW, this doesn't look right. This will instantly run the callback, as it's not kept alive by any variable.
>>> 
>>> lol, good catch! Not the first time we get caught by that. The return value of makeScopeExit should really be marked as never ignored.
>> 
>> I mean WARN_UNUSED_RETURN.
> 
> Oops, nice. I'm creating a patch for this now.

Opened for this https://bugs.webkit.org/show_bug.cgi?id=208162