Changeset 283178 in webkit

Timestamp:

Sep 28, 2021 9:59:52 AM (3 years ago)

Author:

fpizlo@apple.com

Message:

[libpas] Fix coalescing of the large sharing pool and make it easy to introspect it (update to e4d20851ee9ff00f2962b349a9ff8465695a83d7)
​https://bugs.webkit.org/show_bug.cgi?id=230867

Reviewed by Yusuke Suzuki.

This adds the ability to enable the libpas status reporter, adds a large sharing pool dump to
the status report, and fixes a large sharing pool coalescing bug found by doing that. Previously
we weren't coalescing things that are not free+committed.

Also updates the export script that I use to keep the libpas git repo in sync with what's in WK.

The large sharing pool is the mechanism by which libpas can find memory that can be decommitted
across isolated large heaps, even if those large heaps share pages with one another. The main
data structure is a red-black tree of nodes that represent memory ranges. If there are two
adjacent ranges of memory that are both fully live and committed or both decommitted, then we
want those to be represented using a single node. That wasn't quite working right. Even the
libpas test for this was testing the wrong thing. This fixes the behavior and the test. It's
perf-neutral since large heaps usually have a small number of objects in them anyway.

The new status reporting functionality can be enabled with the WebKitPasStatusReporter
environment variable. This takes an integer that tells the amount of data in the report. Here
are the recognized values:

1 - just report number of heaps
2 - something in between 1 and 3
3 - report everything that the status reporter can report right now (per-page data for

segregated/bitfit heaps, lots of details for large heaps)

If the status reporter ever reported per-object information, it would be at level 4 or higher.
It's safe to pass 9999 or whatever if you just want the maximum report that libpas supports.
TL;DR for now you usually want WebKitPasStatusReporter=3.

• bmalloc/Environment.cpp:

(bmalloc::Environment::Environment):

• libpas/export.rb: Added.
• libpas/export.sh: Removed.
• libpas/src/libpas/pas_bitfit_directory.c:

(pas_bitfit_directory_construct): I needed to rationalize how we initialize disabled directories to make status reporting work.
(pas_bitfit_directory_get_first_free_view):

• libpas/src/libpas/pas_large_sharing_pool.c:

(states_match):

• libpas/src/libpas/pas_status_reporter.c:

(pas_status_reporter_dump_bitfit_directory):
(dump_large_sharing_pool_node_callback):
(pas_status_reporter_dump_large_sharing_pool):
(pas_status_reporter_dump_everything):

• libpas/src/libpas/pas_status_reporter.h:
• libpas/src/test/LargeSharingPoolDump.cpp:
• libpas/src/test/LargeSharingPoolDump.h:
• libpas/src/test/LargeSharingPoolTests.cpp:

(std::Range::Range):
(std::Range::operator== const):
(std::Range::operator!= const):
(std::operator<<):
(std::assertState):
(std::testGoodCoalesceEpochUpdate):
(addLargeSharingPoolTests):
(std::testBadCoalesceEpochUpdate): Deleted.

Location:

trunk/Source/bmalloc

Files:

• ChangeLog (modified) (1 diff)
• bmalloc/Environment.cpp (modified) (2 diffs)
• libpas/export.rb (added)
• libpas/export.sh (deleted)
• libpas/src/libpas/pas_bitfit_directory.c (modified) (4 diffs)
• libpas/src/libpas/pas_large_sharing_pool.c (modified) (1 diff)
• libpas/src/libpas/pas_status_reporter.c (modified) (4 diffs)
• libpas/src/libpas/pas_status_reporter.h (modified) (2 diffs)
• libpas/src/test/LargeSharingPoolDump.cpp (modified) (1 diff)
• libpas/src/test/LargeSharingPoolDump.h (modified) (1 diff)
• libpas/src/test/LargeSharingPoolTests.cpp (modified) (5 diffs)

trunk/Source/bmalloc/ChangeLog

