Performance: use event delegation #319

Indigo744 · 2017-10-03T11:11:25Z

Ok, this one is quite big.

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

mouseover (for hover attributes and tooltips)
mousemove (for tooltip position)
mouseout (for hover attributes and tooltips)
click (for href)

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:

remove any event attachment for nodes ($().on()..)
implements each event listeners on container in initDelegatedMapEvents() and dispatch relevant action

~~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:

1113 events listeners, 21 MB JS heap
After:

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 · 2017-10-03T11:14:02Z

Ah, I may need to update the tests

neveldo · 2017-10-04T08:02:08Z

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 · 2017-10-06T13:50:44Z

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

neveldo · 2017-10-10T07:49:58Z

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

neveldo · 2017-10-10T18:17:56Z

js/jquery.mapael.js

+            self.$map.on("mouseover." + pluginName, "[data-id]", function () {
+                var elem = this;
+                clearTimeout(mapMouseOverTimeoutID);
+                mapMouseOverTimeoutID = setTimeout(function(){


missing space between ) and {

neveldo · 2017-10-10T18:19:57Z

js/jquery.mapael.js

+            self.$map.on("mousemove." + pluginName, "[data-id]", function (event) {
+                var elem = this;
+                clearTimeout(mapMouseMoveTimeoutID);
+                mapMouseMoveTimeoutID = setTimeout(function(){


missing space between ) and {

neveldo · 2017-10-10T18:28:21Z

js/jquery.mapael.js

+
+            /* Handle tooltip position update */
+            if (elem.tooltip !== undefined) {
+                console.log(elem);


console.log() should be removed

I don't see it actually... Did you check the latest rev?

neveldo

Hi @Indigo744 ,

Wow, very ... very huge work for a great improvement upcoming !

I have reviewed your PR, but commit by commit, I hope you will be able to read all the comments (some of them are obselete as you have fixed the issues in the next commits, like the trailing console.log() for instance).

All seems to work, except that the "onhover" (for instance to display the tooltips) behaviour is now jerky. I think this is due to the setTimeout() within the mouseover event handler ?

I think its ok to have a timeout before displaying a tooltip on an area or a plot (this is the current behaviour). But the tooltip should not be jerky when it is already displayed and the user is moving the cursor within an area.

neveldo · 2017-10-10T18:34:30Z

js/jquery.mapael.js

+                    var id = $elem.attr('data-id');
+                    var type = $elem.attr('data-type');
+
+                    if (type === 'area' || type === 'area-text') {


Maybe this if/elseif/elseif condition could be simplified with :

typesMapping = { 'area' : 'areas', 'area-text' : 'areas', 'plot' : 'plots', 'plot-text' : 'plots', 'link' : 'links', 'link-text' : 'links' }; self.elemClick(self[typesMapping[type]][id]);

(maybe typesMapping could be more global, as this if/elseif/elseif pattern is repeating in each event handling functions).

Really good idea! I just committed the change.

neveldo · 2017-10-10T18:37:11Z

js/jquery.mapael.js

@@ -1954,7 +1952,7 @@
                    if (elem.tooltip.overflow.right === true) {
                        tooltipPosition.left = mouseX - self.$map.offset().left + 10;
                    }
-                    if (selem.tooltip.overflow.bottom === true) {
+                    if (elem.tooltip.overflow.bottom === true) {


It seems that you have fixed this typo twice ;)

Ahah yes indeed!

…o event_delegation

Indigo744 · 2017-10-10T19:29:12Z

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?

…o event_delegation

Indigo744 · 2017-10-11T18:44:02Z

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 · 2017-10-15T15:03:54Z

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 · 2017-10-15T17:18:52Z

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 · 2017-10-17T07:49:50Z

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.

…nto event_delegation

Indigo744 requested a review from neveldo October 3, 2017 11:11

Indigo744 and others added 13 commits October 6, 2017 14:59

Add data-type to elements

3420269

Delegate all mouseover/mousemove/mouseleave events

c4438e6

Delegate click events

3016553

Fix typo

dc417de

Set better data-type for legend elems and canvas

e1c7ebb

Run delegated events on container (to also handle legends)

67f6cd1

Store all generated legends

c5011c5

Set legends as mapElem / textElem

209b044

Event delegation mouseover/mousemove/mouseout for legends

c972935

No event filtering for click events

8f3ba39

self.legends uses legendType

331a93a

Event delegation for legend click

ecc133a

Fix tests

74f5d97

Indigo744 force-pushed the event_delegation branch from 5f1bd73 to 74f5d97 Compare October 6, 2017 13:00

Implements delegation for custom events

35eaed4

neveldo reviewed Oct 10, 2017

View reviewed changes

Add space between () and {

61812c7

neveldo reviewed Oct 10, 2017

View reviewed changes

Indigo744 added 3 commits October 10, 2017 21:08

Merge branch 'master' of https://github.com/neveldo/jQuery-Mapael int…

c01dde6

…o event_delegation

Reduce settimeout value for tooltips hover to avoid jerkiness

596cd4b

Simplify event delegation computation function

a4b473f

Merge branch 'master' of https://github.com/neveldo/jQuery-Mapael int…

258043a

…o event_delegation

neveldo approved these changes Oct 17, 2017

View reviewed changes

Merge branch 'master' of https://github.com/Indigo744/jQuery-Mapael i…

ded2138

…nto event_delegation

Indigo744 merged commit 5c66915 into neveldo:master Oct 24, 2017

Indigo744 deleted the event_delegation branch October 24, 2017 12:34

This was referenced Oct 30, 2017

Fix: custom event handler on text element #342

Merged

Fix hover legend #347

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance: use event delegation #319

Performance: use event delegation #319

Indigo744 commented Oct 3, 2017 •

edited

Loading

Indigo744 commented Oct 3, 2017

neveldo commented Oct 4, 2017

Indigo744 commented Oct 6, 2017

neveldo commented Oct 10, 2017

neveldo Oct 10, 2017

Indigo744 Oct 10, 2017

neveldo Oct 10, 2017

Indigo744 Oct 10, 2017

neveldo Oct 10, 2017

Indigo744 Oct 10, 2017

neveldo left a comment

neveldo Oct 10, 2017

Indigo744 Oct 10, 2017

neveldo Oct 10, 2017

Indigo744 Oct 10, 2017

Indigo744 commented Oct 10, 2017

Indigo744 commented Oct 11, 2017

neveldo commented Oct 15, 2017

Indigo744 commented Oct 15, 2017

neveldo commented Oct 17, 2017

Performance: use event delegation #319

Performance: use event delegation #319

Conversation

Indigo744 commented Oct 3, 2017 • edited Loading

Indigo744 commented Oct 3, 2017

neveldo commented Oct 4, 2017

Indigo744 commented Oct 6, 2017

neveldo commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neveldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Indigo744 commented Oct 10, 2017

Indigo744 commented Oct 11, 2017

neveldo commented Oct 15, 2017

Indigo744 commented Oct 15, 2017

neveldo commented Oct 17, 2017

Indigo744 commented Oct 3, 2017 •

edited

Loading