[webkit-reviews] review granted: [Bug 233097] [libpas] Build and enable libpas on 64bit JSCOnly : [Attachment 444236] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 15 09:53:21 PST 2021


Filip Pizlo <fpizlo at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 233097: [libpas] Build and enable libpas on 64bit JSCOnly
https://bugs.webkit.org/show_bug.cgi?id=233097

Attachment 444236: Patch

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




--- Comment #12 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 444236
  --> https://bugs.webkit.org/attachment.cgi?id=444236
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.


More information about the webkit-reviews mailing list