Bug 233097 – [libpas] Build and enable libpas on 64bit JSCOnly

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

[Yusuke Suzuki]

[libpas] build on Linux

[Yusuke Suzuki]

Created attachment 444174 [details]
Patch

[Yusuke Suzuki]

Created attachment 444176 [details]
Patch

[Yusuke Suzuki]

Created attachment 444213 [details]
Patch

[Yusuke Suzuki]

Created attachment 444220 [details]
Patch

[Yusuke Suzuki]

Created attachment 444225 [details]
Patch

[Yusuke Suzuki]

Created attachment 444226 [details]
Patch

[Yusuke Suzuki]

Created attachment 444228 [details]
Patch

[Yusuke Suzuki]

Created attachment 444231 [details]
Patch

[Yusuke Suzuki]

Created attachment 444232 [details]
Patch

[Yusuke Suzuki]

Created attachment 444234 [details]
Patch

[Yusuke Suzuki]

Created attachment 444236 [details]
Patch

[Filip Pizlo]

Comment on attachment 444236 [details]
Patch

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

Wow, this is cool!

Have you checked that the fastMalloc and fastFree fast paths look decent on Linux?

> Source/bmalloc/ChangeLog:14
> +        1. C does not handle `const` variables as constants. So libpas's config is not strictly constant
> +           in the C spec, and GCC actually rejects it. To make it built correctly, we need to build them
> +           in C++. In this patch, when building libpas via CMake, we build some libpas C files as C++.

Lol haha that's too funny.

> Source/bmalloc/libpas/src/libpas/pas_cartesian_tree.h:63
> +    memset(tree->root.payload, 0, PAS_COMPACT_PTR_SIZE);
> +    memset(tree->minimum.payload, 0, PAS_COMPACT_PTR_SIZE);

I would use pas_zero_memory instead of memset if possible.

Even better, I would say: pas_compact_cartesian_tree_node_ptr_store(&tree->root, NULL) and same for minimum.

> Source/bmalloc/libpas/src/libpas/pas_compact_ptr.h:35
> -#define PAS_COMPACT_PTR_INITIALIZER { .payload = {[0 ... PAS_COMPACT_PTR_SIZE - 1] = 0} }
> +#define PAS_COMPACT_PTR_INITIALIZER { .payload = { 0 } }

Does this _really_ zero-fill the whole payload array?

> Source/bmalloc/libpas/src/libpas/pas_internal_config.h:145
> +#if PAS_OS(DARWIN)
> +#define PAS_MADVISE_SYMMETRIC            0
> +#else
> +#define PAS_MADVISE_SYMMETRIC            1
> +#endif

Is it really necessary to do symmetric decommit on Linux?  I thought Linux does fine with asymmetric decommit.

> Source/bmalloc/libpas/src/libpas/pas_large_free_inlines.h:66
> +    } while (1);

i would say while (true).  Even better, I would make this a "for (;;)" loop.  That's how libpas says "loop infinitely" everywhere else.

> Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:77
> +uint64_t pas_get_current_monotonic_time_nanoseconds(void)
> +{
> +    struct timespec ts;
> +    clock_gettime(CLOCK_MONOTONIC, &ts);
> +    return ts.tv_sec * 1.0e9 + ts.tv_nsec;
> +}

I am worried that this creates a huge perf problem on Linux.  I don't think it's necessary to fix it to land your patch, but eventually, we may want to consider having platforms that don't have approximate_time instead using the scavenger's tick as their time.

> Source/bmalloc/libpas/src/libpas/pas_page_malloc.c:246
> +#elif PAS_OS(FREEBSD)
> +    PAS_SYSCALL(madvise((void*)base_as_int, end_as_int - base_as_int, MADV_FREE));
> +#else
> +    PAS_SYSCALL(madvise((void*)base_as_int, end_as_int - base_as_int, MADV_DONTNEED));
> +#if PAS_OS(LINUX)
> +    PAS_SYSCALL(madvise((void*)base_as_int, end_as_int - base_as_int, MADV_DONTDUMP));
> +#endif
> +#endif

