From 75fc0b498602e6d11961567a737a9e4163d498b7 Mon Sep 17 00:00:00 2001 From: Sverker Eriksson Date: Fri, 8 Jul 2016 15:37:05 +0200 Subject: erts: Optimize db_finalize_dbterm_hash Always free term after WUNLOCK_HASH --- erts/emulator/beam/erl_db_hash.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/erts/emulator/beam/erl_db_hash.c b/erts/emulator/beam/erl_db_hash.c index fa925c94a5..ccffc2767a 100644 --- a/erts/emulator/beam/erl_db_hash.c +++ b/erts/emulator/beam/erl_db_hash.c @@ -2916,6 +2916,7 @@ db_finalize_dbterm_hash(int cret, DbUpdateHandle* handle) HashDbTerm **bp = (HashDbTerm **) handle->bp; HashDbTerm *b = *bp; erts_smp_rwmtx_t* lck = (erts_smp_rwmtx_t*) handle->lck; + HashDbTerm* free_me = NULL; ERTS_SMP_LC_ASSERT(IS_HASH_WLOCKED(tb, lck)); /* locked by db_lookup_dbterm_hash */ @@ -2927,7 +2928,7 @@ db_finalize_dbterm_hash(int cret, DbUpdateHandle* handle) b->hvalue = INVALID_HASH; } else { *bp = b->next; - free_term(tb, b); + free_me = b; } WUNLOCK_HASH(lck); @@ -2936,12 +2937,15 @@ db_finalize_dbterm_hash(int cret, DbUpdateHandle* handle) } else if (handle->flags & DB_MUST_RESIZE) { db_finalize_resize(handle, offsetof(HashDbTerm,dbterm)); WUNLOCK_HASH(lck); - - free_term(tb, b); + free_me = b; } else { WUNLOCK_HASH(lck); } + + if (free_me) + free_term(tb, free_me); + #ifdef DEBUG handle->dbterm = 0; #endif -- cgit v1.2.1 From 028455fe63ebc8f816c7d63029e5b562e2da5a86 Mon Sep 17 00:00:00 2001 From: Sverker Eriksson Date: Fri, 8 Jul 2016 15:13:35 +0200 Subject: erts: Fix deadlock in ets:update_counter/4 in 'set' with 'write_concurrency' when inserting default object causes table to grow and the bucket to split is protected by same lock as the key. --- erts/emulator/beam/erl_db_hash.c | 34 ++++++++++++++++++---------------- erts/emulator/beam/erl_db_util.h | 1 + 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/erts/emulator/beam/erl_db_hash.c b/erts/emulator/beam/erl_db_hash.c index ccffc2767a..0b31eb3bcd 100644 --- a/erts/emulator/beam/erl_db_hash.c +++ b/erts/emulator/beam/erl_db_hash.c @@ -2869,15 +2869,7 @@ db_lookup_dbterm_hash(Process *p, DbTable *tbl, Eterm key, Eterm obj, q->hvalue = hval; q->next = NULL; *bp = b = q; - - { - int nitems = erts_smp_atomic_inc_read_nob(&tb->common.nitems); - int nactive = NACTIVE(tb); - - if (nitems > nactive * (CHAIN_LEN + 1) && !IS_FIXED(tb)) { - grow(tb, nactive); - } - } + flags |= DB_INC_TRY_GROW; } else { HashDbTerm *q, *next = b->next; @@ -2934,13 +2926,23 @@ db_finalize_dbterm_hash(int cret, DbUpdateHandle* handle) WUNLOCK_HASH(lck); erts_smp_atomic_dec_nob(&tb->common.nitems); try_shrink(tb); - } else if (handle->flags & DB_MUST_RESIZE) { - db_finalize_resize(handle, offsetof(HashDbTerm,dbterm)); - WUNLOCK_HASH(lck); - free_me = b; - } - else { - WUNLOCK_HASH(lck); + } else { + if (handle->flags & DB_MUST_RESIZE) { + db_finalize_resize(handle, offsetof(HashDbTerm,dbterm)); + free_me = b; + } + if (handle->flags & DB_INC_TRY_GROW) { + int nactive; + int nitems = erts_smp_atomic_inc_read_nob(&tb->common.nitems); + WUNLOCK_HASH(lck); + nactive = NACTIVE(tb); + + if (nitems > nactive * (CHAIN_LEN + 1) && !IS_FIXED(tb)) { + grow(tb, nactive); + } + } else { + WUNLOCK_HASH(lck); + } } if (free_me) diff --git a/erts/emulator/beam/erl_db_util.h b/erts/emulator/beam/erl_db_util.h index 0903a40460..bfd81f5d86 100644 --- a/erts/emulator/beam/erl_db_util.h +++ b/erts/emulator/beam/erl_db_util.h @@ -79,6 +79,7 @@ typedef union db_table DbTable; #define DB_MUST_RESIZE 1 #define DB_NEW_OBJECT 2 +#define DB_INC_TRY_GROW 4 /* Info about a database entry while it's being updated * (by update_counter or update_element) -- cgit v1.2.1 From 317013f45039e47296c7ddc5941cf0e5dd0feac2 Mon Sep 17 00:00:00 2001 From: Sverker Eriksson Date: Fri, 8 Jul 2016 14:51:59 +0200 Subject: erts: Add test ets_SUITE:update_counter_table_growth --- lib/stdlib/test/ets_SUITE.erl | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/stdlib/test/ets_SUITE.erl b/lib/stdlib/test/ets_SUITE.erl index 30a158d9e1..1ddc4e7868 100644 --- a/lib/stdlib/test/ets_SUITE.erl +++ b/lib/stdlib/test/ets_SUITE.erl @@ -49,6 +49,7 @@ fixtable_next/1, fixtable_insert/1, rename/1, rename_unnamed/1, evil_rename/1, update_element/1, update_counter/1, evil_update_counter/1, partly_bound/1, match_heavy/1]). -export([update_counter_with_default/1]). +-export([update_counter_table_growth/1]). -export([member/1]). -export([memory/1]). -export([select_fail/1]). @@ -102,6 +103,7 @@ heavy_lookup_element_do/1, member_do/1, otp_5340_do/1, otp_7665_do/1, meta_wb_do/1, do_heavy_concurrent/1, tab2file2_do/2, exit_large_table_owner_do/2, types_do/1, sleeper/0, memory_do/1, update_counter_with_default_do/1, + update_counter_table_growth_do/1, ms_tracee_dummy/1, ms_tracee_dummy/2, ms_tracee_dummy/3, ms_tracee_dummy/4 ]). @@ -141,6 +143,7 @@ all() -> rename, rename_unnamed, evil_rename, update_element, update_counter, evil_update_counter, update_counter_with_default, partly_bound, + update_counter_table_growth, match_heavy, {group, fold}, member, t_delete_object, t_init_table, t_whitebox, t_delete_all_objects, t_insert_list, t_test_ms, t_select_delete, t_ets_dets, @@ -2063,6 +2066,16 @@ update_counter_with_default_do(Opts) -> ok. +update_counter_table_growth(_Config) -> + repeat_for_opts(update_counter_table_growth_do). + +update_counter_table_growth_do(Opts) -> + Set = ets_new(b, [set | Opts]), + [ets:update_counter(Set, N, {2, 1}, {N, 1}) || N <- lists:seq(1,10000)], + OrderedSet = ets_new(b, [ordered_set | Opts]), + [ets:update_counter(OrderedSet, N, {2, 1}, {N, 1}) || N <- lists:seq(1,10000)], + ok. + fixtable_next(doc) -> ["Check that a first-next sequence always works on a fixed table"]; fixtable_next(suite) -> -- cgit v1.2.1