I noticed that HTMLAnchorElement is relatively frequently allocated, while it has not squeezed
Created attachment 387902 [details] Patch
Created attachment 387903 [details] Patch
Created attachment 387996 [details] Patch Uploading just to ensure that SharedStringHash is not modified
Created attachment 387998 [details] Patch
The first patch is demonstrating that upper 32bit is never used.
No need to say WebCore. It's pretty obvious from class names.
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.
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.
Committed r254739: <https://trac.webkit.org/changeset/254739>
<rdar://problem/58673446>
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.
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.
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.
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.
(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.
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