[libpas] Refactor PGM to align with libpas allocation norms and split pas_get_random #597

stwrt · 2022-05-12T04:24:02Z

`ff11be4`

[libpas] Refactor PGM to align with libpas allocation norms and split pas_get_random
https://bugs.webkit.org/show_bug.cgi?id=240331

Reviewed by Yusuke Suzuki.

This patch touches a few major areas.

PGM did not properly align with how we returned allocation results with the rest of the code base.
We now use pas_allocation_result instead. This resulted in touching a lot of the PGM testing code.

Added numerous helper utilities to make it easier to check whether we should call into PGM.

Added config option for each heap whether PGM will be enabled or not.

Cleaned up documentation.

I split the pas_get_random into two functions (pas_get_fast_random and pas_get_secure_random).

* Source/bmalloc/libpas/Documentation.md:
* Source/bmalloc/libpas/ReadMe.md:
* Source/bmalloc/libpas/src/libpas/bmalloc_heap_config.h:
* Source/bmalloc/libpas/src/libpas/hotbit_heap_config.h:
* Source/bmalloc/libpas/src/libpas/iso_heap_config.h:
* Source/bmalloc/libpas/src/libpas/iso_test_heap_config.h:
* Source/bmalloc/libpas/src/libpas/minalign32_heap_config.h:
* Source/bmalloc/libpas/src/libpas/pagesize64k_heap_config.h:
* Source/bmalloc/libpas/src/libpas/pas_baseline_allocator_table.c:
(pas_baseline_allocator_table_get_random_index):
* Source/bmalloc/libpas/src/libpas/pas_dynamic_primitive_heap_map.c:
(pas_dynamic_primitive_heap_map_find_slow):
* Source/bmalloc/libpas/src/libpas/pas_heap_config.h:
* Source/bmalloc/libpas/src/libpas/pas_heap_config_utils.h:
* Source/bmalloc/libpas/src/libpas/pas_large_heap.c:
(pas_large_heap_try_allocate_pgm):
* Source/bmalloc/libpas/src/libpas/pas_large_heap.h:
* Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.c:
(pas_probabilistic_guard_malloc_allocate):
(pas_probabilistic_guard_malloc_deallocate):
(pas_probabilistic_guard_malloc_check_exists):
(pas_probabilistic_guard_malloc_get_free_virtual_memory):
(pas_probabilistic_guard_malloc_get_free_wasted_memory):
(pas_probabilistic_guard_malloc_debug_info):
(pas_probabilistic_guard_malloc_trigger): Deleted.
(pas_probabilistic_guard_malloc_can_use): Deleted.
(pas_probabilistic_guard_malloc_should_use): Deleted.
* Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h:
* Source/bmalloc/libpas/src/libpas/pas_random.h:
(pas_get_fast_random):
(pas_get_secure_random):
(pas_get_random): Deleted.
* Source/bmalloc/libpas/src/libpas/pas_segregated_shared_page_directory.c:
(find_first_eligible_consider_view):
* Source/bmalloc/libpas/src/libpas/thingy_heap_config.h:
* Source/bmalloc/libpas/src/test/IsoHeapPartialAndBaselineTests.cpp:
* Source/bmalloc/libpas/src/test/PGMTests.cpp:
(std::testPGMSingleAlloc):
(std::testPGMMultipleAlloc):
(std::testPGMErrors):

Canonical link: https://commits.webkit.org/250997@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294866 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Constellation

OK, some comments :)

Constellation · 2022-05-16T02:17:22Z

Source/bmalloc/libpas/Documentation.md

+
+
+


Let's remove these three more empty lines.

Constellation · 2022-05-16T02:18:11Z

Source/bmalloc/libpas/src/libpas/pas_baseline_allocator_table.c

