At this point, maps cannot be accessed by index, but their size can be acquired, and they can be iterated.

Using the same benchmark I used to report the slowness (with 1000 entries):

goos: linux
goarch: js
BenchmarkSliceLen           	1000000000	         0.2530 ns/op
BenchmarkMapLen             	1000000000	         0.5040 ns/op
BenchmarkMapNilCheck        	1000000000	         0.5050 ns/op
BenchmarkMapNilElementCheck 	144578312	         8.300 ns/op
PASS

The len check is as quick as I would hope. (The setup of the map is very slow, and could possibly be improved (Because I'm making a slice of maps that then get converted in $makeMap, but I don't think that is any different than master.)

FWIW, this is with 1 million elements

goos: linux
goarch: js
BenchmarkSliceLen           	1000000000	         0.2560 ns/op
BenchmarkMapLen             	1000000000	         0.5920 ns/op
BenchmarkMapNilCheck        	1000000000	         0.6890 ns/op
BenchmarkMapNilElementCheck 	98797087	        10.13 ns/op

Well, I think I am almost there. It looks like I have a Sprintf problem to fix, and then done?

As you can see in CI, there are some fmt stdlib tests that are failing. It seems like it is because I didn't wire something up to reflect/reflectlite properly.

Here's one of the tests that are failing.
https://cs.opensource.google/go/go/+/master:src/fmt/example_test.go;l=295-300?q=dach&ss=go%2Fgo

The expected output is

map[dachshund:false peanut:true] map[string]bool{"dachshund":false, "peanut":true}

The incorrect output is

map[] map[string]bool{}

I'm a bit out of my depth here, but it seems like this is because the keys and values aren't reflecting properly. My gut says it has something to do with the change from e to e.v in https://github.com/gopherjs/gopherjs/pull/1136/files#diff-70e5ca5fb5a75cb3a234c5a2caee3036426e4ffbb46a9e6b2ee556cb244eccbcR623. I'll keep taking cracks at it, but if the maintainers have a hunch, I could use the guidance.

Or maybe it has something to do with $internalize and $externalize...?

I'm going with the theory that by doing this I clobbered the information needed for reflection to work properly. I'll massage it so that we can use the Map as a POJSO, with the reflection type info...I think.

@tomconnell-wf I've looked through the diff and there is nothing obviously incorrect in it I could pin the test failure on. My best guess is that somewhere in the reflect package we read the $keys().length value, which is now 0, and so iteration is not happening properly. I am not sure I will have time for a deep dive until the weekend, but if you don't find the bug until then, I'll give it a shot.

As a side note, I recommend you rebasing on top of the latest master, which includes Go 1.18 support. There have been some additions to the reflect map-related APIs in 1.18 which may be impacted by your change or will cause merge conflicts when you are ready.

I think the official recommendation it to create a source tools.go somewhere

We can go one step further, and put it in a subdir so that its in its own package that is never even parsed during compilation (but is parsed by go mod)

This may be considered phoning it in, but I can't figure out the reflection. As best I can understand, it's built around the keys and the elements being available on the javascript object. Rather than having a special case for Map types, and since these are all references anyway, I am thinking of leaving the key and element as they were before, but also putting them into the javascript Map api, for faster size calls. I will push commits for this later today, and some javascript heap snapshots to make sure memory usage isn't significantly higher.

I would strongly prefer not to do this, not for performance, but for maintenance reasons. A lot of our work on gopherjs over the past year has been dedicated to simplifying and documenting the codebase, and I would very much prefer to help you figure out the reflection side of the problem over adding more cruft that will be confusing people in future.

Side note - when it is decided that ECMA Script 2017 is supported, we can use .entries() instead of .keys() on the map range statement, and it'll be about 2x faster.
cf9a8fa

10000 elements:

Before
✗ GOOS=linux gopherjs test ./... --bench="Benchmark.*"
Using GOOS=linux and GOARCH=ecmascript in GopherJS is deprecated and will be removed in future. Use GOOS=js GOARCH=ecmascript instead.
goos: js
goarch: ecmascript
BenchmarkSliceLen 1000000000 0.2520 ns/op
BenchmarkMapLen 1000000000 0.2490 ns/op
BenchmarkMapNilCheck 1000000000 0.5010 ns/op
BenchmarkMapNilElementCheck 363636363 3.311 ns/op
BenchmarkSliceRange 222222 4991 ns/op
BenchmarkMapRange 5454 186285 ns/op
PASS

After
✗ GOOS=linux gopherjs test ./... --bench="Benchmark.*"
Using GOOS=linux and GOARCH=ecmascript in GopherJS is deprecated and will be removed in future. Use GOOS=js GOARCH=ecmascript instead.
goos: js
goarch: ecmascript
BenchmarkSliceLen 1000000000 0.2440 ns/op
BenchmarkMapLen 1000000000 0.2410 ns/op
BenchmarkMapNilCheck 1000000000 0.4830 ns/op
BenchmarkMapNilElementCheck 377358489 3.246 ns/op
BenchmarkSliceRange 222222 4847 ns/op
BenchmarkMapRange 13656 83553 ns/op
PASS

When this merges, I can make an issue. Making it now would be putting the cart before the horse.

I would strongly prefer not to do this, not for performance, but for maintenance reasons

I spent most of the day yesterday trying to figure it out. I'll give it another shot. Maybe having slept on it will unlock something.

Current state of performance. Range is a bit worse?!? :(

10000 elements

Master:

✗ GOOS=linux gopherjs test ./... --bench="Benchmark.*"
Using GOOS=linux and GOARCH=ecmascript in GopherJS is deprecated and will be removed in future. Use GOOS=js GOARCH=ecmascript instead.
goos: js
goarch: ecmascript
BenchmarkSliceLen           	1000000000	         0.2500 ns/op
BenchmarkMapLen             	   10774	    107203 ns/op
BenchmarkMapNilCheck        	1000000000	         0.4900 ns/op
BenchmarkMapNilElementCheck 	150753768	         8.026 ns/op
BenchmarkSliceRange         	  230768	      4853 ns/op
BenchmarkMapRange           	    6315	    164054 ns/op
PASS
ok  	github.com/Workiva/doc_plat_client/tool/internal/gopherjsbenchmarks/benchmark/tom	7.427s

This commit:

✗ GOOS=linux gopherjs test ./... --bench="Benchmark.*"
Using GOOS=linux and GOARCH=ecmascript in GopherJS is deprecated and will be removed in future. Use GOOS=js GOARCH=ecmascript instead.
goos: js
goarch: ecmascript
BenchmarkSliceLen           	1000000000	         0.2440 ns/op
BenchmarkMapLen             	1000000000	         0.2430 ns/op
BenchmarkMapNilCheck        	1000000000	         0.4840 ns/op
BenchmarkMapNilElementCheck 	357142856	         3.214 ns/op // Huh, did I make indexing faster?
BenchmarkSliceRange         	  218181	      4968 ns/op
BenchmarkMapRange           	    6458	    183958 ns/op
PASS
ok  	github.com/Workiva/doc_plat_client/tool/internal/gopherjsbenchmarks/benchmark/tom	6.549s

Those numbers make me think I could change tack and just account for the size of the map during creating, assign and delete. That seems semantically less correct, through. 🤔

I haven't tested it, but I'm pretty sure this is the line you need to modify to make reflect work:

it probably should be something like keys: js.InternalObject(m).Call("keys"),.

Is there something special I need to do to include reflect changes? If I change reflect.go or reflectlite.go, then go install and then build/serve with gopherjs, the javascript output doesn't include my changes. This may be what I have been struggling with today and yesterday.

Is there something special I need to do to include reflect changes?

I believe this is probably what you're looking for: https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#code-generation

Yeah, I just discovered that vfsgendev wasn't working right. 🤦🤦🤦

That'll make things easier, I think I'm getting some traction, now.

I don't really understand the whole keysFor construct, but I figure that's been because I've been dealing with strings and ints; probably needed for reliable keys for structs and such?

Anyway, now I can see the failing fmt test mapiter shape, I think I can fix it up.
Master

This branch

I hope CI doesn't cost much. I think it may actually pass now.

➜  gopherjs git:(javascript_map_impl) ✗ gopherjs test  --short fmt log os ./tests

PASS
ok  	fmt	1.170s
PASS
ok  	log	0.444s
PASS
ok  	os	1.447s
PASS
ok  	github.com/gopherjs/gopherjs/tests	6.368s

And, makes sense that we should be cautious with this.

My curiosity is afire wanting to know what the one potential issue might be. 🐱

One other potential source of corner cases we need to verify is map internalization/externalization (that is, conversions between js.Object and Go native types). I would start by checking that mappings at the top of https://pkg.go.dev/github.com/gopherjs/gopherjs/js still hold. Then there is also the matter of embedding js.Object into structs:

type S structs {
  *js.Object
  Prop map[string]int `js:prop`
}

Good call, I'll see if I can make a test that can demonstrate correctness there.

One other potential source of corner cases we need to verify is map internalization/externalization

What is the expectation on wrapping? It seems to me that we would still want the new map implementation to wrap to a js.Object with map keys as properties, otherwise existing code will break. In other words:

type myStruct struct {
  map[string]string myMap
}

ms := &myStruct{map[string]string{`key`: `value`}}

wrapper := js.MakeFullWrapper(ms)

// Old Behavior - I think we should keep
wrapper.Get(`key`) // value

// Use Map API, breaking change
wrapper.Call(`get`, `key`) // value

Yes, I think we should make sure this doesn't break existing code and that Go maps are correctly translated into objects and back.

How do those tests look to demonstrate correctness of wrapping/internalize/externalize?

Oh, node doesn't like that. I'm not much of a Node guy. I wonder how best to do that namespacing for both contexts.

@tomconnell-wf see my suggestion here :)

Fwiw, I transpiled all of our rendering code with this branch, and it seemed to operate and output normally.

How are you all feeling about this? Is there anything additional I can do to increase comfort levels?

I can squash again when this is close to merge

@tomconnell-wf thanks, I left a few more comments. Please give me a day or two to rack my brain for more corner cases, but I agree that it is dangerously close to being merge-ready 😉

You're cooking up some good things to investigate. I'm not particularly in a rush. Now that the 1.18.5 tag is out, I will be busy getting upgraded for a while.

Any brainstorms over the weekend about edge case problems?

I agree with all of your suggestions. I'll get on that today.

Okay, I think that's everything. I'll be happy to assist if any bugs come up with this in the near future.

All looks good to me. I've edited the commit message to exclude irrelevant bits, but I think as soon as CI completes, we are good to merge \o/