feat(eventListener): fix #798, improve EventTarget.addEventListener performance #812

JiaLiPassion · 2017-06-11T16:36:07Z

improve EventTarget.addEventListener performance with @mhevery 's idea which described in Slow addEventListener performance. #798, we will not save a eventTask list on event target, and will not go through this eventTask list to find whether we have already registered the event or not.

instead we save the information to callback itself, so

div.addEventListener('click', () => {});

will not slow down

div.addEventListener('mouseover', () => {});

this solution only work in browser, in nodejs we still use the original way to save taskList,
because in the eventListener callback, there is no eventName information, and nodejs support to add the identical listener multiple times.

mhevery · 2017-06-12T00:57:51Z

I added a performance test here: 3a017d3
Just run gulp build and than open the file in the browser to see how fast it is. In DevTools go to Performance tab and record the click. Do bottom-up view to see what is going on.

Right away two things jump out.

Function.toString should not be called (I left a comment where it is coming from.)
I see try-catch which needs to go (left a comment)
Way to much GC

The GC problem means that we are still creating too many objects. Can we do an audit and see if every one of these objects is needed?

mhevery · 2017-06-12T01:03:31Z

The issue is coming from zoneAwareAddListener

    try {
      // In cross site contexts (such as WebDriver frameworks like Selenium),
      // accessing the handler object here will cause an exception to be thrown which
      // will fail tests prematurely.
      validZoneHandler = data.handler && data.handler.toString() === '[object FunctionWrapper]';
    } catch (error) {
      // we can still try to add the data.handler even we are in cross site context
      data.crossContext = true;
      return data.invokeAddFunc(addFnSymbol, data.handler);
    }

we have to remove the try-catch since that is preventing the code from optimizing.
Inside we do a check by calling data.handler.toString, this is expensive and has to go.

JiaLiPassion · 2017-06-12T17:12:11Z

@mhevery , thank you for review!

I tried several approaches and did some test.

native add/remove same callback performance: 140ms

Approach 1. current PR version.

add/remove same callback performance: 1400~1600ms (10x slower than native).

Approach 2. comment the `try/catch` and `toString` code below.

  try {
      // In cross site contexts (such as WebDriver frameworks like Selenium),
      // accessing the handler object here will cause an exception to be thrown which
      // will fail tests prematurely.
      validZoneHandler = data.handler && data.handler.toString() === '[object FunctionWrapper]';
    } catch (error) {
      // we can still try to add the data.handler even we are in cross site context
      data.crossContext = true;
      return data.invokeAddFunc(addFnSymbol, data.handler);
    }

add/remove same callback performance: 1300ms (9x slower than native).
And need to consider the cross context issue.

Approach 3. remove `generic code`, make `zoneAwareCallback` global.

comment the try/catch and toString code.
and make zoneAwareCallback be global. (all event target will share one zoneAwareCallback). so we will not create a zoneAwareCallback function for each addEventListener.
remove all generic code such as patchMethod, ListenerMeta and so on...

// all event target will share this callback.
const globalZoneAwareCallback = function(event: Event) {
  const target = this || _global;
  const task = target['__zone_symbol__' + event.type][FALSE_STR];
  if (task) {
    return task.invoke.apply(target, [event]);
  }
  
}

// all event target will share this callback with capture
const globalZoneAwareCaptureCallback = function(event: Event) {
  const target = this || _global;
  const task = target['__zone_symbol__' + event.type][TRUE_STR];
  if (task) {
    return task.invoke.apply(target, [event]);
  }
 
}

// define some global string
const TRUE_STR = 'true';
const FALSE_STR = 'false';

// define native delegate
let nativeAddEventListener = (EventTarget.prototype as any)['__zone_symbol__addEventListener'] = EventTarget.prototype.addEventListener;
let nativeRemoveEventListener = (EventTarget.prototype as any)['__zone_symbol__removeEventListener'] = EventTarget.prototype.removeEventListener;