@@ -63,7 +63,7 @@ void pas_baseline_allocator_table_initialize_if_necessary(void)

 unsigned pas_baseline_allocator_table_get_random_index(void)
 {
-    return pas_get_random(pas_fast_random, PAS_MIN(PAS_NUM_BASELINE_ALLOCATORS,
+    return pas_get_fast_random(PAS_MIN(PAS_NUM_BASELINE_ALLOCATORS,
                                                     pas_baseline_allocator_table_bound));


This indentation looks weird, so let's just remove this indent.

return pas_get_fast_random(PAS_MIN(PAS_NUM_BASELINE_ALLOCATORS, pas_baseline_allocator_table_bound));

Constellation · 2022-05-16T02:19:57Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.c


    // allocate memory
-    pas_allocation_result result = pas_large_heap_try_allocate_and_forget(&heap->large_heap, mem_to_alloc, page_size,
+    result = pas_large_heap_try_allocate_and_forget(large_heap, mem_to_alloc, page_size,
                                                                          heap_config, transaction);


Fix, or remove this indentation.

Constellation · 2022-05-16T02:20:40Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.c

@@ -315,4 +222,34 @@ size_t pas_probabilistic_guard_malloc_get_free_wasted_memory() {
    return free_wasted_mem;
 }

+static PAS_ALWAYS_INLINE void pas_probabilistic_guard_malloc_debug_info(const void *key, const pgm_storage *value, const char *operation) {


Put newline before { of the function (WebKit's coding style).
Looks like functions in this file has this issue, so it is good timing to fix them :)

Constellation · 2022-05-16T20:43:43Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h

+/* We want a fast way to determine if we can call PGM or not.
+ * It would be really wasteful to recompute this answer each time we try to allocate,
+ * so just update this variable each time we allocate or deallocate. */
+extern PAS_API bool pgm_can_use;


If it is exposed as PAS_API, let's put prefix pas_, so pas_can_use_pgm.

Constellation · 2022-05-16T20:44:14Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h

-void* pas_probabilistic_guard_malloc_allocate(size_t size, pas_heap* heap, pas_heap_config* heap_config, pas_physical_memory_transaction* transaction);
+// max amount of virtual memory that can be used by PGM (1GB)
+// including guard pages and wasted memory
+#define PGM_MAX_VIRTUAL_MEMORY (1024 * 1024 * 1024)


Let's put prefix PAS_

Constellation · 2022-05-16T20:44:19Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h

+#define PGM_MAX_VIRTUAL_MEMORY (1024 * 1024 * 1024)
+
+// Probability that we should call PGM in percentage (0-100)
+#define PGM_PROBABILITY (1)


Let's put prefix PAS_

Constellation · 2022-05-16T20:44:21Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h

-bool pas_probabilistic_guard_malloc_can_use(void);
-bool pas_probabilistic_guard_malloc_should_use(void);
+// max amount of free memory that can be wasted (1MB)
+#define PGM_MAX_WASTED_MEMORY (1024 * 1024)


Let's put prefix PAS_

Constellation · 2022-05-16T20:45:00Z

Source/bmalloc/libpas/src/libpas/thingy_heap_config.h

@@ -75,7 +75,8 @@ PAS_BEGIN_EXTERN_C;
    .medium_bitfit_min_align_shift = PAS_MIN_MEDIUM_ALIGN_SHIFT, \
    .use_marge_bitfit = true, \
    .marge_bitfit_min_align_shift = PAS_MIN_MARGE_ALIGN_SHIFT, \
-    .marge_bitfit_page_size = PAS_MARGE_PAGE_DEFAULT_SIZE)
+    .marge_bitfit_page_size = PAS_MARGE_PAGE_DEFAULT_SIZE,       \


Only one space after SIZE,.

Constellation · 2022-05-16T21:06:08Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h

+    size_t allocation_size_requested;
+    size_t size_of_data_pages;
+    size_t mem_to_waste;
+    size_t mem_to_alloc;
+    uintptr_t start_of_data_pages;
+    uintptr_t upper_guard_page;
+    uintptr_t lower_guard_page;


I think only necessary thing in this storage will be allocation_size_requested. The other values are only for debugging purpose or values that can be calculated from pointer & allocation_size_requested.
Because utility heap is precious resource in bmalloc (it is using limited compact heap), in the production version of PGM in libpas, we should not allocate it for each allocation. So, please keep in mind that we need to make this struct just the following in the release build.

struct pas_pgm_storage { size_t allocation_size_requested; };

And then, we do not need to have storage allocation via utility heap since pas_ptr_hash_map can store 64bit value as a value.

Constellation

r=me with nits.

Constellation · 2022-05-19T04:07:56Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.c


-void pas_probabilistic_guard_malloc_trigger(void) {
-    // ???
+    // free memory


Since it is obvious, I think this comment is not necessary.

Constellation · 2022-05-19T04:08:28Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.c

    }

-    return key;
+    result.begin = (uintptr_t) key;


Remove space between (uintptr_t) and key.

Constellation · 2022-05-19T04:09:51Z

Source/bmalloc/libpas/src/libpas/pas_probabilistic_guard_malloc_allocator.h


 PAS_BEGIN_EXTERN_C;

-/* Initial Function Definitions */
+// structure for holding metadata of pgm allocations
+// TODO : Reduce size of structure


Let's rename TODO with FIXME.

webkit-early-warning-system · 2022-05-26T03:47:16Z

Committed r294866 (250997@main): https://commits.webkit.org/250997@main

Reviewed commits have been landed. Closing PR #597 and removing active labels.

stwrt self-assigned this May 12, 2022

stwrt added bmalloc For bugs in bmalloc Other labels May 12, 2022

stwrt requested a review from Constellation May 12, 2022 04:24

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 12, 2022

stwrt removed merging-blocked Applied to prevent a change from being merged bmalloc For bugs in bmalloc Other labels May 14, 2022

stwrt force-pushed the eng/libpas_pgm_refactor branch from ea819b7 to b19765c Compare May 14, 2022 17:26

stwrt added bmalloc For bugs in bmalloc Other labels May 14, 2022

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 14, 2022

Constellation requested changes May 16, 2022

View reviewed changes

stwrt removed merging-blocked Applied to prevent a change from being merged bmalloc For bugs in bmalloc Other labels May 17, 2022

stwrt force-pushed the eng/libpas_pgm_refactor branch from b19765c to 19b4698 Compare May 17, 2022 03:09

stwrt added bmalloc For bugs in bmalloc Other labels May 17, 2022

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 17, 2022

stwrt removed merging-blocked Applied to prevent a change from being merged bmalloc For bugs in bmalloc Other labels May 17, 2022

stwrt force-pushed the eng/libpas_pgm_refactor branch from 19b4698 to 6af9ccf Compare May 17, 2022 17:05

stwrt added bmalloc For bugs in bmalloc Other labels May 17, 2022

stwrt force-pushed the eng/libpas_pgm_refactor branch from 6af9ccf to e3532db Compare May 19, 2022 02:14

Constellation approved these changes May 19, 2022

View reviewed changes

stwrt force-pushed the eng/libpas_pgm_refactor branch 2 times, most recently from e8cb8c4 to e220474 Compare May 22, 2022 05:17

stwrt changed the title ~~[libpas] PGM Refactoring~~ [libpas] Refactor PGM to align with libpas allocation norms May 22, 2022

webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 22, 2022

stwrt removed merging-blocked Applied to prevent a change from being merged bmalloc For bugs in bmalloc Other labels May 26, 2022

stwrt force-pushed the eng/libpas_pgm_refactor branch from e220474 to b821836 Compare May 26, 2022 03:18

stwrt changed the title ~~[libpas] Refactor PGM to align with libpas allocation norms~~ [libpas] Refactor PGM to align with libpas allocation norms and split pas_get_random May 26, 2022

stwrt added bmalloc For bugs in bmalloc Other merge-queue Applied to send a pull request to merge-queue labels May 26, 2022

webkit-early-warning-system closed this May 26, 2022

webkit-early-warning-system force-pushed the eng/libpas_pgm_refactor branch from b821836 to ff11be4 Compare May 26, 2022 03:47

webkit-early-warning-system merged commit ff11be4 into WebKit:main May 26, 2022

webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 26, 2022

stwrt deleted the eng/libpas_pgm_refactor branch July 1, 2022 22:49

[libpas] Refactor PGM to align with libpas allocation norms and split pas_get_random #597

[libpas] Refactor PGM to align with libpas allocation norms and split pas_get_random #597

Uh oh!

Conversation

stwrt commented May 12, 2022 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Constellation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Constellation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented May 26, 2022

Uh oh!

Uh oh!

stwrt commented May 12, 2022 •

edited by webkit-early-warning-system

Loading