neveldo / jQuery-Mapael

jQuery plugin based on raphael.js that allows you to display dynamic vector maps
https://www.vincentbroute.fr/mapael/
MIT License
1.01k stars 195 forks source link

Performance: use event delegation #319

Closed Indigo744 closed 7 years ago

Indigo744 commented 7 years ago

Ok, this one is quite big.

Until now, we attached to each area/plot/link node the following event listeners:

This can become quite a lot for big maps with lots of elements.

The solution is actually quite simple: use event delegation! Basically, instead of attaching all event listener per node, we attach only one event listener to the container, and filter the child element with delegation. More information: http://api.jquery.com/on/ and https://learn.jquery.com/events/event-delegation/

So the modifications are:

For now, I haven't touched the way the custom eventHandlers works. They are still attached to each node according to the user option. What do you think?

Here is a test using one of the example with lots of plots and tooltips Before: Huge number of listener events 1113 events listeners, 21 MB JS heap After: Small number of listener events 4 events listeners, 15 MB JS heap

This may help for #304

I've checked all example without finding any regression. But I'll let you review this ;-)

Indigo744 commented 7 years ago

Ah, I may need to update the tests

neveldo commented 7 years ago

Hi @Indigo744 ,

Wow, indeed its a big one. Thanks a lot for it, I was having in my mind this issue regarding the huge number of events added by Mapael since a long time, but never took the time to dive into it !

I will take the necessary time to look at your PR and I will keep you informated within the next few days !

Indigo744 commented 7 years ago

Added delegation for custom events! I've namespaced them like 'click.mapael.custom' to be able to easily unbind/rebind them.

neveldo commented 7 years ago

Wow thanks ! I keep you informed ASAP for the review !

Indigo744 commented 7 years ago

I've updated as per your recommendation =)

I tried to create enough small commits so you can review one by one. But in the end, maybe we should use the "squash and merge" option of GH. What do you think?

Indigo744 commented 7 years ago

Since we are already talking about v3 (plugin related), I'm wondering if we should postpone this evolution to the next major release?

I am afraid it may break on some people who has weirdly override some event handlers on each element...

What do you think?

neveldo commented 7 years ago

Hi @Indigo744 ,

Thanks for the fixes and for having separated the work in small commits, it helped a lot for the review ! (I agree for the squash when we will merge it with the master branch).

Have you some weirdly override cases that will not work anymore when this PR will be merged ? Don't you think that all regular use cases should still work fine after this PR will be merged ?

I think that if some user extend some of the internal functions of Mapael, they should do this with full knowledge of the facts.Otherwise, each minor change of any internal function interface should lead to a new major release of mapael.

Indigo744 commented 7 years ago

Nah I don't have any weird override cases ^^ but I was trying to imagine what could be the implication.

I think you are right, and the benefits can be important for users with lots of plots.

As a side note, I was wondering if we should put the filtering Timeout values as a variable so user can override them if needed.

neveldo commented 7 years ago

I think it could be indeed a good idea to make the timeout delay an internal variable to allow overriding. However, I think we shouldn't expose it as an regular option as the great majority of users will not need to override it.