Bug 195573 – Alter Tools/Scripts/dump-class-layout to be able to dump all classes with suspicious padding

• Status: REOPENED
• Component: JavaScriptCore

[Robin Morisset]

Finding exactly the classes whose padding can be reduced might be tricky, but finding classes with at least 8 bytes of padding at the top-level is easy, and it is a very good approximation of what we want.
It is quite easy to then go through the list and see at a glance if their padding can be reduced.

[Robin Morisset]

Created attachment 364292 [details]
Patch

[Robin Morisset]

Comment on attachment 364292 [details]
Patch

After using the script a bit more I realized it has a fairly high rate of duplicates, so I am removing it from review until I find a way to fix it.

[Robin Morisset]

Created attachment 364329 [details]
Patch

[Daniel Bates]

I didn’t read the patch, but we have existing tests for this code and I noticed you had **no** tests in your patch. Tests please or explain why you can’t in the ChangeLog.

[Daniel Bates]

Comment on attachment 364329 [details]
Patch

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

> Tools/Scripts/dump-class-layout:55
> +        help='name of the class or struct to dump. ALL_WASTEFUL is a special name that makes it dump all classes with at least 8 bytes of padding at the top level.')

OK as-is, but in my eyes this just reads lazy and not at all intuitive. But hey, you explained in the usage so I guess that’s fair :/. I personally use this tool quite a bit. So I find this kind of Jack very bitter to swallow, but maybe I won’t ever use this feature. Exposing a new command line flag is the correct approach to this situation and the most Unix-like.

[Simon Fraser (smfr)]

Comment on attachment 364329 [details]
Patch

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

r- for another round.

>> Tools/Scripts/dump-class-layout:55
>> +        help='name of the class or struct to dump. ALL_WASTEFUL is a special name that makes it dump all classes with at least 8 bytes of padding at the top level.')
> 
> OK as-is, but in my eyes this just reads lazy and not at all intuitive. But hey, you explained in the usage so I guess that’s fair :/. I personally use this tool quite a bit. So I find this kind of Jack very bitter to swallow, but maybe I won’t ever use this feature. Exposing a new command line flag is the correct approach to this situation and the most Unix-like.

I think it would be better as a new argument --all-wasteful that just overrides classname.

[Robin Morisset]

Created attachment 364409 [details]
Patch

[Robin Morisset]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 364329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364329&action=review
> 
> r- for another round.
> 
> >> Tools/Scripts/dump-class-layout:55
> >> +        help='name of the class or struct to dump. ALL_WASTEFUL is a special name that makes it dump all classes with at least 8 bytes of padding at the top level.')
> > 
> > OK as-is, but in my eyes this just reads lazy and not at all intuitive. But hey, you explained in the usage so I guess that’s fair :/. I personally use this tool quite a bit. So I find this kind of Jack very bitter to swallow, but maybe I won’t ever use this feature. Exposing a new command line flag is the correct approach to this situation and the most Unix-like.
> 
> I think it would be better as a new argument --all-wasteful that just
> overrides classname.

Fair enough, I should have used a new argument from the start. I've updated a new patch with this fixed (and fixed the problem where it used to dump duplicate types).

I could not see how to tell argparse that exactly one of classname and --all-wasteful must be set, so I added that check separately.

[Simon Fraser (smfr)]

Comment on attachment 364409 [details]
Patch

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

> Tools/Scripts/dump-class-layout:88
> +    if args.all_wasteful and (args.classname is not None):
> +        print "The -w/--all-wasteful option is incompatible with providing a class name"

Maybe we should allow multiple classnames, and then you could just handle both.

[WebKit Commit Bot]

Comment on attachment 364409 [details]
Patch

Clearing flags on attachment: 364409

Committed r242825: <https://trac.webkit.org/changeset/242825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48822054>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 195648