@@ +1 @@
+2021-09-27  Filip Pizlo  <fpizlo@apple.com>
+
+        [libpas] Fix coalescing of the large sharing pool and make it easy to introspect it (update to e4d20851ee9ff00f2962b349a9ff8465695a83d7)
+        https://bugs.webkit.org/show_bug.cgi?id=230867
+
+        Reviewed by Yusuke Suzuki.
+
+        This adds the ability to enable the libpas status reporter, adds a large sharing pool dump to
+        the status report, and fixes a large sharing pool coalescing bug found by doing that. Previously
+        we weren't coalescing things that are not free+committed.
+
+        Also updates the export script that I use to keep the libpas git repo in sync with what's in WK.
+
+        The large sharing pool is the mechanism by which libpas can find memory that can be decommitted
+        across isolated large heaps, even if those large heaps share pages with one another. The main
+        data structure is a red-black tree of nodes that represent memory ranges. If there are two
+        adjacent ranges of memory that are both fully live and committed or both decommitted, then we
+        want those to be represented using a single node. That wasn't quite working right. Even the
+        libpas test for this was testing the wrong thing. This fixes the behavior and the test. It's
+        perf-neutral since large heaps usually have a small number of objects in them anyway.
+
+        The new status reporting functionality can be enabled with the WebKitPasStatusReporter
+        environment variable. This takes an integer that tells the amount of data in the report. Here
+        are the recognized values:
+
+        1 - just report number of heaps
+        2 - something in between 1 and 3
+        3 - report everything that the status reporter can report right now (per-page data for
+            segregated/bitfit heaps, lots of details for large heaps)
+
+        If the status reporter ever reported per-object information, it would be at level 4 or higher.
+        It's safe to pass 9999 or whatever if you just want the maximum report that libpas supports.
+        TL;DR for now you usually want WebKitPasStatusReporter=3.
+
+        * bmalloc/Environment.cpp:
+        (bmalloc::Environment::Environment):
+        * libpas/export.rb: Added.
+        * libpas/export.sh: Removed.
+        * libpas/src/libpas/pas_bitfit_directory.c:
+        (pas_bitfit_directory_construct): I needed to rationalize how we initialize disabled directories to make status reporting work.
+        (pas_bitfit_directory_get_first_free_view):
+        * libpas/src/libpas/pas_large_sharing_pool.c:
+        (states_match):
+        * libpas/src/libpas/pas_status_reporter.c:
+        (pas_status_reporter_dump_bitfit_directory):
+        (dump_large_sharing_pool_node_callback):
+        (pas_status_reporter_dump_large_sharing_pool):
+        (pas_status_reporter_dump_everything):
+        * libpas/src/libpas/pas_status_reporter.h:
+        * libpas/src/test/LargeSharingPoolDump.cpp:
+        * libpas/src/test/LargeSharingPoolDump.h:
+        * libpas/src/test/LargeSharingPoolTests.cpp:
+        (std::Range::Range):
+        (std::Range::operator== const):
+        (std::Range::operator!= const):
+        (std::operator<<):
+        (std::assertState):
+        (std::testGoodCoalesceEpochUpdate):
+        (addLargeSharingPoolTests):
+        (std::testBadCoalesceEpochUpdate): Deleted.
+
 2021-09-22  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/bmalloc/bmalloc/Environment.cpp

@@ -48 +48 @@
 int malloc_engaged_nano(void);
 }
+#endif
+
+#if BUSE(LIBPAS)
+#include "pas_status_reporter.h"
 #endif
 
@@ -137 +141 @@
     : m_isDebugHeapEnabled(computeIsDebugHeapEnabled())
 {
+#if BUSE(LIBPAS)
+    const char* statusReporter = getenv("WebKitPasStatusReporter");
+    if (statusReporter) {
+        unsigned enabled;
+        if (sscanf(statusReporter, "%u", &enabled) == 1)
+            pas_status_reporter_enabled = enabled;
+    }
+#endif
 }
 

trunk/Source/bmalloc/libpas/src/libpas/pas_bitfit_directory.c

@@ -47 +47 @@
     static const bool verbose = false;
 
+    /* NOTE - this works even if the config is disabled, and produces a directory that is empty and
+       does nothing. This makes sense because it makes it easy to iterate over the directories in a heap
+       without knowing what your config is. */
+
     if (verbose)
         pas_log("Creating directory %p\n", directory);
@@ -55 +59 @@
     pas_bitfit_directory_view_vector_construct(&directory->views);
     pas_compact_atomic_bitfit_size_class_ptr_store(&directory->largest_size_class, NULL);
-    directory->config_kind = config->kind;
+    directory->config_kind = config->base.is_enabled ? config->kind : pas_bitfit_page_config_kind_null;
 
     directory->heap = heap;
@@ -63 +67 @@
     /* We could have been lazy about this - but it's probably not super necessary since the point
        of bitfit global directories is that there won't be too many of them. */
