Bug 217989 – Add new build option USE(64KB_PAGE_BLOCK)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Add new build option USE(64KB_PAGE_BLOCK)

Why do we need this option? Because JSC and bmalloc both want to know the userspace page size at compile time, which is impossible on Linux because it's a runtime setting. We cannot test the system page size at build time in hopes that it will be the same on the target system, because (a) cross compiling wouldn't work, and (b) the build system could use a different page size than the target system (which will be true for Fedora aarch64, because Fedora is built using RHEL), so the best we can do is guess based on based on the target CPU architecture. In practice, guessing works for all architectures except aarch64 (unless unusual page sizes are used), but it fails for aarch64 because distros are split between using 4 KB and 64 KB pages there. Most distros (including Fedora) use 4 KB, but RHEL uses 16 KB. SUSE actually supports both. Since there is no way to guess correctly, the best we can do is provide an option for it. You should probably only to use this if building for aarch64. Otherwise, known CPUs except PowerPC will use 4 KB, while PowerPC and unknown CPUs will use 64 KB (see wtf/PageBlock.h). aarch64 will continue to default to 4 KB because this is a better default on systems where it doesn't crash.

Having one flag will help avoid mistakes. E.g. both RHEL and SUSE were manually passing -DENABLE_JIT=OFF and -DUSE_SYSTEM_MALLOC=ON, but we missed -DENABLE_C_LOOP=ON and -DENABLE_SAMPLING_PROFILER=OFF, so wound up running with both JIT and cloop disabled, a configuration not otherwise used on Linux (and not supported by GTK or WPE ports). It will be easier to not mess up if we only have to pass one special build option.

[Michael Catanzaro]

Created attachment 411919 [details]
Patch

[Michael Catanzaro]

