File 0503-ets-use-initial-salt-different-from-phash2... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 0503-ets-use-initial-salt-different-from-phash2.patch of Package erlang (Revision 6cc4e2769dd7ada0d41c527b51da0114)

Currently displaying revision 6cc4e2769dd7ada0d41c527b51da0114 , Show latest

From b37a99f5f45eb701602462b8bf3f423d05dca1d2 Mon Sep 17 00:00:00 2001
From: Maxim Fedorov <maximfca@gmail.com>
Date: Fri, 15 Jan 2021 18:52:17 -0800
Subject: [PATCH 1/2] ets: use initial salt different from phash2

It is convenient to use erlang:phash2 to determine frag (ETS table)
to use. If salt is the same, both phash2 and ets hashing generate
the same frag/bucket number.
Since ETS hash is not portable, there is no need to keep backwards
compatibility with previous releases.

Co-authored-by: Sverker Eriksson <sverker@erlang.org>
---
 erts/emulator/beam/utils.c                   |  12 ++-
 erts/emulator/test/map_SUITE.erl             |  12 ++-
 erts/emulator/test/persistent_term_SUITE.erl | 102 ++++++++++++++-----
 lib/stdlib/test/ets_SUITE.erl                |  13 ++-
 4 files changed, 107 insertions(+), 32 deletions(-)

diff --git a/erts/emulator/beam/utils.c b/erts/emulator/beam/utils.c
index afe02f8f13..7df04420da 100644
--- a/erts/emulator/beam/utils.c
+++ b/erts/emulator/beam/utils.c
@@ -2061,7 +2061,7 @@ trapping_make_hash2(Eterm term, Eterm* state_mref_write_back, Process* p)
  * One IMPORTANT property must hold (for hamt).
  * EVERY BIT of the term that is significant for equality (see EQ)
  * MUST BE USED AS INPUT FOR THE HASH. Two different terms must always have a
- * chance of hashing different when salted: hash([Salt|A]) vs hash([Salt|B]).
+ * chance of hashing different when salted.
  *
  * This is why we cannot use cached hash values for atoms for example.
  *
@@ -2073,14 +2073,19 @@ do {  /* Lightweight mixing of constant (type info) */  \
     hash = (hash << 17) ^ (hash >> (32-17));            \
 } while (0)
 
