Skip to content

When ECMA Script 2017 is adopted, use Map.entries() to make map range 2x faster. #1142

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

Open
tomconnell-wf opened this issue Aug 25, 2022 · 6 comments

Comments

@tomconnell-wf
Copy link
Contributor

In #1135 and #1136, ECMA Script 2015 support allowed the use of Javascript Maps to significantly speed up len() calls on maps. Yet, range loops on maps are significantly slower than on slices. Using the benchmarks in map_test.go:

goos: js
goarch: ecmascript
...
BenchmarkSliceRange         	  230768	      4849 ns/op
BenchmarkMapRange           	    6450	    184651 ns/op 
PASS

ECMA Script 2017 added the entries() call to Javascript Maps, allowing simplification of the RangeStmt:
https://github.com/gopherjs/gopherjs/compare/master...tomconnell-wf:gopherjs:js_map_entires?expand=1

Making this simple change makes map range statements over twice as fast:

goos: js
goarch: ecmascript
...
BenchmarkSliceRange         	  226414	      4876 ns/op
BenchmarkMapRange           	   19509	     61510 ns/op
PASS
@nevkontakte
Copy link
Member

That's an interesting idea, although I don't know how soon we will upgrade to ES 2017. However, ES 2015 introduced for-of and iterable protocol, so I wonder if we can get comparable improvement already?

@tomconnell-wf
Copy link
Contributor Author

I was thinking about this, and I read this good characterization of the state of browser support:

Browser vendors don't implement specific versions, but specific features. Almost every modern browser is still missing features from ES2017-ES2020.

If this project agrees with that assessment, then perhaps it would be a better policy to specify the minimum browser versions that are supported. Then, any feature that is in those ranges could be used.

For this particular api, it's been supported by a long time for all browsers except IE 11. IE 11 is the only difference between this Object.entries() and js Map.

image

I'm eager for more improvements, because the len() fix improved our rendering times by 10 to 15% and I'm stoked.

I'm aware that half a percentage of worldwide use is still quite a bit, but it'd benefit everyone else to cut IE 11 loose for this and other features.

@tomconnell-wf
Copy link
Contributor Author

I glossed over your mention of iterable and for-of. The map implementation is already using the iterable protocol already to get over the keys, so I am thinking there isn't much traction there. for-of may work, but it seemed like it would not work with fc.translateLoopingStmt, so would create a special case how loops for maps work, so I never explored it. entries() fits right in to the existing RangeStmt structure, and that's why I went down that path.

@nevkontakte
Copy link
Member

@tomconnell-wf I understand your enthusiasm for performance improvements and, to some degree, share it. However, hear me out 🙂 GopherJS has claimed ES5 compatibility for ages (even though in some respects this claim wasn't entirely true), so breaking away from it is a big shift.

As many open source projects, we don't know our clients very well, we can't contact and ask them if a certain change is okay or not, and we don't have any telemetry to know exactly where and how they use GopherJS. The best we have is the voluntary user survey, which at the moment has 90 responses, only 47 of which actually say they use GopherJS. This is tiny compared to the volume of clones GitHub reports to us.

In general, survey results paint a hopeful picture: the responders don't seem to care about browsers older than 5 years. On the other hand, some of them do say that they care about ES 2015 or even ES 5 compatibility ¯_(ツ)_/¯ So I think the responsible option for us to exercise caution about introducing newer ES features, and ES 2015 seems like a relatively safe start. If we go through at least one release cycle where nobody complains (note that in 1.18 we haven't even published a stable release, so go get is likely still directing many users to 1.17 release...), we could nudge requirements even further. Realistically, this is unlikely to happen before mid-2023.

Browser vendors don't implement specific versions, but specific features.

This is true for browser vendors, but it's arguably a rather sad state of affairs for web developers :) There is no way for you to have a bunch of linters that check the syntax and APIs of your JS code and be certain that all browsers newer than a certain age support it. If we, as GopherJS maintainers, decided to pick and choose a salad of ES features we want to use, it would be a lot of fun for us, but not for our users. This is why we added this to our ES version policy:

For the ECMAScript features from standard versions newer than the baseline, the following rules apply:

  • Using new syntax features is not allowed.
  • Using new APIs may be allowed if the following conditions are met:
    • It doesn't harm the GopherJS performance or stability in the runtimes missing the feature. Significant bundle size increases are considered a negative effect in this context.
    • GopherJS runtime handles missing APIs gracefully and transparently. GopherJS users don't have to manually polyfill or change build configuration.
    • There is a consensus among the active GopherJS maintainers that this change would be beneficial to GopherJS and its users overall.

So for now... They say that constraints fuel creativity, so we'll have to put that to test :) for ... of is a part of ES 2015 and my intuition also says me that it should be the fastest way to iterate over containers overall. It is also the most idiomatic representation of Go's for-range loop, so I think that direction is worth exploring. We could start with maps, but I'm sure it will work better for arrays and slices too (and possibly channels, if/when we switch our goroutine implementation to generators). Hope that makes sense to you.

@tomconnell-wf
Copy link
Contributor Author

I hear all of that, and appreciate the constraints you are under as a OSS maintainer. I'm being shifted away to a different project, so was kind of hoping for an 11th hour quick win. I'd love to look into for-of, but I figure that will be now left to my free time, of which I have none. C'est la vie.

@nevkontakte
Copy link
Member

@tomconnell-wf fair enough. Spare time is something we all have precious little of. Your suggestions and contributions are appreciated nevertheless 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants