From 3d6b8e7fb68543713f1620a45b8a590ef4ed88a5 Mon Sep 17 00:00:00 2001 From: Guilherme Andrade Date: Mon, 24 Apr 2017 22:51:49 +0100 Subject: erts: Discontinue salted use of enif_hash/phash2 --- erts/doc/src/erl_nif.xml | 8 ++++---- erts/emulator/beam/bif.c | 4 ++-- erts/emulator/beam/erl_nif.c | 6 +++++- erts/emulator/beam/erl_utils.h | 2 +- erts/emulator/beam/utils.c | 30 +++++++++++++++--------------- erts/emulator/hipe/hipe_bif0.c | 2 +- erts/emulator/test/nif_SUITE.erl | 13 ++++--------- 7 files changed, 32 insertions(+), 33 deletions(-) diff --git a/erts/doc/src/erl_nif.xml b/erts/doc/src/erl_nif.xml index 534c90cbdc..05b519fe7d 100644 --- a/erts/doc/src/erl_nif.xml +++ b/erts/doc/src/erl_nif.xml @@ -829,10 +829,10 @@ typedef enum {

Portable hash function that gives the same hash for the same Erlang term regardless of machine architecture and ERTS version.

-

It takes 32-bit salt values and generates hashes within 0..2^27-1.

+

It ignores salt values and generates hashes within 0..2^27-1.

Slower than ERL_NIF_INTERNAL_HASH. - When used with salt=0, it corresponds to - erlang:phash2/1.

+ It corresponds to erlang:phash2/1. +

@@ -1418,7 +1418,7 @@ typedef enum {

Hashes term according to the specified ErlNifHash type.

-

Ranges of taken salt and returned value depend on the hash type.

+

Ranges of taken salt (if any) and returned value depend on the hash type.

 diff --git a/erts/emulator/beam/bif.c b/erts/emulator/beam/bif.c index 7473a8e43f..214de3652f 100644 --- a/erts/emulator/beam/bif.c +++ b/erts/emulator/beam/bif.c @@ -4881,7 +4881,7 @@ BIF_RETTYPE phash2_1(BIF_ALIST_1) { Uint32 hash; - hash = make_hash2(BIF_ARG_1, 0); + hash = make_hash2(BIF_ARG_1); BIF_RET(make_small(hash & ((1L << 27) - 1))); } @@ -4901,7 +4901,7 @@ BIF_RETTYPE phash2_2(BIF_ALIST_2) } range = (Uint32) u; } - hash = make_hash2(BIF_ARG_1, 0); + hash = make_hash2(BIF_ARG_1); if (range) { final_hash = hash % range; /* [0..range-1] */ } else { diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c index 3ff6c8d3ed..e101747011 100644 --- a/erts/emulator/beam/erl_nif.c +++ b/erts/emulator/beam/erl_nif.c @@ -1220,7 +1220,11 @@ ErlNifUInt64 enif_hash(ErlNifHash type, Eterm term, ErlNifUInt64 salt) case ERL_NIF_INTERNAL_HASH: return make_internal_hash(term, (Uint32) salt); case ERL_NIF_PHASH2: - return make_hash2(term, (Uint32) salt) & ((1 << 27) - 1); + /* It appears that make_hash2 doesn't always react to seasoning + * as well as it should. Therefore, let's make it ignore the salt + * value and declare salted uses of phash2 as unsupported. + */ + return make_hash2(term) & ((1 << 27) - 1); default: return 0; } diff --git a/erts/emulator/beam/erl_utils.h b/erts/emulator/beam/erl_utils.h index b38beaf93d..75d7e47239 100644 --- a/erts/emulator/beam/erl_utils.h +++ b/erts/emulator/beam/erl_utils.h @@ -118,7 +118,7 @@ int erts_fit_in_bits_uint(Uint); Sint erts_list_length(Eterm); int erts_is_builtin(Eterm, Eterm, int); Uint32 block_hash(byte *, unsigned, Uint32); -Uint32 make_hash2(Eterm, Uint32 salt); +Uint32 make_hash2(Eterm); Uint32 make_hash(Eterm); Uint32 make_internal_hash(Eterm, Uint32 salt); diff --git a/erts/emulator/beam/utils.c b/erts/emulator/beam/utils.c index 2378c057dd..9d140470ac 100644 --- a/erts/emulator/beam/utils.c +++ b/erts/emulator/beam/utils.c @@ -1003,7 +1003,7 @@ tail_recur: break; } case MAP_DEF: - hash = hash*FUNNY_NUMBER13 + FUNNY_NUMBER14 + make_hash2(term, 0); + hash = hash*FUNNY_NUMBER13 + FUNNY_NUMBER14 + make_hash2(term); break; case TUPLE_DEF: { @@ -1109,7 +1109,7 @@ block_hash(byte *k, unsigned length, Uint32 initval) } Uint32 -make_hash2(Eterm term, Uint32 salt) +make_hash2(Eterm term) { Uint32 hash; Uint32 hash_xor_pairs; @@ -1179,7 +1179,7 @@ make_hash2(Eterm term, Uint32 salt) switch (term & _TAG_IMMED2_MASK) { case _TAG_IMMED2_ATOM: /* Fast, but the poor hash value should be mixed. */ - return atom_tab(atom_val(term))->slot.bucket.hvalue ^ salt; + return atom_tab(atom_val(term))->slot.bucket.hvalue; } break; case _TAG_IMMED1_SMALL: @@ -1190,7 +1190,7 @@ make_hash2(Eterm term, Uint32 salt) term = small_to_big(x, tmp_big); break; } - hash = salt; + hash = 0; SINT32_HASH(x, HCONST); return hash; } @@ -1201,7 +1201,7 @@ make_hash2(Eterm term, Uint32 salt) DECLARE_ESTACK(s); UseTmpHeapNoproc(2); - hash = salt; + hash = 0; for (;;) { switch (primary_tag(term)) { case TAG_PRIMARY_LIST: @@ -1277,7 +1277,7 @@ make_hash2(Eterm term, Uint32 salt) ESTACK_PUSH(s, hash_xor_pairs); ESTACK_PUSH(s, hash); ESTACK_PUSH(s, HASH_MAP_TAIL); - hash = salt; + hash = 0; hash_xor_pairs = 0; for (i = size - 1; i >= 0; i--) { ESTACK_PUSH(s, HASH_MAP_PAIR); @@ -1296,7 +1296,7 @@ make_hash2(Eterm term, Uint32 salt) ESTACK_PUSH(s, hash_xor_pairs); ESTACK_PUSH(s, hash); ESTACK_PUSH(s, HASH_MAP_TAIL); - hash = salt; + hash = 0; hash_xor_pairs = 0; } switch (hdr & _HEADER_MAP_SUBTAG_MASK) { @@ -1471,7 +1471,7 @@ make_hash2(Eterm term, Uint32 salt) break; default: - erts_exit(ERTS_ERROR_EXIT, "Invalid tag in make_hash2(0x%X, %lu)\n", term, salt); + erts_exit(ERTS_ERROR_EXIT, "Invalid tag in make_hash2(0x%X)\n", term); } } break; @@ -1488,21 +1488,21 @@ make_hash2(Eterm term, Uint32 salt) case _TAG_IMMED1_IMMED2: switch (term & _TAG_IMMED2_MASK) { case _TAG_IMMED2_ATOM: - if (hash == salt) + if (hash == 0) /* Fast, but the poor hash value should be mixed. */ - hash = atom_tab(atom_val(term))->slot.bucket.hvalue ^ salt; + hash = atom_tab(atom_val(term))->slot.bucket.hvalue; else UINT32_HASH(atom_tab(atom_val(term))->slot.bucket.hvalue, HCONST_3); goto hash2_common; case _TAG_IMMED2_NIL: - if (hash == salt) - hash = 3468870702UL ^ salt; + if (hash == 0) + hash = 3468870702UL; else UINT32_HASH(NIL_DEF, HCONST_2); goto hash2_common; default: - erts_exit(ERTS_ERROR_EXIT, "Invalid tag in make_hash2(0x%X, %lu)\n", term, salt); + erts_exit(ERTS_ERROR_EXIT, "Invalid tag in make_hash2(0x%X)\n", term); } case _TAG_IMMED1_SMALL: { @@ -1518,7 +1518,7 @@ make_hash2(Eterm term, Uint32 salt) } break; default: - erts_exit(ERTS_ERROR_EXIT, "Invalid tag in make_hash2(0x%X, %lu)\n", term, salt); + erts_exit(ERTS_ERROR_EXIT, "Invalid tag in make_hash2(0x%X)\n", term); hash2_common: /* Uint32 hash always has the hash value of the previous term, @@ -1542,7 +1542,7 @@ make_hash2(Eterm term, Uint32 salt) } case HASH_MAP_PAIR: hash_xor_pairs ^= hash; - hash = salt; + hash = 0; goto hash2_common; default: break; diff --git a/erts/emulator/hipe/hipe_bif0.c b/erts/emulator/hipe/hipe_bif0.c index b7297043e5..8b420b9e9b 100644 --- a/erts/emulator/hipe/hipe_bif0.c +++ b/erts/emulator/hipe/hipe_bif0.c @@ -496,7 +496,7 @@ static ErlOffHeap const_term_table_off_heap; static HashValue const_term_hash(void *tmpl) { - return make_hash2((Eterm)tmpl, 0); + return make_hash2((Eterm)tmpl); } static int const_term_cmp(void *tmpl, void *bucket) diff --git a/erts/emulator/test/nif_SUITE.erl b/erts/emulator/test/nif_SUITE.erl index 6e4866bfd8..8ad11d3bf3 100644 --- a/erts/emulator/test/nif_SUITE.erl +++ b/erts/emulator/test/nif_SUITE.erl @@ -59,8 +59,7 @@ nif_snprintf/1, nif_internal_hash/1, nif_internal_hash_salted/1, - nif_phash2/1, - nif_phash2_salted/1 + nif_phash2/1 ]). -export([many_args_100/100]). @@ -97,8 +96,7 @@ all() -> nif_snprintf, nif_internal_hash, nif_internal_hash_salted, - nif_phash2, - nif_phash2_salted]. + nif_phash2]. groups() -> [{G, [], api_repeaters()} || G <- api_groups()] @@ -2637,7 +2635,8 @@ nif_phash2(Config) -> lists:map( fun (Term) -> HashValue = erlang:phash2(Term), - NifHashValue = hash_nif(phash2, Term, 0), + Salt = random_uint32(), % phash2 should ignore salt + NifHashValue = hash_nif(phash2, Term, Salt), (HashValue =:= NifHashValue orelse ct:fail("Expected: ~p\nActual: ~p", [HashValue, NifHashValue])), @@ -2646,10 +2645,6 @@ nif_phash2(Config) -> Terms), test_bit_distribution_fitness(HashValues, HashValueBitSize, 0.05). -nif_phash2_salted(Config) -> - ensure_lib_loaded(Config), - test_salted_nif_hash(phash2). - test_salted_nif_hash(HashType) -> HashValueBitSize = nif_hash_result_bitsize(HashType), Terms = unique([random_term() || _ <- lists:seq(1, 5000)]), -- cgit v1.2.3