(This will also allow us to stop patching PageBlock.h downstream, because I don't like downstream patches that we have to keep forever.)

[Michael Catanzaro]

Created attachment 411920 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #0)
> Having one flag will help avoid mistakes. E.g. both RHEL and SUSE were
> manually passing -DENABLE_JIT=OFF and -DUSE_SYSTEM_MALLOC=ON, but we missed
> -DENABLE_C_LOOP=ON and -DENABLE_SAMPLING_PROFILER=OFF, so wound up running
> with both JIT and cloop disabled, a configuration not otherwise used on
> Linux (and not supported by GTK or WPE ports). It will be easier to not mess
> up if we only have to pass one special build option.

Any idea why the JSC JIT or bmalloc don't work on Aarch64 with a 64kb pagesize?

[Carlos Alberto Lopez Perez]

Comment on attachment 411920 [details]
Patch

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

> Source/WTF/wtf/PageBlock.h:53
> +#elif USE(64KB_PAGE_BLOCK) || CPU(PPC) || CPU(PPC64) || CPU(PPC64LE) || CPU(UNKNOWN)
> +constexpr size_t CeilingOnPageSize = 64 * KB;

On Source/bmalloc/bmalloc/mbmalloc.cpp I see the variables ConfigAlignment and ConfigSizeToProtect that likely should match this. There is a FIXME for that in such file.

> Source/cmake/WebKitFeatures.cmake:71
> +# there. Most distros (including Fedora) use 4 KB, but RHEL uses 16 KB. SUSE actually supports both.

I guess you mean here 64KB and not 16KB

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #5)
> > Source/WTF/wtf/PageBlock.h:53
> > +#elif USE(64KB_PAGE_BLOCK) || CPU(PPC) || CPU(PPC64) || CPU(PPC64LE) || CPU(UNKNOWN)
> > +constexpr size_t CeilingOnPageSize = 64 * KB;
> 
> On Source/bmalloc/bmalloc/mbmalloc.cpp I see the variables ConfigAlignment
> and ConfigSizeToProtect that likely should match this. There is a FIXME for
> that in such file.

Well there are more places that need tweaked too (see bug #209360), and I don't know how to do it. Even if I did know what was needed, I'd have to figure out how to access a machine to test my changes on (that would be an... adventure).

> > Source/cmake/WebKitFeatures.cmake:71
> > +# there. Most distros (including Fedora) use 4 KB, but RHEL uses 16 KB. SUSE actually supports both.
> 
> I guess you mean here 64KB and not 16KB

Yup, I'll fix that.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> Well there are more places that need tweaked too (see bug #209360), and I
> don't know how to do it. Even if I did know what was needed, I'd have to
> figure out how to access a machine to test my changes on (that would be
> an... adventure).

Oh that's just for bmalloc. For JSC JIT, I don't know what all would need to be changed. (Fixing ConfigSizeToProtect was needed for cloop to work. I really don't know about JIT.)

[Michael Catanzaro]

Created attachment 411938 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Carlos Alberto Lopez Perez from comment #5)
> > > Source/WTF/wtf/PageBlock.h:53
> > > +#elif USE(64KB_PAGE_BLOCK) || CPU(PPC) || CPU(PPC64) || CPU(PPC64LE) || CPU(UNKNOWN)
> > > +constexpr size_t CeilingOnPageSize = 64 * KB;
> > 
> > On Source/bmalloc/bmalloc/mbmalloc.cpp I see the variables ConfigAlignment
> > and ConfigSizeToProtect that likely should match this. There is a FIXME for
> > that in such file.
> 
> Well there are more places that need tweaked too (see bug #209360), and I
> don't know how to do it. Even if I did know what was needed, I'd have to
> figure out how to access a machine to test my changes on (that would be
> an... adventure).
> 

I see.. having access to a test machine with this non-standard configurations is usually the biggest problem

I wonder if you tested to define the pre-processor defines ENABLE_UNIFIED_AND_FREEZABLE_CONFIG_RECORD BENABLE_UNIFIED_AND_FREEZABLE_CONFIG_RECORD? they seem to be guarding this code and seem to be disabled for OS(WINDOWS)

Maybe is a better idea to disable this feature of UNIFIED_AND_FREEZABLE_CONFIG_RECORD than to disable bmalloc or the JSC JIT =?

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #9)
> Maybe is a better idea to disable this feature of
> UNIFIED_AND_FREEZABLE_CONFIG_RECORD than to disable bmalloc or the JSC JIT =?

Well... if we can make it work without crashing, then yes, of course it's better to not disable bmalloc. But again, there will be more work needed in at least Sizes.h and Gigacage.h that is not guarded by BENABLE_UNIFIED_AND_FREEZABLE_CONFIG_RECORD.

If we do make it work, we would still need USE(64KB_PAGE_BLOCK), it just wouldn't need to disable bmalloc or JIT anymore. And we could probably then enable bmalloc for CPU(UNKNOWN). (But JIT would still need to stay disabled for CPU(UNKNOWN).)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> If we do make it work, we would still need USE(64KB_PAGE_BLOCK), it just
> wouldn't need to disable bmalloc or JIT anymore. And we could probably then
> enable bmalloc for CPU(UNKNOWN). (But JIT would still need to stay disabled
> for CPU(UNKNOWN).)

So... I think we should add USE(64KB_PAGE_BLOCK) regardless. OK? :)

[Carlos Alberto Lopez Perez]

Comment on attachment 411938 [details]
Patch

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

> ChangeLog:26
> +        Having one flag will help avoid mistakes. E.g. both RHEL and SUSE were manually passing
> +        -DENABLE_JIT=OFF and -DUSE_SYSTEM_MALLOC=ON, but we missed -DENABLE_C_LOOP=ON and
> +        -DENABLE_SAMPLING_PROFILER=OFF, so wound up running with both JIT and cloop disabled, a
> +        configuration not otherwise used on Linux (and not supported by GTK or WPE ports). It will

I think we should add a check in cmake to ensure that one (and only one) of JIT or C_LOOP is enabled, so that we don't allow building with both options disabled (or with both enabled)

[Michael Catanzaro]

OK, bug #218201.

[Michael Catanzaro]

Comment on attachment 411938 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:73
> +# Since there is no way to guess correctly, the best we can do is provide an option for it. You
> +# should probably only to use this if building for aarch64. Otherwise, known CPUs except PowerPC

Typo to fix before landing: "You should probably only to use this"

> ChangeLog:18
> +        can do is provide an option for it. You should probably only to use this if building for

Typo to fix before landing: "You should probably only to use this"

>> ChangeLog:26
>> +        configuration not otherwise used on Linux (and not supported by GTK or WPE ports). It will
> 
> I think we should add a check in cmake to ensure that one (and only one) of JIT or C_LOOP is enabled, so that we don't allow building with both options disabled (or with both enabled)

Proposed in bug #218201, but we decided not to do this.

[Michael Catanzaro]

Ping reviewers

[Yusuke Suzuki]

Comment on attachment 411938 [details]
Patch

r=me

[Michael Catanzaro]

Thanks.

[Michael Catanzaro]

Created attachment 413198 [details]
Patch

[EWS]

Committed r269396: <https://trac.webkit.org/changeset/269396>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413198 [details].

[Carlos Alberto Lopez Perez]

Comment on attachment 413198 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:86
> -    if (WTF_CPU_ARM64 OR WTF_CPU_X86_64)
> +    if (USE_64KB_PAGE_BLOCK)
> +        set(ENABLE_JIT_DEFAULT OFF)
> +        set(ENABLE_FTL_DEFAULT OFF)
> +        set(USE_SYSTEM_MALLOC_DEFAULT ON)
> +        set(ENABLE_C_LOOP_DEFAULT ON)

I wonder if it would be better to disable C_LOOP for the case of "USE_64KB_PAGE_BLOCK && WTF_CPU_ARM64" ? 
According to https://bugs.webkit.org/show_bug.cgi?id=218201#c6 asm LLInt+no JIT is faster than C_LOOP and it seems well supported on ARM64

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #20)
> I wonder if it would be better to disable C_LOOP for the case of
> "USE_64KB_PAGE_BLOCK && WTF_CPU_ARM64" ? 
> According to https://bugs.webkit.org/show_bug.cgi?id=218201#c6 asm LLInt+no
> JIT is faster than C_LOOP and it seems well supported on ARM64

I'm OK with changing this to whatever Igalia prefers. My inclination would be to lean towards using cloop, since I know you're not testing 64 KB aarch64, and I think you're not testing llint without JIT on any architecture (right?), and cloop ought to work well on every architecture. So I figure the chances of RHEL/SUSE-specific bugs should be lower using cloop than with llint without JIT, right? But if Guij and Xan think it's safe, then OK. Whatever you prefer.

I really do not care one at all about performance. I'd just rather not wind up with crashes or correctness problems that nobody else can reproduce.

[Xan Lopez]

(In reply to Michael Catanzaro from comment #21)
> (In reply to Carlos Alberto Lopez Perez from comment #20)
> > I wonder if it would be better to disable C_LOOP for the case of
> > "USE_64KB_PAGE_BLOCK && WTF_CPU_ARM64" ? 
> > According to https://bugs.webkit.org/show_bug.cgi?id=218201#c6 asm LLInt+no
> > JIT is faster than C_LOOP and it seems well supported on ARM64
> 
> I'm OK with changing this to whatever Igalia prefers. My inclination would
> be to lean towards using cloop, since I know you're not testing 64 KB
> aarch64, and I think you're not testing llint without JIT on any
> architecture (right?), and cloop ought to work well on every architecture.
> So I figure the chances of RHEL/SUSE-specific bugs should be lower using
> cloop than with llint without JIT, right? But if Guij and Xan think it's
> safe, then OK. Whatever you prefer.
> 
> I really do not care one at all about performance. I'd just rather not wind
> up with crashes or correctness problems that nobody else can reproduce.

LLInt on aarch64 is really well tested, I think having or not having JIT should not change things much in that regard (sure, it's a moving part and weird things could happen). In the long term it's also probably better if we test that instead of CLOOP. It is true, though, that going for CLOOP might be slightly safer.

That being said the difference in the end is going to be pretty small, for users in that architecture the deal breaker is not having any working tier above LLInt tbh. So if you are planning to figure that out at some point it might be worth just going for asm LLInt to laid the ground for future improvements.

[Michael Catanzaro]

OK, bug #218613.