-    if (pas_bitfit_directory_does_sharing(directory)) {
+    if (config->base.is_enabled && pas_bitfit_directory_does_sharing(directory)) {
         pas_page_sharing_pool_add(
             &pas_physical_page_sharing_pool,
@@ -103 +107 @@
 {
     static const bool verbose = false;
+
+    PAS_ASSERT(page_config->base.is_enabled);
     
     for (;;) {

trunk/Source/bmalloc/libpas/src/libpas/pas_large_sharing_pool.c

@@ -288 +288 @@
                          pas_large_sharing_node* right)
 {
-    return left->is_committed == right->is_committed
-        && left->synchronization_style == right->synchronization_style
-        && ((!left->num_live_bytes && !right->num_live_bytes) ||
-            (pas_range_size(left->range) == left->num_live_bytes &&
-             pas_range_size(right->range) == right->num_live_bytes))
-        && (left->use_epoch == right->use_epoch
-            || (!left->is_committed && !left->num_live_bytes));
+    bool both_empty;
+    bool both_full;
+    
+    if (left->is_committed != right->is_committed)
+        return false;
+
+    if (left->synchronization_style != right->synchronization_style)
+        return false;
+
+    both_empty =
+        !left->num_live_bytes &&
+        !right->num_live_bytes;
+    
+    both_full =
+        pas_range_size(left->range) == left->num_live_bytes &&
+        pas_range_size(right->range) == right->num_live_bytes;
+
+    if (!both_empty && !both_full)
+        return false;
+
+    /* Right now: both sides have identical commit states and identical synchronization styes. And
+       either both sides are empty or both sides are full.
+    
+       The only reason why we wouldn't want to coalesce is if epochs didn't match. But that only
+       matters when the memory is free and committed. */
+
+    if (!left->is_committed)
+        return true;
+
+    if (both_full)
+        return true;
+
+    return left->use_epoch == right->use_epoch;
 }
 

trunk/Source/bmalloc/libpas/src/libpas/pas_status_reporter.c

@@ -46 +46 @@
 #include "pas_large_heap.h"
 #include "pas_large_map.h"
+#include "pas_large_sharing_pool.h"
 #include "pas_large_utility_free_heap.h"
 #include "pas_log.h"
@@ -197 +198 @@
     pas_stream* stream, pas_bitfit_directory* directory)
 {
+    if (directory->config_kind == pas_bitfit_page_config_kind_null)
+        return;
+    
     pas_stream_printf(stream, "            %s Global Dir (%p): ",
                       pas_bitfit_page_config_variant_get_capitalized_string(
@@ -643 +647 @@
     pas_heap_summary_dump(pas_all_heaps_compute_total_non_utility_large_summary(), stream);
     pas_stream_printf(stream, "\n");
+}
+
+static bool dump_large_sharing_pool_node_callback(pas_large_sharing_node* node,
+                                                  void* arg)
+{
+    pas_stream* stream;
+
+    stream = arg;
+
+    pas_stream_printf(stream, "        %p...%p: %s, %zu/%zu live (%.0lf%%), %llu",
+                      (void*)node->range.begin,
+                      (void*)node->range.end,
+                      pas_commit_mode_get_string(node->is_committed),
+                      node->num_live_bytes,
+                      pas_range_size(node->range),
+                      100. * (double)node->num_live_bytes / (double)pas_range_size(node->range),
+                      node->use_epoch);
+
+    if (node->synchronization_style != pas_physical_memory_is_locked_by_virtual_range_common_lock) {
+        pas_stream_printf(
+            stream, ", %s",
+            pas_physical_memory_synchronization_style_get_string(node->synchronization_style));
+    }
+
+    pas_stream_printf(stream, "\n");
+
+    return true;
+}
+
+void pas_status_reporter_dump_large_sharing_pool(pas_stream* stream)
+{
+    pas_stream_printf(stream, "    Large sharing pool contents:\n");
+    pas_large_sharing_pool_for_each(dump_large_sharing_pool_node_callback, stream, pas_lock_is_held);
 }
 
@@ -959 +996 @@
     pas_status_reporter_dump_all_shared_page_directories(stream);
     pas_status_reporter_dump_all_heaps_non_utility_summaries(stream);
+    
+    if (pas_status_reporter_enabled >= 3)
+        pas_status_reporter_dump_large_sharing_pool(stream);
+    
     pas_status_reporter_dump_utility_heap(stream);
 

trunk/Source/bmalloc/libpas/src/libpas/pas_status_reporter.h

@@ -1 +1 @@
 /*
- * Copyright (c) 2019-2020 Apple Inc. All rights reserved.
+ * Copyright (c) 2019-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -69 +69 @@
 PAS_API void pas_status_reporter_dump_all_shared_page_directories(pas_stream* stream);
 PAS_API void pas_status_reporter_dump_all_heaps_non_utility_summaries(pas_stream* stream);
+PAS_API void pas_status_reporter_dump_large_sharing_pool(pas_stream* stream);
 PAS_API void pas_status_reporter_dump_utility_heap(pas_stream* stream);
 PAS_API void pas_status_reporter_dump_total_fragmentation(pas_stream* stream);

trunk/Source/bmalloc/libpas/src/test/LargeSharingPoolDump.cpp

@@ -50 +50 @@
 }
 
+vector<pas_large_sharing_node*> largeSharingPoolAsVector()
+{
+    vector<pas_large_sharing_node*> result;
+    forEachLargeSharingPoolNode([&] (pas_large_sharing_node* node) -> bool {
+        result.push_back(node);
+        return true;
+    });
+    return result;
+}
+
 void dumpLargeSharingPool()
 {

trunk/Source/bmalloc/libpas/src/test/LargeSharingPoolDump.h

@@ -32 +32 @@
 #include <functional>
 #include "pas_large_sharing_pool.h"
+#include <vector>
 
 void forEachLargeSharingPoolNode(std::function<bool(pas_large_sharing_node*)> visitor);
+std::vector<pas_large_sharing_node*> largeSharingPoolAsVector();
 void dumpLargeSharingPool();
 

trunk/Source/bmalloc/libpas/src/test/LargeSharingPoolTests.cpp

@@ -1 +1 @@
 /*
- * Copyright (c) 2018-2019 Apple Inc. All rights reserved.
+ * Copyright (c) 2018-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -57 +57 @@
     {
     }
+
+    explicit Range(pas_large_sharing_node* node)
+        : begin(node->range.begin)
+        , end(node->range.end)
+        , isCommitted(node->is_committed)
+        , numLiveBytes(node->num_live_bytes)
+        , epoch(node->use_epoch)
+    {
+    }
+
+    bool operator==(Range other) const
+    {
+        return begin == other.begin
+            && end == other.end
+            && isCommitted == other.isCommitted
+            && numLiveBytes == other.numLiveBytes
+            && epoch == other.epoch;
+    }
+
+    bool operator!=(Range other) const { return !(*this == other); }
     
     uintptr_t begin;
@@ -65 +85 @@
 };
 
+ostream& operator<<(ostream& out, Range range)
+{
+    out << reinterpret_cast<void*>(range.begin) << "..." << reinterpret_cast<void*>(range.end)
+        << ", " << (range.isCommitted ? "committed" : "decommitted") << ", "
+        << range.numLiveBytes << "/" << (range.end - range.begin) << ", " << range.epoch;
+    return out;
+}
+
 void assertState(const vector<Range>& ranges)
 {
-    size_t index = 0;
-    forEachLargeSharingPoolNode(
-        [&] (pas_large_sharing_node* node) -> bool {
-            CHECK_LESS(index, ranges.size());
-            Range expectedRange = ranges[index++];
-            CHECK_EQUAL(node->range.begin, expectedRange.begin);
-            CHECK_EQUAL(node->range.end, expectedRange.end);
-            CHECK_EQUAL(node->is_committed, expectedRange.isCommitted);
-            CHECK_EQUAL(node->num_live_bytes, expectedRange.numLiveBytes);
-            CHECK_EQUAL(node->use_epoch, expectedRange.epoch);
-            return true;
-        });
-    CHECK_EQUAL(index, ranges.size());
+    vector<pas_large_sharing_node*> nodes = largeSharingPoolAsVector();
+    
+    bool allGood = true;
+
+    if (nodes.size() != ranges.size()) {
+        cout << "State does not match because we expected " << ranges.size() << " ranges but got "
+             << nodes.size() << " ranges.\n";
+        allGood = false;
+    } else {
+        for (size_t index = 0; index < nodes.size(); ++index) {
+            pas_large_sharing_node* node = nodes[index];
+            Range actualRange(node);
+            Range expectedRange = ranges[index];
+            if (expectedRange != actualRange) {
+                cout << "State does not match at index " << index << ": expected:\n"
+                     << "    " << expectedRange << ", but got:\n"
+                     << "    " << actualRange << "\n";
+                allGood = false;
+            }
+        }
+    }
+
+    if (!allGood) {
+        cout << "Got mismatch in states. Expected the state to be:\n";
+        for (Range range : ranges)
+            cout << "    " << range << "\n";
+        cout << "But got:\n";
+        for (pas_large_sharing_node* node : nodes)
+            cout << "    " << Range(node) << "\n";
+    }
+
+    CHECK(allGood);
 }
 
-void testBadCoalesceEpochUpdate()
+void testGoodCoalesceEpochUpdate()
 {
     static constexpr bool verbose = false;
@@ -124 +171 @@
         dumpLargeSharingPool();
 
-    assertState({ Range(0, 10 * PG, pas_committed, 10 * PG, 0),
-                  Range(10 * PG, 30 * PG, pas_committed, 20 * PG, 3),
-                  Range(30 * PG, END, pas_committed, END - 30 * PG, 0) });
-    
+    assertState({ Range(0, END, pas_committed, END, 3) });
 }
 
@@ -139 +183 @@
     EpochIsCounter epochIsCounter;
     
-    ADD_TEST(testBadCoalesceEpochUpdate());
+    ADD_TEST(testGoodCoalesceEpochUpdate());
 #endif // TLC
 }