Bug 174993

Created attachment 316805 [details] Patch

Comment on attachment 316805 [details] Patch Clearing flags on attachment: 316805 Committed r220087: <http://trac.webkit.org/changeset/220087>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33646963>

Could you please apply this change on build.webkit.org master? It seems it didn't restart itself automatically.

(In reply to Csaba Osztrogonác from comment #5) > Could you please apply this change on build.webkit.org master? > It seems it didn't restart itself automatically. Ping?

I'm looking into this now.

(In reply to Lucas Forschler from comment #7) > I'm looking into this now. Thanks. But it seems I made a mistake and increaded the timeout only for running test, but not for build step. :( I already fixed it in https://trac.webkit.org/changeset/220123/webkit Could you pick up this change too? Thanks, and sorry for the inconvenience.

Reopen, because the problem is bigger now. :( Recent offlineasm changes amazingly increased the generating time. Previously a clean build took (gnerating and compiling took) ~9 minutes, https://trac.webkit.org/changeset/220047 increased it to ~30 minutes and https://trac.webkit.org/changeset/220184 increased it to ~91 minutes. I haven't measured the exact runtime of the ruby scripts separately yet, but it's obvious that these changes are responsible for long running ruby scripts. (I bet the slowdown isn't problem on native builds, but I can imagine that the runtime was increased from few seconds to 1-2 minutes.) FYI, I stopped the JSCOnly ARM bots because of this blocker issue, because I won't be available for the following 2 weeks, so I won't have any chance to be able to investigate or help to fix this issue.

(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #9) > Reopen, because the problem is bigger now. :( > > Recent offlineasm changes amazingly increased the generating time. > Previously a clean build took (gnerating and compiling took) ~9 minutes, > https://trac.webkit.org/changeset/220047 increased it to ~30 minutes and > https://trac.webkit.org/changeset/220184 increased it to ~91 minutes. > > I haven't measured the exact runtime of the ruby scripts separately yet, but > it's obvious that these changes are responsible for long running ruby > scripts. > (I bet the slowdown isn't problem on native builds, but I can imagine that > the runtime was increased from few seconds to 1-2 minutes.) > > FYI, I stopped the JSCOnly ARM bots because of this blocker issue, > because I won't be available for the following 2 weeks, so I won't > have any chance to be able to investigate or help to fix this issue. My Ruby knowledge is near zero, but I find hard to believe r220184 has increased the build time in 60 minutes (from 30 to 91)!. Even when running on an ARM CPU, the diff on the ruby script don't seem that big or complex.

(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #9) > Reopen, because the problem is bigger now. :( > > Recent offlineasm changes amazingly increased the generating time. > Previously a clean build took (gnerating and compiling took) ~9 minutes, > https://trac.webkit.org/changeset/220047 increased it to ~30 minutes and > https://trac.webkit.org/changeset/220184 increased it to ~91 minutes. > > I haven't measured the exact runtime of the ruby scripts separately yet, but > it's obvious that these changes are responsible for long running ruby > scripts. > (I bet the slowdown isn't problem on native builds, but I can imagine that > the runtime was increased from few seconds to 1-2 minutes.) > > FYI, I stopped the JSCOnly ARM bots because of this blocker issue, > because I won't be available for the following 2 weeks, so I won't > have any chance to be able to investigate or help to fix this issue. I made a quick measurement on X86_64 Linux. The runtime of the ruby scripts (generate_offset_extractor.rb and asm.rb) was 3-4 secs/script before r220047, which was increased to 14-15 secs by r220047 and was increased to 25-26 secs by r220184. It is a huge regression, the original runtime was increased to 8x. Keith, Filip, please investigate this huge regression as the authors of r220047 and r220184 changes? Thanks.

(In reply to Carlos Alberto Lopez Perez from comment #10) > My Ruby knowledge is near zero, but I find hard to believe r220184 has > increased the build time in 60 minutes (from 30 to 91)!. Even when running > on an ARM CPU, the diff on the ruby script don't seem that big or complex. But it's true, the slowdown is near ~8x on X86_64. (Back to the ARM numbers, I remember the original runtime was ~10 minutes, now it is ~90 minutes)

I investigated this huge regression a little bit, it seems the offlineasm scripts (generate_offset_extractor.rb) are doing fundamentally wrong things, they generates useless things. Before r220047 LLIntDesiredOffsets.h was 12Mb, now it is 87Mb sized, because there are zillion but one useless ifdefed code in it. There are 10 valid backends now: --------------------------------- OFFLINE_ASM_X86 !OFFLINE_ASM_X86_WIN !OFFLINE_ASM_X86_64 !OFFLINE_ASM_X86_64_WIN !OFFLINE_ASM_ARM !OFFLINE_ASM_ARMv7 !OFFLINE_ASM_ARMv7_TRADITIONAL !OFFLINE_ASM_ARM64 !OFFLINE_ASM_MIPS !OFFLINE_ASM_C_LOOP There are 8 misc options now: ------------------------------ OFFLINE_ASM_ARMv7k OFFLINE_ASM_ARMv7s OFFLINE_ASM_JSVALUE64 OFFLINE_ASM_BIG_ENDIAN OFFLINE_ASM_ASSERT_ENABLED OFFLINE_ASM_COLLECT_STATS OFFLINE_ASM_EXECUTION_TRACING OFFLINE_ASM_GIGACAGE_ENABLED generate_offset_extractor.rb now generates 3 ifdef block for each misc option combinations, which means 3 * 10 * 2^8 = 7680. And only 3 of them are valid, the remaining 7677 are completely useless for anything. Adding one more misc option to offlineasm, it doubles these configurations, doubles the script runtime and doubles the size of LLIntDesiredOffsets.h. It is completely useless and unsustainable. Why do we need to generate useless codes at all? It's obvious that - OFFLINE_ASM_ARMv7k and OFFLINE_ASM_ARMv7s can be true on Apple's iOS build - JSVALUE64 can be true only on 64 bit platforms (X86_64 and AArch64) - OFFLINE_ASM_BIG_ENDIAN is clearly defined by platform ... I think each backends are clearly define all misc options, we don't need to generate any code for invalid misc option combinations. And we don't need to genarate any code for a platform we don't build right now.

I found a partial fix for this issue, we can pass the backend name(s) to generate_offset_extractor.rb and it will generate only that backend(s). This feature is introduced 3 years ago in https://trac.webkit.org/changeset/172777. With only one backend generation, a complete JSCOnly ARM clean build takes only 9-10 minutes. I added this local hack to the bots and will find how to pass ARMv7, ARMv7_TRADITIONAL or ARM64 to generate_offset_extractor.rb via cmake automatically. Additionally I still think that we don't need to generate 256 combinations for each backend when only one can be valid. We should find how to do it.

Created attachment 318061 [details] Patch

Comment on attachment 318061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318061&action=review > Source/JavaScriptCore/CMakeLists.txt:1173 > + elseif (ARM_TRADITIONAL_DETECTED) > + set(OFFLINE_ASM_BACKEND "ARMv7_TRADITIONAL") > + endif () > + Please add an extra "elseif" for MIPS + elseif (WTF_CPU_ARM64) + set(OFFLINE_ASM_BACKEND "MIPS")

Created attachment 318244 [details] Patch for landing

(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #17) > Created attachment 318244 [details] > Patch for landing (In reply to Carlos Alberto Lopez Perez from comment #16) > Comment on attachment 318061 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318061&action=review > > > Source/JavaScriptCore/CMakeLists.txt:1173 > > + elseif (ARM_TRADITIONAL_DETECTED) > > + set(OFFLINE_ASM_BACKEND "ARMv7_TRADITIONAL") > > + endif () > > + > Please add an extra "elseif" for MIPS > > + elseif (WTF_CPU_ARM64) > + set(OFFLINE_ASM_BACKEND "MIPS") I bet you meant this change ;) I added it. + elseif (WTF_CPU_MIPS) + set(OFFLINE_ASM_BACKEND "MIPS")

(In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #18) > (In reply to Csaba Osztrogonác_OOO_until_21st_Aug from comment #17) > > Created attachment 318244 [details] > > Patch for landing > > (In reply to Carlos Alberto Lopez Perez from comment #16) [....] > > Please add an extra "elseif" for MIPS > > > > + elseif (WTF_CPU_ARM64) > > + set(OFFLINE_ASM_BACKEND "MIPS") > > I bet you meant this change ;) I added it. > + elseif (WTF_CPU_MIPS) > + set(OFFLINE_ASM_BACKEND "MIPS") Ups! sure :) thanks!

Comment on attachment 318244 [details] Patch for landing Clearing flags on attachment: 318244 Committed r220791: <http://trac.webkit.org/changeset/220791>

All reviewed patches have been landed. Closing bug.