Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime maps: random iteration and maps.clone #4476

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Sep 18, 2024

This PR fixes a few map-related issues:

  • make map iteration less predictable. It's still not a fully random key, but then again neither is Big Go's.
  • fix building the tsip runtime hash. At some point this broken when an import cycle was introduced, so just inline the bits we need from math/bits and encoding/binary.
  • add maps.clone so the maps package can use it.
  • initialize fastrand() on process startup so we actually get random numbers for map seeds
  • fix build for tsip memhash

@dgryski dgryski changed the title runtime: make map iteration less defined runtime: add unspecified map iteration and maps.clone Sep 18, 2024
@dgryski dgryski changed the title runtime: add unspecified map iteration and maps.clone runtime maps: random iteration and maps.clone Sep 18, 2024
@dgryski dgryski requested a review from aykevl September 18, 2024 23:41
src/runtime/algorithm.go Outdated Show resolved Hide resolved
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it ironic that big Go said map iteration order is undefined, so made map iteration random, and now there's a lot of code depending on the random iteration order.
(There's a difference between random and undefined! Always producing the map keys in sorted order would be entirely valid according to the Go spec).

Anyway, see my comment below. Didn't investigate the code that changes the iteration order in detail, but it seems reasonable on the surface.

src/runtime/memhash_tsip.go Show resolved Hide resolved
Comment on lines +290 to +295
func hashmapClone(intf _interface) _interface {
typ, val := decomposeInterface(intf)
m := (*hashmap)(val)
n := hashmapCopy(m, m.bucketBits)
return composeInterface(typ, unsafe.Pointer(&n))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I didn't think of reusing the growing code for map cloning.

src/runtime/algorithm.go Outdated Show resolved Hide resolved
builder/sizes_test.go Outdated Show resolved Hide resolved
@dgryski dgryski force-pushed the dgryski/map-random-iteration branch 2 times, most recently from fb43c1d to cbb87af Compare September 25, 2024 22:43
@dgryski dgryski force-pushed the dgryski/map-random-iteration branch 2 times, most recently from 03c03fe to 10872fc Compare September 29, 2024 16:34
@dgryski
Copy link
Member Author

dgryski commented Sep 29, 2024

Need to fix the binary sizes again ..

@aykevl aykevl merged commit 9d14489 into tinygo-org:dev Oct 2, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants