Bug 195573 - Alter Tools/Scripts/dump-class-layout to be able to dump all classes with suspicious padding
Summary: Alter Tools/Scripts/dump-class-layout to be able to dump all classes with sus...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 195648
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-11 14:55 PDT by Robin Morisset
Modified: 2019-03-12 15:45 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.80 KB, patch)
2019-03-11 15:01 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (4.86 KB, patch)
2019-03-11 18:17 PDT, Robin Morisset
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (5.33 KB, patch)
2019-03-12 11:06 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-03-11 14:55:47 PDT
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.
Comment 1 Robin Morisset 2019-03-11 15:01:17 PDT
Created attachment 364292 [details]
Patch
Comment 2 Robin Morisset 2019-03-11 16:50:25 PDT
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.
Comment 3 Robin Morisset 2019-03-11 18:17:49 PDT
Created attachment 364329 [details]
Patch
Comment 4 Daniel Bates 2019-03-11 19:15:01 PDT
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.
Comment 5 Daniel Bates 2019-03-11 19:20:03 PDT
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.
Comment 6 Simon Fraser (smfr) 2019-03-11 19:27:30 PDT
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.
Comment 7 Robin Morisset 2019-03-12 11:06:55 PDT
Created attachment 364409 [details]
Patch
Comment 8 Robin Morisset 2019-03-12 11:10:03 PDT
(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.
Comment 9 Simon Fraser (smfr) 2019-03-12 13:30:06 PDT
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.
Comment 10 WebKit Commit Bot 2019-03-12 13:55:55 PDT
Comment on attachment 364409 [details]
Patch

Clearing flags on attachment: 364409

Committed r242825: <https://trac.webkit.org/changeset/242825>
Comment 11 WebKit Commit Bot 2019-03-12 13:55:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-03-12 13:56:23 PDT
<rdar://problem/48822054>
Comment 13 WebKit Commit Bot 2019-03-12 15:45:35 PDT
Re-opened since this is blocked by bug 195648