Bug 188945 - Shrink size of HTMLCollection
Summary: Shrink size of HTMLCollection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-25 06:57 PDT by Yusuke Suzuki
Modified: 2018-08-30 03:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.58 KB, patch)
2018-08-25 06:57 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-08-25 06:57:30 PDT
Shrink size of HTMLCollection
Comment 1 Yusuke Suzuki 2018-08-25 06:57:53 PDT
Created attachment 348077 [details]
Patch
Comment 2 Darin Adler 2018-08-25 17:18:59 PDT
Comment on attachment 348077 [details]
Patch

I wish there is something we could do to make sure people don’t accidentally undo this when making other changes in the future.
Comment 3 Yusuke Suzuki 2018-08-27 01:52:02 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 348077 [details]
> Patch
> 
> I wish there is something we could do to make sure people don’t accidentally
> undo this when making other changes in the future.

I think the only reasonable way we have right now is adding `static_assert(sizeof(HTMLCollection) <= 40, "")`.
But I don't think it is good since the size of HTMLCollection is only the part of the problem.
The rather important thing is HTMLCollection had many paddings.
The size can increase due to reasonable reasons. But increasing paddings should be avoided.
So it should be caught.

The ideal way would be having a test using dump-class-layout that ensures the padding rate is small in some important classes.
But this is not achievable without tooling and test infrastructure support.

So, in the meantime, I'll land this patch as is.

Simon, do you have any idea about introducing such a test (described above) into WebKit tests?
Comment 4 Yusuke Suzuki 2018-08-27 01:52:46 PDT
Committed r235357: <https://trac.webkit.org/changeset/235357>
Comment 5 Radar WebKit Bug Importer 2018-08-27 01:53:30 PDT
<rdar://problem/43746564>
Comment 6 Simon Fraser (smfr) 2018-08-27 09:28:38 PDT
(In reply to Yusuke Suzuki from comment #3)
> (In reply to Darin Adler from comment #2)
> > Comment on attachment 348077 [details]
> > Patch
> > 
> Simon, do you have any idea about introducing such a test (described above)
> into WebKit tests?

We do have testing of dump-class-layout in webkitpy tests, so it wouldn't be hard to rig some up somehow.
Comment 7 Simon Fraser (smfr) 2018-08-27 09:32:16 PDT
Comment on attachment 348077 [details]
Patch

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

> Source/WebCore/html/HTMLCollection.h:107
>      const unsigned m_collectionType : 5;
>      const unsigned m_invalidationType : 4;

Would be nice to see comments say what the enum type is.
Comment 8 Yusuke Suzuki 2018-08-30 03:17:42 PDT
Committed r235497: <https://trac.webkit.org/changeset/235497>