const customSchedule = function(task: Task) {
  let data: any = task.data as any;
  return nativeAddEventListener.apply(data.target, [data.event, data.zoneAwareCallback, data.options]);
}

const customCancel = function(task: Task) {
  let data: any = task.data as any;
  return nativeRemoveEventListener.apply(data.target, [data.event, data.zoneAwareCallback, data.options]);
}

export function patchEventTargetMethodsOptimized() {
  EventTarget.prototype.addEventListener = function() {
    const target = this || _global;
    let capture = arguments.length > 2 ? (typeof arguments[2] === 'boolean' ? arguments[2] : !!arguments[2].capture) : false;
    let events = target['__zone_symbol__' + arguments[0]];
    if (events) {
      const task = events[capture ? TRUE_STR : FALSE_STR];
      if (task) {
        return nativeAddEventListener.apply(target, [arguments[0], capture ? globalZoneAwareCaptureCallback : globalZoneAwareCallback, arguments.length > 2 ? arguments[2] : undefined]);
      }
    } else {
      target['__zone_symbol__' + arguments[0]] = events = {};
    }  
    events[capture ? TRUE_STR : FALSE_STR] = Zone.current.scheduleEventTask(eventName, delegate, {event, capture, zoneAwareCallback}, customSchedule, customCancel); 
  };

  EventTarget.prototype.removeEventListener = function() {
    const target = this || _global;
    let capture = arguments.length > 2 ? (typeof arguments[2] === 'boolean' ? arguments[2] : !!arguments[2].capture) : false;
    const task = target['__zone_symbol__' + arguments[0]][capture ? TRUE_STR : FALSE_STR];
    if (task) {
      target['__zone_symbol__' + arguments[0]][capture ? TRUE_STR : FALSE_STR] = undefined;
      task.zone.cancelTask(task);
    } else {
      nativeRemoveEventListener.apply(target, arguments);
    }
    
  }
}

So the performance will become from 500 ~ 700 ms. (4x slower than native)

Approach 4. don't schedule task at all.

base on the above approach , I want to see how much performance can boost by remove task, so the code will just like.

// global zoneAwareCallback.
const globalZoneAwareCallback = function(event: Event) {
  const target = this || _global;
 
  const zoneWithCallback = target['__zone_symbol__' + event.type][FALSE_STR];
  if (zoneWithCallback) {
    zoneWithCallback.zone.runGuarded(zoneWithCallback.callback, this || _global, [event.type, zoneWithCallback.callback, zoneWithCallback.options]);
  }
}

EventTarget.prototype.addEventListener = function() {
    const target = this || _global;
    let capture = arguments.length > 2 ? (typeof arguments[2] === 'boolean' ? arguments[2] : !!arguments[2].capture) : false;
    let events = target['__zone_symbol__' + arguments[0]];
    if (!events) {   
      target['__zone_symbol__' + arguments[0]] = events = {};
    }
    events[capture ? TRUE_STR : FALSE_STR] = {zone: Zone.current, callback: arguments[1], options: arguments.length > 2 ? arguments[2] : undefined};
    
    return nativeAddEventListener.apply(target, [eventName, zoneAwareCallback, options]);
  };

So the performance will become 260~350ms (2x slower than native).

Approach 5. schedule fake `eventTask`

based on Approach 4, I think maybe we can create a fake eventTask which can also be shared globally, and we just call scheduleTask, invokeTask, cancelTask callback of zoneSpec directly in patched method. Because eventTask will not affect taskCount/hasTask,and task status is also not that useful, so if we can create a pure fake eventTask which only for trigger zoneSpec's callback, maybe we can get the same performance as Approach 4.

Please review.

mhevery · 2017-06-13T00:05:46Z

@JiaLiPassion WOW, very impressive analysis! Kudos for such amazing work and insight!

Yes approach 4/5 seems a way to go. It is our best hope to decrease the memory pressure and increases registration speed.

