-
Notifications
You must be signed in to change notification settings - Fork 570
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
Comments
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? |
I was thinking about this, and I read this good characterization of the state of browser support: 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. I'm eager for more improvements, because the 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. |
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 |
@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
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:
So for now... They say that constraints fuel creativity, so we'll have to put that to test :) |
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 |
@tomconnell-wf fair enough. Spare time is something we all have precious little of. Your suggestions and contributions are appreciated nevertheless 🙂 |
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: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:
The text was updated successfully, but these errors were encountered: