[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