Libpas really wants asymmetric decommit.  It works so much better with asymmetric.  The semantics we want are: please decommit this page whenever you like, unless someone writes to it, in which case cancel decommit. If someone writes to it after you decommit it, then zero-fill and recommit it please.

> Source/bmalloc/libpas/src/libpas/pas_utils.h:373
> +#define PAS_PAIR_LOW(pair) ((pair).low)
> +#define PAS_PAIR_HIGH(pair) ((pair).high)

I think this is fine, but I would have made these static inline functions named "pas_pair_low" and "pas_pair_high" unless I absolutely needed to use them in const context.  I'm OK with this, though, it's not a big deal, and it does have the benefit that if we ever did use it in const context, it would Just Work.

[Yusuke Suzuki]

Comment on attachment 444236 [details]
Patch

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

Thanks! Yeah, the generated code for fastMalloc looks sane.

>> Source/bmalloc/libpas/src/libpas/pas_cartesian_tree.h:63
>> +    memset(tree->minimum.payload, 0, PAS_COMPACT_PTR_SIZE);
> 
> I would use pas_zero_memory instead of memset if possible.
> 
> Even better, I would say: pas_compact_cartesian_tree_node_ptr_store(&tree->root, NULL) and same for minimum.

Nice, changed.

>> Source/bmalloc/libpas/src/libpas/pas_compact_ptr.h:35
>> +#define PAS_COMPACT_PTR_INITIALIZER { .payload = { 0 } }
> 
> Does this _really_ zero-fill the whole payload array?

Yes https://stackoverflow.com/questions/201101/how-to-initialize-all-members-of-an-array-to-the-same-value

>> Source/bmalloc/libpas/src/libpas/pas_internal_config.h:145
>> +#endif
> 
> Is it really necessary to do symmetric decommit on Linux?  I thought Linux does fine with asymmetric decommit.

OK, in Linux, we can use MADV_FREE / MADV_DONTNEED. We can make Linux asymmetric too. Changed.

>> Source/bmalloc/libpas/src/libpas/pas_large_free_inlines.h:66
>> +    } while (1);
> 
> i would say while (true).  Even better, I would make this a "for (;;)" loop.  That's how libpas says "loop infinitely" everywhere else.

OK, changed :)

>> Source/bmalloc/libpas/src/libpas/pas_monotonic_time.c:77
>> +}
> 
> I am worried that this creates a huge perf problem on Linux.  I don't think it's necessary to fix it to land your patch, but eventually, we may want to consider having platforms that don't have approximate_time instead using the scavenger's tick as their time.

Yup! I've changed it to CLOCK_MONOTONIC_COARSE, which offers the same thing to mach_approximate_time on Linux.

>> Source/bmalloc/libpas/src/libpas/pas_page_malloc.c:246
>> +#endif
> 
> Libpas really wants asymmetric decommit.  It works so much better with asymmetric.  The semantics we want are: please decommit this page whenever you like, unless someone writes to it, in which case cancel decommit. If someone writes to it after you decommit it, then zero-fill and recommit it please.

Yeah, changed it to use MADV_FREE / MADV_DONTNEED in Linux to make it asymmetric.

>> Source/bmalloc/libpas/src/libpas/pas_utils.h:373
>> +#define PAS_PAIR_HIGH(pair) ((pair).high)
> 
> I think this is fine, but I would have made these static inline functions named "pas_pair_low" and "pas_pair_high" unless I absolutely needed to use them in const context.  I'm OK with this, though, it's not a big deal, and it does have the benefit that if we ever did use it in const context, it would Just Work.

OK, changed it to inline function.

[Yusuke Suzuki]

Committed r285853 (244279@main): <https://commits.webkit.org/244279@main>

[Radar WebKit Bug Importer]

<rdar://problem/85444660>

[Yusuke Suzuki]

Committed r285880 (244306@main): <https://commits.webkit.org/244306@main>

[Yusuke Suzuki]

Committed r286051 (244438@main): <https://commits.webkit.org/244438@main>