checker: Support integer and voidptr key types for maps #7503

ntrel · 2020-12-23T12:56:29Z

Part of #6991.

Allow maps with integer, rune and voidptr keys & implement basic indexing operations for them (see map_test.v additions). I have implemented non-string map literals too but I'll submit that separately for easier review. for and map.clone() also work (and are in the int keys test) but I have a list of other things which need fixing so for now I have added a warning saying non-string keys are WIP.

Add map.has_string_keys field - for now this is true when the key size is bigger than a pointer. When false, keys are assumed to be bitwise comparable (the parser enforces these assumptions).
~~Change DenseArray.clone_key to a map method to access map.has_string_keys (in future it will probably need to access a function pointer field of map).~~
Change DenseArray.push to only make space, not clone key (cloning needs knowledge of the key type).
Fix map.clone to clone keys too.

Update: use function pointers for hash, equal, clone and free key operations.

ntrel · 2020-12-23T13:13:17Z

Unrelated gitly error:

Errors while retrieving meta data for module markdown:
Skipping module "markdown", since https://vpm.vlang.io reported that "markdown" does not exist.

vlib/builtin/map.v

ka-weihe · 2020-12-23T13:35:39Z

vlib/builtin/map.v

+		key := *&string(pkey)
+		hash.wyhash_c(key.str, u64(key.len), 0)
+	} else {
+		hash.wyhash_c(pkey, u64(m.key_bytes), 0)


You should not use wyhash_c for integers. We have wyhash64 in the cheaders, which should be a lot faster.

I think we would need to have more if branches for each value of key_bytes to use wyhash64. E.g. this pull supports byte, i16, int, i64 sizes.

wyhash64(*&u64(pkey), 0) does not work unless key_bytes is 8.

wyhash64(*&u64(pkey), 0) does not work unless key_bytes is 8.

Why can't you do:
wyhash64(u64(*&u32(pkey)), u64(0))

if the hash-function is passed a function pointer, we would just have a hash-function for each integer type.

*&u32(pkey)

That reads 4 bytes from pkey when it may point to only 1, 2 or maybe 8 bytes.

It was only an example for u32 keys. You would have to cast differently for each key type.

ka-weihe · 2020-12-23T13:40:58Z

Nice work. I'm just a bit worried that these changes will hurt performance quite a lot, but I guess it is okay for now - I don't know, not up to me. I will probably improve it in the new year. When this approach is expanded to even more key types, then it will be extremely slow 😢

ntrel · 2020-12-24T11:39:40Z

Rebased.

vlib/v/parser/parse_type.v

ntrel · 2020-12-24T16:57:43Z

I've now added a hash_fn map field - this depends on #7541 for bootstrapping (hence tests failing ATM). This is initialized in new_map, and non-string keys now use wyhash64 as requested.

ka-weihe · 2020-12-25T15:19:15Z

vlib/builtin/map.v

+	// for now assume anything bigger than a pointer is a string
+	has_string_keys := key_bytes > sizeof(voidptr)
+	mut hash_fn := voidptr(0)
+	match key_bytes {


I think it is a lot better now, but this match-statement should be in cgen. This can also be done in another PR.

and we also need function pointers for free, clone and equal

Now added function pointers for those 3 operations.

ka-weihe · 2020-12-25T16:54:25Z

vlib/builtin/map.v

 // The growth-factor is roughly 1.125 `(x + (x >> 3))`
 [inline]
-fn (mut d DenseArray) push(key voidptr, value voidptr) int {
+fn (mut d DenseArray) push() int {


why did you remove the parameters from this method?

nvm, you describe this in #7538, but it shouldn't be named push if it takes no arguments. I'm fine with it being fixed in another PR.

Now renamed to expand.

Add map.has_string_keys field.

Move DenseArray.clone_key to map method to access map.has_string_keys. Change DenseArray.push to only make space, not clone key.

map can't have a field of type function pointer. Also workaround broken function pointer cast.

ntrel · 2020-12-26T12:56:04Z

This pull now depends on #7578 for bootstrap.

ka-weihe · 2020-12-26T13:09:52Z

Good job @ntrel. My only complaint now is that the match-statement is not in cgen, but I'm totally fine with it being done in another PR. I approve.

medvednikov · 2020-12-27T10:21:24Z

@ka-weihe

regarding performance, would simply converting a key to a string be better?

for example m[2] => m['2']

ka-weihe · 2020-12-27T10:45:30Z

@ka-weihe

regarding performance, would simply converting a key to a string be better?

for example m[2] => m['2']

I don't think so. What if the integer is "2147483647", hashing that with the string hash-function would take so much longer compared to the integer hash-function. The same can be said for the equality function. I think what we are doing here is the fastest we can get without generics and that is probably why Go does the same 🙂

medvednikov · 2020-12-27T10:53:43Z

I see, thanks.

medvednikov · 2020-12-27T10:55:00Z

Conflict after merging the other map PR.

This prevents 'conflicting types' errors with some C compilers (even though the types are the same).

/tmp/v/test_session_247731369751/x.10116584592008963799.tmp.c: In function ‘new_map’: /tmp/v/test_session_247731369751/x.10116584592008963799.tmp.c:7428:18: error: ‘_t123’ undeclared (first use in this function); did you mean ‘_t122’? map_free_string* _t123; /* if prepend */ ^~~~~ _t122

ntrel · 2020-12-27T11:47:27Z

@medvednikov Tests green!

Delta456 · 2020-12-27T11:52:32Z

@ntrel Wonderful work!! 🎉 🥳

medvednikov · 2020-12-27T13:18:55Z

Great job, thanks!

ntrel requested a review from ka-weihe December 23, 2020 13:14

ntrel marked this pull request as ready for review December 23, 2020 13:16

ka-weihe reviewed Dec 23, 2020

View reviewed changes

ntrel force-pushed the map-int branch from 86ac9ed to e14c5af Compare December 24, 2020 11:21

Delta456 reviewed Dec 24, 2020

View reviewed changes

vlib/v/parser/parse_type.v Show resolved Hide resolved

This was referenced Dec 24, 2020

map: fix map.clone, refactor, fix map_get_1, map_set_1 non-string keys #7538

Merged

gen: add MapHashFn typedef before map struct is declared #7541

Merged

ka-weihe reviewed Dec 25, 2020

View reviewed changes

ka-weihe closed this Dec 25, 2020

ka-weihe reopened this Dec 25, 2020

ntrel added 15 commits December 26, 2020 10:57

Support non-string key equality, hashing

924aa22

Add map.has_string_keys field.

Support non-string key clone, free

3d3534c

Move DenseArray.clone_key to map method to access map.has_string_keys. Change DenseArray.push to only make space, not clone key.

Fix map.clone to clone keys too

b37a1ca

Fix missing field

8602df9

parser: allow integer and C pointer map key types

6826a38

Don't assume string key for map_get_1, map_set_1

cad22ed

Add test_int_keys

23cf790

warn non-string keys are WIP

13f501d

only allow voidptr pointer keys

b697e5b

Add tests for indexing with invalid key

32a71ec

vfmt

a41276b

Add voidptr key tests

1dd8e4c

list supported types when creating a map with an unsupported key type

c654833

Add map.hash_fn

6a0acc5

Use MapHashFn workaround

de86605

map can't have a field of type function pointer. Also workaround broken function pointer cast.

ntrel added 3 commits December 26, 2020 10:57

hash: add wyhash64_c function and use it for non-string map keys

33ed780

vfmt

ab3409e

Use function pointer for key equality

3f96a8c

ntrel force-pushed the map-int branch from 03fb163 to 3f96a8c Compare December 26, 2020 11:17

ntrel mentioned this pull request Dec 26, 2020

gen: fix return type for MapEqFn and add MapCloneFn, MapFreeFn #7578

Merged

ntrel added 2 commits December 26, 2020 12:49

map: use function pointer to clone keys

3bf54f5

map: use function pointer to free keys

f4cf5b8

remove debugging println (oops)

77a41f0

Rename DenseArray.push to expand; comment bitwise equal; vfmt

cd6b78b

medvednikov closed this Dec 27, 2020

medvednikov reopened this Dec 27, 2020

medvednikov closed this Dec 27, 2020

medvednikov reopened this Dec 27, 2020

ntrel added 2 commits December 27, 2020 11:15

parser: mark MapFooFn typedefs as already declared

0f8543c

This prevents 'conflicting types' errors with some C compilers (even though the types are the same).

medvednikov merged commit e813583 into vlang:master Dec 27, 2020

ntrel deleted the map-int branch December 27, 2020 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker: Support integer and voidptr key types for maps #7503

checker: Support integer and voidptr key types for maps #7503

ntrel commented Dec 23, 2020 •

edited

Loading

ntrel commented Dec 23, 2020

ka-weihe Dec 23, 2020

ntrel Dec 24, 2020 •

edited

Loading

ntrel Dec 24, 2020

ka-weihe Dec 24, 2020 •

edited

Loading

ntrel Dec 24, 2020

ka-weihe Dec 24, 2020 •

edited

Loading

ka-weihe commented Dec 23, 2020 •

edited

Loading

ntrel commented Dec 24, 2020

ntrel commented Dec 24, 2020 •

edited

Loading

ka-weihe Dec 25, 2020 •

edited

Loading

ka-weihe Dec 25, 2020

ntrel Dec 26, 2020

ka-weihe Dec 25, 2020 •

edited

Loading

ka-weihe Dec 25, 2020

ntrel Dec 26, 2020

ntrel commented Dec 26, 2020

ka-weihe commented Dec 26, 2020 •

edited

Loading

medvednikov commented Dec 27, 2020

ka-weihe commented Dec 27, 2020

medvednikov commented Dec 27, 2020

medvednikov commented Dec 27, 2020

ntrel commented Dec 27, 2020

Delta456 commented Dec 27, 2020

medvednikov commented Dec 27, 2020

checker: Support integer and voidptr key types for maps #7503

checker: Support integer and voidptr key types for maps #7503

Conversation

ntrel commented Dec 23, 2020 • edited Loading

ntrel commented Dec 23, 2020

ka-weihe Dec 23, 2020

Choose a reason for hiding this comment

ntrel Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

ntrel Dec 24, 2020

Choose a reason for hiding this comment

ka-weihe Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

ntrel Dec 24, 2020

Choose a reason for hiding this comment

ka-weihe Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

ka-weihe commented Dec 23, 2020 • edited Loading

ntrel commented Dec 24, 2020

ntrel commented Dec 24, 2020 • edited Loading

ka-weihe Dec 25, 2020 • edited Loading

Choose a reason for hiding this comment

ka-weihe Dec 25, 2020

Choose a reason for hiding this comment

ntrel Dec 26, 2020

Choose a reason for hiding this comment

ka-weihe Dec 25, 2020 • edited Loading

Choose a reason for hiding this comment

ka-weihe Dec 25, 2020

Choose a reason for hiding this comment

ntrel Dec 26, 2020

Choose a reason for hiding this comment

ntrel commented Dec 26, 2020

ka-weihe commented Dec 26, 2020 • edited Loading

medvednikov commented Dec 27, 2020

ka-weihe commented Dec 27, 2020

medvednikov commented Dec 27, 2020

medvednikov commented Dec 27, 2020

ntrel commented Dec 27, 2020

Delta456 commented Dec 27, 2020

medvednikov commented Dec 27, 2020

ntrel commented Dec 23, 2020 •

edited

Loading

ntrel Dec 24, 2020 •

edited

Loading

ka-weihe Dec 24, 2020 •

edited

Loading

ka-weihe Dec 24, 2020 •

edited

Loading

ka-weihe commented Dec 23, 2020 •

edited

Loading

ntrel commented Dec 24, 2020 •

edited

Loading

ka-weihe Dec 25, 2020 •

edited

Loading

ka-weihe Dec 25, 2020 •

edited

Loading

ka-weihe commented Dec 26, 2020 •

edited

Loading