To confirm correctness and improvement provided by various code-size-reduction optimization we should have a way to measure JIT generated code size.
Created attachment 69697 [details] Patch adds function to dump generated code size before and after branch compaction.
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".
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)
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.
Created attachment 69946 [details] conditionally include ARM assembly dump via CPU(ARM_THUMB2)
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)".
Created attachment 70006 [details] Fix style
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.
David is not a committer. Either this will need a new patch to be cq+'d or we can cq+ it as is...
I committed this the old-fashioned way, with Darin's suggested changes: Committed revision 69787.