HashMap.each performance

[JonasBa]

Hello, we (@getsentry) have recently started profiling one of our services which runs echarts to generate thumbnail images of charts so that they can be easily shared as previews and while checking some profiles generated in prod, we noticed that a function called each is often at the top of the stack.

I've manually looked up the dist to verify that this is hashmap related.

Looking at HashMap.each, there could be two possible performance culprits in there.

First is v8 disabling optimization of for...in described here, this could occur in the event that our {} dict exceeds the number of keys and the underlying v8 data structure backing the object starts using an actual hash map which makes enumeration slow.

Second, I noticed that map.get does a call to hasOwnProperty before returning the value, this seems much slower than just returning the value of dict[key], see micro benchmark.

I could see this being improved by using native maps as the underlying class and relying on Map.entries for iteration (there is a performance benefit here in the sense that we can also avoid a second function call to get) as well as possibly removing the hasOwnProperty call.

I would like to get your input on this issue, more specifically, I'd like to better understand the motivation behind not using native maps (assuming this could have been due to bad browsers support at the time?). Full disclaimer that I am not very familiar with the rest of the echarts codebase, so I lack context if this is a very commonly used method or if it just relates to the specific charts we generate. I would also be more than happy to contribute myself if we can agree on a direction.

Appreciate any thoughts and it goes without saying, but thank you for maintaining the library that we use daily 👏🏼

[plainheart]

Thanks for your investigation and suggestion! It sounds great and we agree on this! :)

I'd like to better understand the motivation behind not using native maps (assuming this could have been due to bad browsers support at the time?)

You are right. The custom HashMap is being used for a long time for the compatibility of IE 10 and lower. So far, I think we can switch to the native Map, and meanwhile, make a polyfill for the legacy IE as we still hope the library can support IE 10 ~ 11.

[JonasBa]

@plainheart Thank you very much, I'd like to open a PR to improve this. I am currently getting permission denied to push my changes, would it be possible to grant me the correct access rights to this repo so I can open a PR?

[plainheart]

Have you forked this repo? You need to push the changes to your forked repo and then open a PR.