From add636596afecb87e220d31ae75a9ba0b4601fbc Mon Sep 17 00:00:00 2001 From: Qi Wang Date: Thu, 11 Mar 2021 23:41:51 -0800 Subject: [PATCH] Stop checking head state in the merge hook. Now that all merging go through try_acquire_edata_neighbor, the mergeablility checks (including head state checking) are done before reaching the merge hook. In other words, merge hook will never be called if the head state doesn't agree. --- include/jemalloc/internal/ehooks.h | 20 ++++-------------- include/jemalloc/internal/emap.h | 20 ++++++++++++++++++ src/ehooks.c | 34 ++++++++++++++---------------- src/emap.c | 20 ------------------ src/extent.c | 4 ++-- 5 files changed, 42 insertions(+), 56 deletions(-) diff --git a/include/jemalloc/internal/ehooks.h b/include/jemalloc/internal/ehooks.h index bae468b3..064ecf5a 100644 --- a/include/jemalloc/internal/ehooks.h +++ b/include/jemalloc/internal/ehooks.h @@ -61,8 +61,7 @@ bool ehooks_default_split_impl(); bool ehooks_default_merge(extent_hooks_t *extent_hooks, void *addr_a, size_t size_a, void *addr_b, size_t size_b, bool committed, unsigned arena_ind); -bool ehooks_default_merge_impl(tsdn_t *tsdn, void *addr_a, bool head_a, - void *addr_b, bool head_b); +bool ehooks_default_merge_impl(tsdn_t *tsdn, void *addr_a, void *addr_b); void ehooks_default_zero_impl(void *addr, size_t size); /* @@ -338,21 +337,10 @@ ehooks_split(tsdn_t *tsdn, ehooks_t *ehooks, void *addr, size_t size, static inline bool ehooks_merge(tsdn_t *tsdn, ehooks_t *ehooks, void *addr_a, size_t size_a, - bool head_a, void *addr_b, size_t size_b, bool head_b, bool committed) { + void *addr_b, size_t size_b, bool committed) { extent_hooks_t *extent_hooks = ehooks_get_extent_hooks_ptr(ehooks); - /* - * The definition of extent_hooks merge function doesn't know about - * extent head state, but the implementation does. As a result, it - * needs to call iealloc again and walk the rtree. Since the cost of an - * iealloc is large relative to the cost of the default merge hook - * (which on posix-likes is just "return false"), we go even further - * when we short-circuit; we don't just check if the extent hooks - * generally are default, we check if the merge hook specifically is. - */ - if (extent_hooks == &ehooks_default_extent_hooks - || extent_hooks->merge == &ehooks_default_merge) { - return ehooks_default_merge_impl(tsdn, addr_a, head_a, addr_b, - head_b); + if (extent_hooks == &ehooks_default_extent_hooks) { + return ehooks_default_merge_impl(tsdn, addr_a, addr_b); } else if (extent_hooks->merge == NULL) { return true; } else { diff --git a/include/jemalloc/internal/emap.h b/include/jemalloc/internal/emap.h index 5a5dbb6d..364aefac 100644 --- a/include/jemalloc/internal/emap.h +++ b/include/jemalloc/internal/emap.h @@ -224,6 +224,26 @@ extent_assert_can_expand(const edata_t *original, const edata_t *expand) { assert(edata_past_get(original) == edata_base_get(expand)); } +JEMALLOC_ALWAYS_INLINE bool +edata_neighbor_head_state_mergeable(bool edata_is_head, + bool neighbor_is_head, bool forward) { + /* + * Head states checking: disallow merging if the higher addr extent is a + * head extent. This helps preserve first-fit, and more importantly + * makes sure no merge across arenas. + */ + if (forward) { + if (neighbor_is_head) { + return false; + } + } else { + if (edata_is_head) { + return false; + } + } + return true; +} + JEMALLOC_ALWAYS_INLINE edata_t * emap_edata_lookup(tsdn_t *tsdn, emap_t *emap, const void *ptr) { EMAP_DECLARE_RTREE_CTX; diff --git a/src/ehooks.c b/src/ehooks.c index e1815ee8..ca3ca209 100644 --- a/src/ehooks.c +++ b/src/ehooks.c @@ -188,11 +188,10 @@ ehooks_default_split(extent_hooks_t *extent_hooks, void *addr, size_t size, } bool -ehooks_default_merge_impl(tsdn_t *tsdn, void *addr_a, bool head_a, void *addr_b, - bool head_b) { +ehooks_default_merge_impl(tsdn_t *tsdn, void *addr_a, void *addr_b) { assert(addr_a < addr_b); /* - * For non-DSS cases (first 2 branches) -- + * For non-DSS cases -- * a) W/o maps_coalesce, merge is not always allowed (Windows): * 1) w/o retain, never merge (first branch below). * 2) with retain, only merge extents from the same VirtualAlloc @@ -204,17 +203,23 @@ ehooks_default_merge_impl(tsdn_t *tsdn, void *addr_a, bool head_a, void *addr_b, * disallowed if b is a head extent, i.e. no merging across * different mmap regions. * - * a2) and b2) share the implementation (the no_merge_heads branch). + * a2) and b2) are implemented in emap_try_acquire_edata_neighbor, and + * sanity checked in the second branch below. */ if (!maps_coalesce && !opt_retain) { return true; } - /* - * Don't merge across mappings when retain is on -- this preserves - * first-fit ordering. - */ - if (opt_retain && head_b) { - return true; + if (config_debug) { + edata_t *a = emap_edata_lookup(tsdn, &arena_emap_global, + addr_a); + bool head_a = edata_is_head_get(a); + edata_t *b = emap_edata_lookup(tsdn, &arena_emap_global, + addr_b); + bool head_b = edata_is_head_get(b); + emap_assert_mapped(tsdn, &arena_emap_global, a); + emap_assert_mapped(tsdn, &arena_emap_global, b); + assert(edata_neighbor_head_state_mergeable(head_a, head_b, + /* forward */ true)); } if (have_dss && !extent_dss_mergeable(addr_a, addr_b)) { return true; @@ -228,14 +233,7 @@ ehooks_default_merge(extent_hooks_t *extent_hooks, void *addr_a, size_t size_a, void *addr_b, size_t size_b, bool committed, unsigned arena_ind) { tsdn_t *tsdn = tsdn_fetch(); - edata_t *a = emap_edata_lookup(tsdn, &arena_emap_global, addr_a); - bool head_a = edata_is_head_get(a); - edata_t *b = emap_edata_lookup(tsdn, &arena_emap_global, addr_b); - bool head_b = edata_is_head_get(b); - emap_assert_mapped(tsdn, &arena_emap_global, a); - emap_assert_mapped(tsdn, &arena_emap_global, b); - - return ehooks_default_merge_impl(tsdn, addr_a, head_a, addr_b, head_b); + return ehooks_default_merge_impl(tsdn, addr_a, addr_b); } void diff --git a/src/emap.c b/src/emap.c index 949b53e5..0fe230ab 100644 --- a/src/emap.c +++ b/src/emap.c @@ -48,26 +48,6 @@ emap_update_edata_state(tsdn_t *tsdn, emap_t *emap, edata_t *edata, emap_assert_mapped(tsdn, emap, edata); } -static inline bool -edata_neighbor_head_state_mergeable(bool edata_is_head, - bool neighbor_is_head, bool forward) { - /* - * Head states checking: disallow merging if the higher addr extent is a - * head extent. This helps preserve first-fit, and more importantly - * makes sure no merge across arenas. - */ - if (forward) { - if (neighbor_is_head) { - return false; - } - } else { - if (edata_is_head) { - return false; - } - } - return true; -} - static inline bool edata_can_acquire_neighbor(edata_t *edata, rtree_contents_t contents, extent_pai_t pai, extent_state_t expected_state, bool forward, diff --git a/src/extent.c b/src/extent.c index 6d9e0029..1748d98b 100644 --- a/src/extent.c +++ b/src/extent.c @@ -1189,8 +1189,8 @@ extent_merge_impl(tsdn_t *tsdn, pac_t *pac, ehooks_t *ehooks, edata_t *a, emap_assert_mapped(tsdn, pac->emap, b); bool err = ehooks_merge(tsdn, ehooks, edata_base_get(a), - edata_size_get(a), edata_is_head_get(a), edata_base_get(b), - edata_size_get(b), edata_is_head_get(b), edata_committed_get(a)); + edata_size_get(a), edata_base_get(b), edata_size_get(b), + edata_committed_get(a)); if (err) { return true;