Bug 174993 – Fix JSCOnly ARM buildbots after r220047 and r220184

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/220047/webkit broke the JSCOnly ARM buildbots,
because they can't generate DerivedSources/JavaScriptCore/LLIntAssembly.h
in the 20 minute timeout since r220047.

( The buildbots run on X86_64 inside a qemu emulated ARM environment,
so ruby is emulated too, that's why it is so slow. )

We have to increase the timeout to have working buildbots again.

[Csaba Osztrogonác]

Created attachment 316805 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316805 [details]
Patch

Clearing flags on attachment: 316805

Committed r220087: <http://trac.webkit.org/changeset/220087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33646963>

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(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?

[Lucas Forschler]

I'm looking into this now.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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.

[Carlos Alberto Lopez Perez]

(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.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

(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)

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 318061 [details]
Patch

[Carlos Alberto Lopez Perez]

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")

[Csaba Osztrogonác]

Created attachment 318244 [details]
Patch for landing

[Csaba Osztrogonác]

(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")

[Carlos Alberto Lopez Perez]

(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!

[WebKit Commit Bot]

Comment on attachment 318244 [details]
Patch for landing

Clearing flags on attachment: 318244

Committed r220791: <http://trac.webkit.org/changeset/220791>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.