Bug 47121 – need way to measure size of JITed ARM code

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Goodwin]

To confirm correctness and improvement provided by various code-size-reduction optimization we should have a way to measure JIT generated code size.

[David Goodwin]

Created attachment 69697 [details]
Patch adds function to dump generated code size before and after branch compaction.

[Geoffrey Garen]

Comment on attachment 69697 [details]
Patch adds function to dump generated code size before and after branch compaction.

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

This patch looks good, but I think it's worth doing those minor style improvements before landing.

> JavaScriptCore/assembler/LinkBuffer.h:288
> +        static unsigned int linkCnt = 0;
> +        static unsigned int totalInitialSize = 0, totalFinalSize = 0;

WebKit style is just "unsigned", not "unsigned int".

> JavaScriptCore/assembler/LinkBuffer.h:289
> +        linkCnt++;

WebKit style is to use full words in names instead of abbreviations: "linkCount".

[David Goodwin]

Created attachment 69863 [details]
Address review comments and add additional output

The updated patch adds #define to enable asm output of JIT generated code ( requires some post processing to make it readable, as noted in the patch)

[David Goodwin]

I notice that LinkBuffer.h is shared across architectures, so I should guard the asm dumping code to be conditional on and ARM targeted build.

[David Goodwin]

Created attachment 69946 [details]
conditionally include ARM assembly dump via CPU(ARM_THUMB2)

[Geoffrey Garen]

Comment on attachment 69946 [details]
conditionally include ARM assembly dump via CPU(ARM_THUMB2)

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

Great patch, but I once again have some minutia about WebKit's coding style.

Here's the full list of guidelines for future reference: http://webkit.org/coding/coding-style.html.

> JavaScriptCore/assembler/LinkBuffer.h:289
> +    static void dumpLinkStats(void *code, size_t initialSize, size_t finalSize)

Should be "void* code".

> JavaScriptCore/assembler/LinkBuffer.h:298
> +        printf("link %p: orig %u, compact %u (delta %u, %.2f%%)\n", 
> +               code, (unsigned)initialSize, (unsigned)finalSize, (unsigned)(initialSize - finalSize),
> +               100.0 * (float)(initialSize - finalSize) / initialSize);

Should be static_cast<x> instead of (x).

> JavaScriptCore/assembler/LinkBuffer.h:301
> +        printf("\ttotal %u: orig %u, compact %u (delta %u, %.2f%%)\n", 
> +               linkCount, totalInitialSize, totalFinalSize, totalInitialSize - totalFinalSize,
> +               100.0 * (float)(totalInitialSize - totalFinalSize) / totalInitialSize);

Ditto.

> JavaScriptCore/assembler/LinkBuffer.h:306
> +    static void dumpCode(void *code, size_t size)

Should be "void* code".

> JavaScriptCore/assembler/LinkBuffer.h:314
> +        unsigned short *tcode = (unsigned short *)code;

Should be "unsigned short* tcode" and "static_cast<unsigned short*>(code)".

[David Goodwin]

Created attachment 70006 [details]
Fix style

[Darin Adler]

Comment on attachment 70006 [details]
Fix style

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

> JavaScriptCore/assembler/LinkBuffer.h:269
> +        dumpLinkStats(m_code, initialSize, m_size);

Why abbreviate to “stats”?

> JavaScriptCore/assembler/LinkBuffer.h:292
> +        static unsigned totalInitialSize = 0, totalFinalSize = 0;

We normally declare these things on separate lines.

[Eric Seidel (no email)]

David is not a committer.  Either this will need a new patch to be cq+'d or we can cq+ it as is...

[Geoffrey Garen]

I committed this the old-fashioned way, with Darin's suggested changes:

Committed revision 69787.