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.
Created attachment 364292 [details] Patch
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.
Created attachment 364329 [details] Patch
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 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 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.
Created attachment 364409 [details] Patch
(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 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 on attachment 364409 [details] Patch Clearing flags on attachment: 364409 Committed r242825: <https://trac.webkit.org/changeset/242825>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48822054>
Re-opened since this is blocked by bug 195648