+/*
+ * Start with salt, 32-bit prime number, to avoid getting same hash as phash2
+ * which can cause bad hashing in distributed ETS tables for example.
+ */
+#define INTERNAL_HASH_SALT 3432918353U
+
 Uint32
 make_internal_hash(Eterm term, Uint32 salt)
 {
-    Uint32 hash;
+    Uint32 hash = salt ^ INTERNAL_HASH_SALT;
 
     /* Optimization. Simple cases before declaration of estack. */
     if (primary_tag(term) == TAG_PRIMARY_IMMED1) {
-        hash = salt;
     #if ERTS_SIZEOF_ETERM == 8
         UINT32_HASH_2((Uint32)term, (Uint32)(term >> 32), HCONST);
     #elif ERTS_SIZEOF_ETERM == 4
@@ -2094,7 +2099,6 @@ make_internal_hash(Eterm term, Uint32 salt)
     Eterm tmp;
     DECLARE_ESTACK(s);
 
-    hash = salt;
     for (;;) {
 	switch (primary_tag(term)) {
 	case TAG_PRIMARY_LIST:
diff --git a/erts/emulator/test/map_SUITE.erl b/erts/emulator/test/map_SUITE.erl
index dbf6fa58ed..a2a23d122a 100644
--- a/erts/emulator/test/map_SUITE.erl
+++ b/erts/emulator/test/map_SUITE.erl
@@ -2815,10 +2815,16 @@ hashmap_balance(KeyFun) ->
 	end,
 
     {_,{MaxDiff,MaxMap}} = lists:foldl(F,
-				       {#{}, {0, 0}},
+				       {#{}, {0, undefined}},
 				       lists:seq(1,10000)),
-    io:format("Max std dev diff ~p for map of size ~p (nodes=~p, flatsize=~p)\n",
-	      [MaxDiff, maps:size(MaxMap), hashmap_nodes(MaxMap), erts_debug:flat_size(MaxMap)]),
+    case MaxMap of
+	undefined ->
+	    io:format("Wow, no maps below \"average\"\n", []);
+	_ ->
+	    io:format("Max std dev diff ~p for map of size ~p (nodes=~p, flatsize=~p)\n",
+		      [MaxDiff, maps:size(MaxMap), hashmap_nodes(MaxMap),
+		       erts_debug:flat_size(MaxMap)])
+    end,
 
     true = (MaxDiff < 6),  % The probability of this line failing is about 0.000000001
                            % for a uniform hash. I've set the probability this "high" for now
diff --git a/erts/emulator/test/persistent_term_SUITE.erl b/erts/emulator/test/persistent_term_SUITE.erl
index 15dafaf2e7..e9ace3cd99 100644
--- a/erts/emulator/test/persistent_term_SUITE.erl
+++ b/erts/emulator/test/persistent_term_SUITE.erl
@@ -35,6 +35,10 @@
 %%
 -export([test_init_restart_cmd/1]).
 
+%% Test writing helper
+-export([find_colliding_keys/0]).
+
+
 suite() ->
     [{ct_hooks,[ts_install_cth]},
      {timetrap,{minutes,10}}].
@@ -589,30 +593,28 @@ collisions_delete([], _) ->
     ok.
 
 colliding_keys() ->
-    %% Collisions found by Jesper L. Andersen for breaking maps.
-    L = [[764492191,2361333849],
-         [49527266765044,90940896816021,20062927283041,267080852079651],
-         [249858369443708,206247021789428,20287304470696,25847120931175],
-         [10645228898670,224705626119556,267405565521452,258214397180678],
-         [264783762221048,166955943492306,98802957003141,102012488332476],
-         [69425677456944,177142907243411,137138950917722,228865047699598],
-         [116031213307147,29203342183358,37406949328742,255198080174323],
-         [200358182338308,235207156008390,120922906095920,116215987197289],
-         [58728890318426,68877471005069,176496507286088,221041411345780],
-         [91094120814795,50665258299931,256093108116737,19777509566621],
-         [74646746200247,98350487270564,154448261001199,39881047281135],
-         [23408943649483,164410325820923,248161749770122,274558342231648],
-         [169531547115055,213630535746863,235098262267796,200508473898303],
-         [235098564415817,85039146398174,51721575960328,173069189684390],
-         [176136386396069,155368359051606,147817099696487,265419485459634],
-         [137542881551462,40028925519736,70525669519846,63445773516557],
-         [173854695142814,114282444507812,149945832627054,99605565798831],
-         [177686773562184,127158716984798,132495543008547],
-         [227073396444896,139667311071766,158915951283562],
-         [26212438434289,94902985796531,198145776057315],
-         [266279278943923,58550737262493,74297973216378],
-         [32373606512065,131854353044428,184642643042326],
-         [34335377662439,85341895822066,273492717750246]],
+    %% Collisions found by find_colliding_keys() below
+    L = [[77674392,148027],
+	 [103370644,950908],
+	 [106444046,870178],
+	 [22217246,735880],
+	 [18088843,694607],
+	 [63426007,612179],
+	 [117354942,906431],
+	 [121434305,94282311,816072],
+	 [118441466,93873772,783366],
+	 [124338174,1414801,123089],
+	 [20240282,17113486,923647],
+	 [126495528,61463488,164994],
+	 [125341723,5729072,445539],
+	 [127450932,80442669,348245],
+	 [123354692,85724182,14241288,180793],
+	 [99159367,65959274,61680971,289939],
+	 [107637580,104512101,62639807,181644],
+	 [139547511,51654420,2062545,151944],
+	 [88078274,73031465,53388204,428872],
+	 [141314238,75761379,55699508,861797],
+	 [88045216,59272943,21030492,180903]],
 
     %% Verify that the keys still collide (this will fail if the
     %% internal hash function has been changed).
@@ -636,6 +638,58 @@ verify_colliding_keys([]) ->
 internal_hash(Term) ->
     erts_debug:get_internal_state({internal_hash,Term}).
 
+%% Use this function to (re)generate the list in colliding_keys/0
+find_colliding_keys() ->
+    MaxCollSz = 4,
+    OfEachSz = 7,
+    erts_debug:set_internal_state(available_internal_state, true),
+    MaxInserts = 1 bsl 20,
+    T = ets:new(x, [set, private]),
+    ok = fck_loop_1(T, 1, MaxInserts, MaxCollSz, OfEachSz),
+    fck_collect(T, MaxCollSz, OfEachSz, []).
+
+fck_collect(_T, 1, _OfEachSz, Acc) ->
+    Acc;
+fck_collect(T, CollSz, OfEachSz, Acc) ->
+    {List, _} = ets:select(T,
+			   [{{'$1','$2'}, [{'==',{length,'$2'},CollSz}], ['$2']}],
+			   OfEachSz),
+    fck_collect(T, CollSz-1, OfEachSz, List ++ Acc).
+
+
+fck_loop_1(T, Key, 0, MaxCollSz, MaxSzLeft) ->
+    fck_loop_2(T, Key, MaxCollSz, MaxSzLeft);
+fck_loop_1(T, Key, Inserts, MaxCollSz, MaxSzLeft) ->
+    Hash = internal_hash(Key),
+    case ets:insert_new(T, {Hash, [Key]}) of
+	true ->
+	    fck_loop_1(T, Key+1, Inserts-1, MaxCollSz, MaxSzLeft);
+	false ->
+	    [{Hash, KeyList}] = ets:lookup(T, Hash),
+	    true = ets:insert(T, {Hash, [Key | KeyList]}),
+	    fck_loop_1(T, Key+1, Inserts, MaxCollSz, MaxSzLeft)
+    end.
+
+fck_loop_2(_T, _Key, _MaxCollSz, 0) ->
+    ok;
+fck_loop_2(T, Key, MaxCollSz, MaxSzLeft0) ->
+    Hash = internal_hash(Key),
+    case ets:lookup(T, Hash) of
+	[] ->
+	    fck_loop_2(T, Key+1, MaxCollSz, MaxSzLeft0);
+	[{Hash, KeyList}] ->
+	    true = ets:insert(T, {Hash, [Key | KeyList]}),
+	    MaxSzLeft1 = case length(KeyList)+1 of
+			     MaxCollSz ->
+				 MaxSzLeft0 - 1;
+			     _ ->
+				 MaxSzLeft0
+			 end,
+	    fck_loop_2(T, Key+1, MaxCollSz, MaxSzLeft1)
+    end.
+
+
+
 %% OTP-17700 Bug skipped refc++ of shared magic reference
 shared_magic_ref(_Config) ->
     Ref = atomics:new(10, []),
diff --git a/lib/stdlib/test/ets_SUITE.erl b/lib/stdlib/test/ets_SUITE.erl
index a7dd2341cc..e161470f75 100644
--- a/lib/stdlib/test/ets_SUITE.erl
+++ b/lib/stdlib/test/ets_SUITE.erl
@@ -31,6 +31,7 @@
 -export([match_delete3/1]).
 -export([firstnext/1,firstnext_concurrent/1]).
 -export([slot/1]).
+-export([hash_clash/1]).
 -export([match1/1, match2/1, match_object/1, match_object2/1]).
 -export([dups/1, misc1/1, safe_fixtable/1, info/1, tab2list/1]).
 -export([info_binary_stress/1]).
@@ -132,7 +133,7 @@ suite() ->
 
 all() ->
     [{group, new}, {group, insert}, {group, lookup},
-     {group, delete}, firstnext, firstnext_concurrent, slot,
+     {group, delete}, firstnext, firstnext_concurrent, slot, hash_clash,
      {group, match}, t_match_spec_run,
      {group, lookup_element}, {group, misc}, {group, files},
      {group, heavy}, {group, insert_list}, ordered, ordered_match,
@@ -4563,6 +4564,16 @@ slot_loop(Tab,SlotNo,EltsSoFar) ->
 
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 
+hash_clash(Config) when is_list(Config) ->
+    %% ensure that erlang:phash2 and ets:slot use different hash seed
+    Tab = ets:new(tab, [set]),
+    Buckets = erlang:element(1, ets:info(Tab, stats)),
+    Phash = erlang:phash2(<<"123">>, Buckets),
+    true = ets:insert(Tab, {<<"123">>, "extra"}),
+    [] = ets:slot(Tab, Phash).
+
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+
 
 match1(Config) when is_list(Config) ->
     repeat_for_opts_all_set_table_types(fun match1_do/1).
-- 
2.26.2

Places

File 0503-ets-use-initial-salt-different-from-phash2.patch of Package erlang (Revision 6cc4e2769dd7ada0d41c527b51da0114)

Places