In approach 5 how would task cancelation/rescheduling work? We have to make sure that this would continue to work.

const someZone = Zone.current.fork({
  name: 'someZone',
  onScheduleTask:
      (delegate: ZoneDelegate, current: Zone, target: Zone, task: Task):
          Task => {
            // Blacklist scroll, mouse, and request animation frame events.
            if ((task.type === 'eventTask' || task.type === 'macroTask') &&
                isBlacklistedEvent(task.source)) {
              task.cancelScheduleRequest();

              // Schedule task in root zone, note Zone.root != target,
              // "target" Zone is Angular. Scheduling a task within Zone.root
              // will prevent the infinite digest cycle from appearing.
              return Zone.root.scheduleTask(task);
            } else {
              return delegate.scheduleTask(target, task);
            }
          }
});```

JiaLiPassion · 2017-06-13T01:34:33Z

@mhevery , got it, I will try to find out Approach 5 work or not.

JiaLiPassion · 2017-06-21T17:16:02Z

Status

Update source to pass all current test cases.
add more test cases for capture, add/removeEventListener
add logic to handle add different handlers for same event name logic.
add IE/edge and cross context check only when is IE/Edge or enable cross context check.
(the try/catch and toString compare code).
fix Microsoft EdgeHTML 15 Browser Tools hooking into zone.js #817,1. use @wizarrc 's suggest way to fix edge dev tools also use zone.js patched addEventListener/removeEventListener issue.

now the performance result is

add same callback

native: 150ms
current zone.js implementation: 1600ms
improved zone.js implementation: 460~580ms

3.5x slower than native and 3x faster than current version.

add different callback

native: 170 ms
current zone.js implementation: 1500ms
improved zone.js implementation: 520ms

3x slower than native and 3x faster than current version.

changes from current implementation.

add different handlers for same event , internal behavior changes

const handler1 = function() {};
const handler2 = function() {};

button.addEventListener('click', handler1);
button.addEventListener('click', handler2);

in current version. nativeAddEventListener will add handler1 and handler2 to button
it looks like

button.addEventListener('click', zoneAwareHandler1);
button.addEventListener('click', zoneAwareHandler2);

in this PR's version, there will only one globalZoneAwareCallback for the same eventName + capture combination, so it will look like this.

button.addEventListener('click', globalZoneAwareCallback);
button['__zone_symbol__tasks'].push(handler1Task);
button['__zone_symbol__tasks'].push(handler2Task);

This is the biggest changes from current implementation.

Another changes is, the IE/Edge and cross context check

 try {
          const testString = delegate.toString();
          if ((testString === FUNCTION_WRAPPER || testString == BROWSER_TOOLS)) {
            return nativeDelegate.apply(this, arguments);
          }
        } catch (error) {
          return nativeDelegate.apply(this, arguments);
        }

will only run when browser is IE/Edge and cross context check is enabled by set __Zone_enable_corss_context_check = true, I have added it into MODULE.md

Improvement point.

Basically one point only.
Don't create may objects

use global zoneAwareCallback
optimized zoneEventTask, don't create ZoneTask.invoke closure when schedule new ZoneTask
don't create string when possible, use predefined string, for example, eventName, task.source and so on. so GC time will be reduced.
don't create task.data object, use global variable to scheduleTask, https://github.com/angular/zone.js/pull/812/files#diff-06332d8779803ae538dab973c558d328R218, so GC time will be reduced.

Some codes may look ugly such as https://github.com/angular/zone.js/pull/812/files#diff-06332d8779803ae538dab973c558d328R218, the purpose is ** don't create additional objects **

Please review! Thank you!

…ner performance

mhevery · 2017-06-24T05:59:25Z

awesome progress...

…

On Wed, Jun 21, 2017 at 7:16 PM, JiaLiPassion ***@***.***> wrote: Progress. Use Approach 5. still schedule event task, now the performance data will be 350~450 ms, it will be 3x native version, and I will continue to add some optimization for zone.scheduleEventTask to remove some not needed try/catch or new Function — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#812 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG1T5qS1_KLR6MMMnWftE2rrggxEci5ks5sGU_SgaJpZM4N2cXP> .

JiaLiPassion · 2017-06-24T06:02:43Z

@mhevery , thank you! currently all the test has passed, please review.

And I will continue to check other tasks such as macroTask or microTask can improve performance or not in another PR.

mhevery

Minor changes requested
It seems like we have added a lot of new code but have not deleted existing code. What is going?
Could we somehow get this better packaged into __load_patch so that it is cleaner?

mhevery · 2017-06-24T08:09:31Z

lib/browser/event-target.ts

+const zoneSymbolEventNames: any = {};
+const globalSources: any = {};
+
+const _global: any =


Could we get this from the patch function global? It would be better if we checked for global in exactly one place.

yeah, I will move this code into __load_patch

mhevery · 2017-06-24T08:10:42Z

lib/browser/event-target.ts

+const BROWSER_TOOLS = 'function __BROWSERTOOLS_CONSOLE_SAFEFUNC() { [native code] }';
+
+//  predefine all __zone_symbol__ + eventName + true/false string
+eventNames.forEach((eventName: string) => {


could we use for loop for perf reasons?

yes, will do.

mhevery · 2017-06-24T08:10:57Z

lib/browser/event-target.ts

+});
+
+//  predefine all task.source string
+WTF_ISSUE_555.split(',').forEach((target: string) => {


could we use for loop for perf reasons?

yes, will do

mhevery · 2017-06-24T08:18:33Z

also thank you for the awesome work on investigating the perf and getting it down.

JiaLiPassion · 2017-06-24T18:43:36Z

@mhevery , I have updated the code.

Minor changes requested
It seems like we have added a lot of new code but have not deleted existing code. What is going?
Could we somehow get this better packaged into __load_patch so that it is cleaner?

the changes you requested have been implemented.
the old code still exists because it will needed by nodejs EventEmitter, nodejs EventEmitter currently can't use this optimized way, because in the event listener, there is not eventName.

target.addListener('msg', function() {
   // here we can not get the event type which should be `msg`.
});

so we can't use globalCallback. We have to create a closure callback for every addListener to save the eventName information.

But we can still benefit from other improvements, so I will move this PR's new addEventListener/removeEventListener into common and remove the old code.

And I have moved the code into __load_patch.

I will continue to make this code also work for nodejs.

JiaLiPassion · 2017-06-25T13:43:19Z

@mhevery , I have updated the source.

remove old addEventListener/removeEventListener code.
make nodejs work in this new method.
And nodejs also get a 40% performance up.

current version.

new callback: 700ms
reuse callback : 550ms

improved version.

new callback: 440ms
reuse callback: 300ms

add new API for browser, now EventTarget support eventListenerList method to get all listeners for all event name or for specified event name
add new API for browser to support removeAllListeners

Please review, Thank you.

googlebot added the cla: yes label Jun 11, 2017

JiaLiPassion mentioned this pull request Jun 17, 2017

feat(saucelab): integrate saucelab webdriver test. #819

Merged

JiaLiPassion force-pushed the new-task branch from b87b3fe to 59118d5 Compare June 21, 2017 17:14

JiaLiPassion force-pushed the new-task branch from 59118d5 to 5c2fc04 Compare June 22, 2017 09:28

feat(performance): fix angular#798, improve EventTarget.addEventListe…

fb03d4c

…ner performance

JiaLiPassion force-pushed the new-task branch from 5c2fc04 to fb03d4c Compare June 22, 2017 09:55

JiaLiPassion added 4 commits June 23, 2017 02:20

add test cases

76e2a7a

add comment to explain the implementations

4bde5c7

add test cases for resschedule eventTask

a14f1a6

add IE/edge and cross context check

c5167b8

JiaLiPassion force-pushed the new-task branch 4 times, most recently from b27622f to 5110e35 Compare June 23, 2017 17:02

modify document for extra flags

24cdf71

JiaLiPassion force-pushed the new-task branch from 5110e35 to 24cdf71 Compare June 23, 2017 17:49

mhevery suggested changes Jun 24, 2017

View reviewed changes

minor changes, define some const, move logic into __load_patch

b133801

JiaLiPassion force-pushed the new-task branch from 0174209 to b133801 Compare June 24, 2017 18:48

let nodejs use new addListener/removeListener method

de99f73

add nodejs test example

58255b4

add eventListeners/removeAllListeners test cases for browser

8b568fb

mhevery approved these changes Jun 30, 2017

View reviewed changes

mhevery merged commit b3a76d3 into angular:master Jun 30, 2017

JiaLiPassion deleted the new-task branch July 13, 2017 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eventListener): fix #798, improve EventTarget.addEventListener performance #812

feat(eventListener): fix #798, improve EventTarget.addEventListener performance #812

JiaLiPassion commented Jun 11, 2017

mhevery commented Jun 12, 2017

mhevery commented Jun 12, 2017

JiaLiPassion commented Jun 12, 2017 •

edited

Loading

mhevery commented Jun 13, 2017

JiaLiPassion commented Jun 13, 2017

JiaLiPassion commented Jun 21, 2017 •

edited

Loading

mhevery commented Jun 24, 2017 via email

JiaLiPassion commented Jun 24, 2017

mhevery left a comment

mhevery Jun 24, 2017

JiaLiPassion Jun 24, 2017

mhevery Jun 24, 2017

JiaLiPassion Jun 24, 2017

mhevery Jun 24, 2017

JiaLiPassion Jun 24, 2017

mhevery commented Jun 24, 2017

JiaLiPassion commented Jun 24, 2017

JiaLiPassion commented Jun 25, 2017

feat(eventListener): fix #798, improve EventTarget.addEventListener performance #812

feat(eventListener): fix #798, improve EventTarget.addEventListener performance #812

Conversation

JiaLiPassion commented Jun 11, 2017

mhevery commented Jun 12, 2017

mhevery commented Jun 12, 2017

JiaLiPassion commented Jun 12, 2017 • edited Loading

Approach 1. current PR version.

Approach 2. comment the try/catch and toString code below.

Approach 3. remove generic code, make zoneAwareCallback global.

Approach 4. don't schedule task at all.

Approach 5. schedule fake eventTask

mhevery commented Jun 13, 2017

JiaLiPassion commented Jun 13, 2017

JiaLiPassion commented Jun 21, 2017 • edited Loading

Status

now the performance result is

changes from current implementation.

Improvement point.

mhevery commented Jun 24, 2017 via email

JiaLiPassion commented Jun 24, 2017

mhevery left a comment

Choose a reason for hiding this comment

mhevery Jun 24, 2017

Choose a reason for hiding this comment

JiaLiPassion Jun 24, 2017

Choose a reason for hiding this comment

mhevery Jun 24, 2017

Choose a reason for hiding this comment

JiaLiPassion Jun 24, 2017

Choose a reason for hiding this comment

mhevery Jun 24, 2017

Choose a reason for hiding this comment

JiaLiPassion Jun 24, 2017

Choose a reason for hiding this comment

mhevery commented Jun 24, 2017

JiaLiPassion commented Jun 24, 2017

JiaLiPassion commented Jun 25, 2017

JiaLiPassion commented Jun 12, 2017 •

edited

Loading

Approach 2. comment the `try/catch` and `toString` code below.

Approach 3. remove `generic code`, make `zoneAwareCallback` global.

Approach 5. schedule fake `eventTask`

JiaLiPassion commented Jun 21, 2017 •

edited

Loading