Bug 47121 - need way to measure size of JITed ARM code
Summary: need way to measure size of JITed ARM code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-04 15:15 PDT by David Goodwin
Modified: 2010-11-22 16:51 PST (History)
7 users (show)

See Also:


Attachments
Patch adds function to dump generated code size before and after branch compaction. (2.23 KB, patch)
2010-10-04 15:22 PDT, David Goodwin
ggaren: review-
Details | Formatted Diff | Diff
Address review comments and add additional output (3.63 KB, patch)
2010-10-05 16:23 PDT, David Goodwin
no flags Details | Formatted Diff | Diff
conditionally include ARM assembly dump via CPU(ARM_THUMB2) (3.67 KB, patch)
2010-10-06 09:02 PDT, David Goodwin
ggaren: review-
Details | Formatted Diff | Diff
Fix style (3.72 KB, patch)
2010-10-06 16:33 PDT, David Goodwin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Goodwin 2010-10-04 15:15:57 PDT
To confirm correctness and improvement provided by various code-size-reduction optimization we should have a way to measure JIT generated code size.
Comment 1 David Goodwin 2010-10-04 15:22:45 PDT
Created attachment 69697 [details]
Patch adds function to dump generated code size before and after branch compaction.
Comment 2 Geoffrey Garen 2010-10-05 08:38:57 PDT
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".
Comment 3 David Goodwin 2010-10-05 16:23:27 PDT
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)
Comment 4 David Goodwin 2010-10-06 08:37:06 PDT
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.
Comment 5 David Goodwin 2010-10-06 09:02:14 PDT
Created attachment 69946 [details]
conditionally include ARM assembly dump via CPU(ARM_THUMB2)
Comment 6 Geoffrey Garen 2010-10-06 10:43:54 PDT
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)".
Comment 7 David Goodwin 2010-10-06 16:33:57 PDT
Created attachment 70006 [details]
Fix style
Comment 8 Darin Adler 2010-10-13 17:22:36 PDT
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.
Comment 9 Eric Seidel (no email) 2010-10-14 11:40:09 PDT
David is not a committer.  Either this will need a new patch to be cq+'d or we can cq+ it as is...
Comment 10 Geoffrey Garen 2010-10-14 12:19:55 PDT
I committed this the old-fashioned way, with Darin's suggested changes:

Committed revision 69787.