Slow addEventListener performance. #798

mhevery · 2017-06-02T18:46:57Z

Issue

Currently calling addEventListener and removeEventListener is significantly slower than native version. This has a negative impact on Angular framework performance.

Root Cause

Given

function userCallback() {....}
div.addEventListener('click', userCallback);
div.removeEventListener('click', userCallback);

In order to make the zone propagate the userCallback has to be wrapped in zoneAwareUserCallback like so:

var zoneAwareUserCallback = makeZoneAware(userCallback);
div.addEventListener('click', zoneAwareUserCallback);
div.removeEventListener('click', zoneAwareUserCallback);

Notice that when the removeEventListener gets called it needs to get called with zoneAwareUserCallback rather than userCallback.

The issue is that the addEventListener has to do the translation from userCallback to zoneAwareUserCallback internally (its monkey patched). And the same translation has to happen in removeEventListener. This translation has to be memoized for this is to work.

Currently the cost of creating the delegate and book keeping is the source of the slowdown.
The current bookkeeping involves creating an array which is patched on the EventTarget instance which contains all of the userCallbacks. For each one there is a corresponding zoneAwareUserCallback. When subsequent call is made to the removeEventListener the array is first checked to see if it contains the callback, and if so we reuse its zone-aware version.

Naive Approach

You may ask why is this not sufficient.

function userCallback() {....}
userCallback.zoneAware = makeZoneAware(userCallback);

This would simply patch the zone-aware function on the function itself and it would greatly simplify the bookkeeping. The problem is that the zone-aware function is bound to a specific Zone. This means that it can't be reused for different zones.

This code would break:

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback);
zoneB.run(() => div.addEventListener('click', userCallback);

The reason is that zone-aware callback is bound to zone, hence it would restore the wrong zone.

Suggested fix: Modification of Naive Fix.

Since the callback is zone specific, instead of patching to a well known name, we could patch to a name which is zone specific. (This assumes that each zone would have a private unique symbol)

function userCallback() {....}
zoneA.run(() => {
  userCallback[Zone.current.__private_symbol]= makeZoneAware(userCallback);
});
zoneB.run(() => {
  userCallback[Zone.current.__private_symbol]= makeZoneAware(userCallback);
});

This would greatly simplify the book-keeping and memory pressure which is the source of the performance slowdown.

Internally addEventListener/removeEventListener can easily determine how to translate from user to zone-aware callback.

Related Code

https://github.com/angular/zone.js/blob/master/lib/browser/event-target.ts

The text was updated successfully, but these errors were encountered:

JiaLiPassion · 2017-06-02T23:47:28Z

@mhevery , I want to confirm whether I understand correctly or not.

Suggested fix: Modification of Naive Fix.

addEventListener

the logic will look like

const originalAddEventListener = document.addEventListener;
const originalRemoveEventListener = document.removeEventListener;

document.addEventListener = function(...) {
    // make Zone aware will schedule an eventTask
    const zoneAwareCallback = userCallback[Zone.current.__private_symbol] = makeZoneAware(userCallback); 
    originalAddEventListener.apply(this, [zoneAwareCallback]);
}

document.removeEventListener = function(...) {
    // find all zoneAwareUserCallback attached to userCallback
    for (let key in userCallback) {
       if (key.startsWith('ZonePrivateSymbol')) {
          originalRemoveEventListener.apply(this, [userCallback[key]]);
       }
    }    
}

// and makeZoneAware will not keep the addtional eventTasks array in zone.js memory

https://github.com/angular/zone.js/blob/master/lib/common/utils.ts#L340
https://github.com/angular/zone.js/blob/master/lib/common/utils.ts#L346

will be removed.

Is that correct?

mhevery · 2017-06-03T15:05:07Z

/cc @pkozlowski-opensource

@JiaLiPassion yes that looks right. But I have a question. Why do we have to loop in removeEventListener? I don't think we do, plus it would add to the performance regression.

I guess there could be an issue if the removeEventListener got called with different zone than remove. In that case we could do the slower case and loop over all of the keys.

But i want to make sure that the happy case is fast, with no loops.

JiaLiPassion · 2017-06-03T18:01:25Z

@mhevery , yeah, you are right, we may don't need to loop. And we may don't need a private unique symbol for each zone either.

consider the case you mentioned above.

Case1

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback);
zoneB.run(() => div.addEventListener('click', userCallback);

// then removeEventListener, this code may run in zoneA/zoneB or any other zone
div.removeEventListener('click', userCallback);

Current version of zone.js

the code above will have the following result.

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback);
// div will register a click eventListener which is a EventTask whose zone is zoneA

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 1);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][0].zone, zoneA);

zoneB.run(() => div.addEventListener('click', userCallback);
// zone will first try to find the existing task, and will get the 
// eventTask which created by zoneA, and it will not try to 
// schedule a new eventTask, it will just use the existing one.

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 1);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][0].zone, zoneA);

// then removeEventListener, this code may run in zoneA/zoneB or any other zone
div.removeEventListener('click', userCallback);
// the existing eventTask will be removed.

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 0);

So for this case, use Naive Approach will also ok.

function userCallback() {....}
userCallback.zoneAware = makeZoneAware(userCallback);

because in this case, will actually will only have one zoneAware listener,
which would be the first zone (in the example it will be zoneA) created EventTask.
and the userCallback will always be triggered in zoneA.

Case2

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback, {useCapture: true});
zoneB.run(() => div.addEventListener('click', userCallback, {useCapture: false});

// then removeEventListener, this code may run in zoneA/zoneB or any other zone
div.removeEventListener('click', userCallback, {useCapture: true});
div.removeEventListener('click', userCallback, {useCapture: false});

Current version of zone.js

the code above will have the following result.

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback, {useCapture: true});
// div will register a click eventListener which is a EventTask whose zone is zoneA

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 1);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][0].zone, zoneA);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][0].data.options.useCapture, true);

zoneB.run(() => div.addEventListener('click', userCallback, {useCapture: false});
// zone will first try to find the existing task with the callback and useCapture
// there will no existing one, so it will try to 
// schedule a new eventTask.

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 2);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][1].zone, zoneB);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][1].data.options.useCapture, false);

// then removeEventListener, this code may run in zoneA/zoneB or any other zone
div.removeEventListener('click', userCallback, {useCapture: true});
// the existing eventTask which created by zoneA will be removed.

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 1);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][1].zone, zoneB);
assert.toStrictEqual(div[zoneSymbol['eventTasks']][1].data.options.useCapture, false);

div.removeEventListener('click', userCallback, {useCapture: false});
// the existing eventTask which created by zoneB will be removed.

assert.toStrictEqual(div[zoneSymbol['eventTasks']].length, 0);

in this case, the pure Native Approach is not enough. but I think we may don't need to use zone specified symbol such as userCallback[Zone.current.__private_symbol] because

when removeEventListener, it has nothing to do with the zone it was called, it just compare the callback and useCapture options.
when addEventListener, it will not allow duplicate one (callback and useCapture), so we can't create duplicate eventListener in different zone.

so we may just need a different version of Suggested fix: Modification of Naive Fix.

function userCallback() {....}
zoneA.run(() => {
  userCallback[{useCapture: true}]= makeZoneAware(userCallback);
});
zoneB.run(() => {
  userCallback[{useCapture: false}]= makeZoneAware(userCallback);
});

so the case2 will become

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback, {useCapture: true});
// div will register a click eventListener which is a EventTask whose zone is zoneA

assert.toStrictEqual(userCallback[{useCapture: true}].zone, zoneA);
assert.toStrictEqual(userCallback[{useCapture: true}].data.options.useCapture, true);

zoneB.run(() => div.addEventListener('click', userCallback, {useCapture: false});
// zone will first try to find the existing task with the callback and useCapture
// there will no existing one, so it will try to 
// schedule a new eventTask.

assert.toStrictEqual(userCallback[{useCapture: false}].zone, zoneB);
assert.toStrictEqual(userCallback[{useCapture: false}].data.options.useCapture, false);

// then removeEventListener, this code may run in zoneA/zoneB or any other zone
div.removeEventListener('click', userCallback, {useCapture: true});
// the existing eventTask which created by zoneA will be removed.

assert.toStrictEqual(userCallback[{useCapture: true}], undefined);
assert.toStrictEqual(userCallback[{useCapture: false}].zone, zoneB);

div.removeEventListener('click', userCallback, {useCapture: false});
// the existing eventTask which created by zoneB will be removed.

assert.toStrictEqual(userCallback[{useCapture: false}], undefined);

mhevery · 2017-06-06T17:39:35Z

@JiaLiPassion yes that sounds right. To summarize:

Subsequent calls to addEventListener with same callback are noops (regardless of zone). (third argument such as useCapture seems to have no effect, ie, uniqueness of callback is all that matters)
Calls to removeEventListener are zone independent.

Here is my take on pseudo code.

// lazy create a zoneAwareCallback and cache it in a zone independent way.
function getZoneAwareCallback(callback) {
  // se if we already have a callback
  let zoneAwareCallback = callback.zoneReturnCallback;
  if (!zoneReturnCallback) {
   // Create a callback but don't associated it with a zone.
    zoneAwareCallback = function(event) { 
      // retrieve the zone form the EventTarget using the event and our ID.
      const zone = this[ZONE_INFO][zoneAwareCallback.symbol + event.name];
      return zone.runGuarde(callback, this, [event]); 
    }
    // create a unique symbol for this callback
    zoneAwareCallback.symbol = '__zone_symbol__' + (counter++) + '_' 
    callback.zoneAwareCallback = zoneAwareCallaback;
  }
  return zoneAwareCallback;
}

function addEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  // lazy create a ZoneInfo. We could patch the callback symbol directly, but that would make the
  // EventTarget shape unpredictable and could negatively impact performance.  So we do the safe
  // thing and put it on a separate object.
  let zoneInfo = this[ZONE_INFO];
  if (!zoneInfo) zoneInfo = this[ZONE_INFO] = {};
  const zoneEventSymbol = zoneAwerCallback + event
  if (!zoneInfo[zoneEventSymbol]) {
    zoneInfo[zoneEventSymbol] = Zone.current;
  }
  return nativeAddEventListener(event, zoneAwareCallback, options);
}

function removeEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  let zoneInfo = this[ZONE_INFO];
  if (zoneInfo) {
    zoneInfo[zoneAwerCallback.symbol + event] = null;
  }
  return nativeRemoveEventListener(event, zoneAwareCallback, options);
}

JiaLiPassion · 2017-06-06T18:20:47Z

@mhevery .

Subsequent calls to addEventListener with same callback are noops (regardless of zone). (third argument such as useCapture seems to have no effect, ie, uniqueness of callback is all that matters)

Calls to removeEventListener are zone independent.

Yes, I agree. but it seems useCapture does matters, I just tested with the following code.

        var div = document.getElementById('thetext');
        const clickHandler = function() {
          console.log('clicked');
        }
        div.addEventListener('click', clickHandler, true);
        div.addEventListener('click', clickHandler, false);
        div.removeEventListener('click', clickHandler, true);

without div.removeEventListener('click', clickHandler, true);, when I click div, the handler will be triggered twice.
after div.removeEventListener('click', clickHandler, true);, when I click div, the handler will be triggered once.

Here is my take on pseudo code.

About the code, I have one question. in the case2 I listed above,

function userCallback() {....}
zoneA.run(() => div.addEventListener('click', userCallback, {useCapture: true});
zoneB.run(() => div.addEventListener('click', userCallback, {useCapture: false});

// then removeEventListener, this code may run in zoneA/zoneB or any other zone
div.removeEventListener('click', userCallback, {useCapture: true});
div.removeEventListener('click', userCallback, {useCapture: false});

we should have 2 zoneAwareCallback attached to the userCallback , and those
2 zoneAwareCallback will run on different zone. So we may still need to use useCapture
as a key to store the different zoneAwareCallback.

And because useCapture can't be get from event parameter,
so should the getZoneAwareCallback look like this?

// lazy create a zoneAwareCallback and cache it in a zone independent way.
function getZoneAwareCallback(callback, useCapture) {
  // se if we already have a callback
  let zoneAwareCallbacks = callback.zoneReturnCallbacks;
  if (!zoneAwareCallbacks) {
    zoneAwareCallbacks = {};
  }  
  let zoneAwareCallback = zoneAwareCallbacks[useCapture.toString()];
  if (!zoneReturnCallback) {
   // Create a callback but don't associated it with a zone.
    zoneAwareCallback = function(event) { 
      // retrieve the zone form the EventTarget using the event and our ID and useCapture flag.
      const zone = this[ZONE_INFO][zoneAwareCallback.symbol + event.name + useCapture];
      return zone.runGuarde(callback, this, [event]); 
    }
    // create a unique symbol for this callback
    zoneAwareCallback.symbol = '__zone_symbol__' + (counter++) + '_' 
    zoneAwareCallbacks[useCapture.toString()] = zoneAwareCallback;
  }
  return zoneAwareCallback;
}

please review.

mhevery · 2017-06-06T23:05:09Z

I agree with this change:

// retrieve the zone form the EventTarget using the event and our ID and useCapture flag.
const zone = this[ZONE_INFO][zoneAwareCallback.symbol + event.name + useCapture];

I would improve on this line as:

if (useCapture) {
  zoneAwareCallbacks.zoneAwareCaptureCallback= zoneAwareCallback;
} else {
  zoneAwareCallbacks.zoneAwareCallback= zoneAwareCallback;
}

Other than that this looks great.

JiaLiPassion · 2017-06-06T23:44:32Z

@mhevery , got it, I will implement this one!

JiaLiPassion · 2017-06-10T10:08:13Z

@mhevery , we discussed this one yesterday that we still need to schedule the eventTask.
so the code will look like.

function addEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  // lazy create a ZoneInfo. We could patch the callback symbol directly, but that would make the
  // EventTarget shape unpredictable and could negatively impact performance.  So we do the safe
  // thing and put it on a separate object.
  const zone = Zone.current;
  let zoneInfo = this[ZONE_INFO];
  if (!zoneInfo) zoneInfo = this[ZONE_INFO] = {};
  const zoneEventSymbol = zoneAwareCallback.symbol + event + useCapture;
   if (!zoneInfo[zoneEventSymbol]) {
	// save task information so we can get task when 
	// removeEventListener
    zoneInfo[zoneEventSymbol] = zone;
  }
  Zone.current.scheduleEventTask(source, zoneAwareCallback, ....);
}

But if we `scheduleEventTask` like this

we will call nativeAddEventListener.apply(target, [eventName, task.invoke]);
So we will already know the zone information in the zoneAwareCallback. we don not need
the ZONE_INFO to keep the zone.
And because we add task.invoke with nativeAddEventListener, in patched removeEventListener, we need to be able to find eventTask from the rawCallback.

function removeEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  let zoneInfo = this[ZONE_INFO];
  if (zoneInfo) {
    zoneInfo[zoneAwerCallback.symbol + event] = null;
  }
  // we need to be able to get ZoneTask from callback or zoneAwareCallback
  const task = ???
  return nativeRemoveEventListener(event, task.invoke, options);
}

So we still need to keep task somewhere which can be searched by callback.
But if we do that, memory and performance will become the same with original version.

So maybe the original idea below means don't using `EventTask`...?

// lazy create a zoneAwareCallback and cache it in a zone independent way.
function getZoneAwareCallback(callback) {
  // se if we already have a callback
  let zoneAwareCallback = callback.zoneReturnCallback;
  if (!zoneReturnCallback) {
   // Create a callback but don't associated it with a zone.
    zoneAwareCallback = function(event) { 
      // retrieve the zone form the EventTarget using the event and our ID.
      const zone = this[ZONE_INFO][zoneAwareCallback.symbol + event.name];
      return zone.runGuarde(callback, this, [event]); 
    }
    // create a unique symbol for this callback
    zoneAwareCallback.symbol = '__zone_symbol__' + (counter++) + '_' 
    callback.zoneAwareCallback = zoneAwareCallaback;
  }
  return zoneAwareCallback;
}

function addEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  // lazy create a ZoneInfo. We could patch the callback symbol directly, but that would make the
  // EventTarget shape unpredictable and could negatively impact performance.  So we do the safe
  // thing and put it on a separate object.
  let zoneInfo = this[ZONE_INFO];
  if (!zoneInfo) zoneInfo = this[ZONE_INFO] = {};
  const zoneEventSymbol = zoneAwerCallback + event
  if (!zoneInfo[zoneEventSymbol]) {
    zoneInfo[zoneEventSymbol] = Zone.current;
  }
  return nativeAddEventListener(event, zoneAwareCallback, options);
}

function removeEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  let zoneInfo = this[ZONE_INFO];
  if (zoneInfo) {
    zoneInfo[zoneAwerCallback.symbol + event] = null;
  }
  return nativeRemoveEventListener(event, zoneAwareCallback, options);
}

mhevery · 2017-06-11T02:16:54Z

I forget about the Tasks. Here it is rewritten with ZoneTask in mind.

// lazy create a zoneAwareCallback and cache it in a zone independent way.
function getZoneAwareCallback(callback) {
  // see if we already have a callback
  let zoneAwareCallback = callback.zoneReturnCallback;
  if (!zoneAwareCallback) {
   // Create a callback but don't associated it with a zone.
    zoneAwareCallback = function(event) { 
      // retrieve the zone form the EventTarget using the event and our ID.
      const task = this[TASK_INFO][zoneAwareCallback.symbol + event.name];
      return task.invoke.apply(this, arguments); 
    }
    // create a unique symbol for this callback
    zoneAwareCallback.symbol = '__zone_symbol__' + (counter++) + '_' 
    callback.zoneAwareCallback = zoneAwareCallback;
  }
  return zoneAwareCallback;
}

function customSchedule() {
  nativeAddEventListener(this.data.event, this.data.zoneAwareCallback, this.data.options);
}
function customCancel() {
  nativeRemoveEventListener(this.data.event, this.data.zoneAwareCallback, this.data.options);
}

function addEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  // lazy create a ZoneInfo. We could patch the callback symbol directly, but that would make the
  // EventTarget shape unpredictable and could negatively impact performance.  So we do the safe
  // thing and put it on a separate object.
  let taskInfo = this[TASK_INFO];
  if (!taskInfo) taskInfo = this[TASK_INFO] = {};
  const zoneTaskSymbol = zoneAwareCallback.symbol + event
  if (!taskInfo[zoneTaskSymbol]) {
    taskInfo[zoneTaskSymbol] = Zone.current.scheduleMacroTask(
      '...', callback, {event, zoneAwareCallback, options}, 
      customSchedule, customCancel);
  }
  
}

function removeEventListener(event, callback, options) {
  const zoneAwareCallback = getZoneAwareCallback(callback);
  let taskInfo = this[TASK_INFO];
  if (taskInfo) {
    const zoneTaskSymbol = zoneAwareCallback.symbol + event
    if (taskInfo[zoneTaskSymbol]) {
      const task: ZoneTask = taskInfo[zoneTaskSymbol];
      task.customCancel();
      taskInfo[zoneTaskSymbol] = null;
    }
  }
  return nativeRemoveEventListener(event, zoneAwareCallback, options);
}

…tener performance

…ner performance

…erformance (#812) * feat(performance): fix #798, improve EventTarget.addEventListener performance * add test cases * add comment to explain the implementations * add test cases for resschedule eventTask * add IE/edge and cross context check * modify document for extra flags * minor changes, define some const, move logic into __load_patch * let nodejs use new addListener/removeListener method * add nodejs test example * add eventListeners/removeAllListeners test cases for browser

pkozlowski-opensource · 2017-07-13T12:08:26Z

@JiaLiPassion @mhevery thnx so much for all the work done on this issue. I've just updated to the latest zone.js (0.8.13) and run benchmarks on my sample app (I'm using it to track Angular perf - it is basically a table with lots of views and ~200 event listeners). Here are the results from my machine:

Before

    forcedGcAmount |       forcedGcTime |           gcAmount |             gcTime |        majorGcTime |     pureScriptTime |         renderTime |         scriptTime
------------------ | ------------------ | ------------------ | ------------------ | ------------------ | ------------------ | ------------------ | ------------------
           3345.75 |               6.61 |             604.05 |               6.10 |               0.01 |              77.33 |              88.79 |              83.43
           3260.74 |               7.31 |             856.98 |               5.43 |               0.01 |              50.61 |              41.36 |              50.61
           3264.73 |               6.55 |             857.93 |               3.98 |               0.01 |              57.32 |              39.90 |              57.32
           3268.59 |               6.56 |             851.98 |               2.87 |               0.01 |              55.16 |              47.82 |              55.16
           3255.20 |               6.44 |             854.98 |               5.21 |               0.01 |              44.69 |              41.17 |              44.69
           3234.17 |               6.78 |             849.86 |               5.00 |               0.01 |              47.42 |              43.26 |              47.42
           3293.90 |               6.59 |             899.10 |               3.27 |               0.01 |              53.81 |              41.72 |              53.81
           3316.69 |               7.51 |             892.16 |               3.32 |               0.01 |              55.78 |              42.06 |              55.78
           2657.14 |               6.69 |            1440.78 |               4.99 |               0.01 |              43.20 |              39.87 |              43.20
           2661.98 |               7.27 |            1440.78 |               4.76 |               0.01 |              52.42 |              42.73 |              52.42
           2657.19 |               7.08 |            1440.27 |               5.15 |               0.01 |              39.22 |              43.21 |              39.22
           4091.68 |              12.55 |               0.00 |               0.00 |               0.00 |              40.00 |              40.89 |              40.00
           4093.99 |              12.63 |               0.00 |               0.00 |               0.00 |              39.91 |              41.10 |              39.91
           2659.42 |               6.51 |            1433.29 |               4.77 |               0.01 |              41.68 |              41.78 |              41.68
           2660.37 |               6.65 |            1435.87 |               4.41 |               0.01 |              42.18 |              41.80 |              42.18
           4091.52 |              12.00 |               0.00 |               0.00 |               0.00 |              42.31 |              46.90 |              42.31
           4093.86 |              11.62 |               0.00 |               0.00 |               0.00 |              41.95 |              41.46 |              41.95
           2663.57 |               6.80 |            1435.90 |               4.94 |               0.01 |              42.43 |              41.45 |              42.43
           4111.86 |              11.77 |               0.00 |               0.00 |               0.00 |              41.41 |              44.19 |              41.41
           2660.90 |               7.80 |            1432.01 |               5.00 |               0.01 |              44.32 |              42.83 |              44.32
           2657.80 |               7.08 |            1435.61 |               4.49 |               0.00 |              44.26 |              44.15 |              44.26
           4092.74 |              11.15 |               0.00 |               0.00 |               0.00 |              39.17 |              39.34 |              39.17
           4090.50 |              10.35 |               0.00 |               0.00 |               0.00 |              39.88 |              43.11 |              39.88
           2657.40 |               6.40 |            1436.37 |               4.10 |               0.00 |              40.06 |              40.17 |              40.06
           2657.76 |               7.36 |            1435.06 |               4.74 |               0.01 |              41.07 |              40.20 |              41.07
           2659.80 |               6.56 |            1435.87 |               5.76 |               0.01 |              40.89 |              44.53 |              40.89
           4090.50 |              12.04 |               0.00 |               0.00 |               0.00 |              39.18 |              38.09 |              39.18
           4091.29 |              11.64 |               0.00 |               0.00 |               0.00 |              36.85 |              44.03 |              36.85
           2657.14 |               6.56 |            1435.38 |               4.21 |               0.00 |              37.38 |              39.70 |              37.38
           4114.40 |              10.62 |               0.00 |               0.00 |               0.00 |              43.40 |              47.58 |              43.40
           2656.74 |               7.06 |            1435.62 |               4.79 |               0.00 |              37.58 |              40.76 |              37.58
           4091.53 |              11.51 |               0.00 |               0.00 |               0.00 |              39.13 |              38.99 |              39.13
           2656.75 |               7.35 |            1435.13 |               4.89 |               0.01 |              35.26 |              41.23 |              35.26
           4094.34 |              12.23 |               0.00 |               0.00 |               0.00 |              38.28 |              38.79 |              38.28
           2659.02 |               6.84 |            1435.08 |               4.99 |               0.01 |              39.53 |              41.77 |              39.53
           4093.63 |              12.42 |               0.00 |               0.00 |               0.00 |              40.29 |              39.18 |              40.29
           2656.23 |               7.30 |            1435.54 |               4.35 |               0.00 |              37.02 |              39.56 |              37.02
           2656.38 |               7.43 |            1436.46 |               4.43 |               0.00 |              32.89 |              40.52 |              32.89
           2656.13 |               7.54 |            1435.67 |               4.52 |               0.00 |              34.41 |              40.22 |              34.41
           2657.38 |               6.78 |            1436.42 |               4.40 |               0.00 |              39.78 |              41.65 |              39.78
           4108.18 |              11.77 |               0.00 |               0.00 |               0.00 |              37.98 |              42.22 |              37.98
           2656.86 |               7.53 |            1435.90 |               4.48 |               0.01 |              37.04 |              47.03 |              37.04
           2656.59 |               6.53 |            1435.54 |               4.45 |               0.00 |              38.06 |              43.54 |              38.06
           4091.49 |              11.40 |               0.00 |               0.00 |               0.00 |              37.55 |              39.27 |              37.55
           2657.86 |               6.67 |            1435.10 |               4.65 |               0.01 |              40.55 |              42.88 |              40.55
           2656.10 |               6.51 |            1436.45 |               4.36 |               0.00 |              36.95 |              41.67 |              36.95
           2657.50 |               7.86 |            1434.27 |               5.30 |               0.01 |              39.16 |              42.24 |              39.16
           2656.95 |               6.57 |            1434.43 |               4.56 |               0.01 |              34.54 |              41.79 |              34.54
           2676.14 |               6.90 |            1434.20 |               4.31 |               0.00 |              38.86 |              38.63 |              38.86
           2657.92 |               7.41 |            1434.54 |               4.93 |               0.02 |              37.46 |              40.21 |              37.46
================== | ================== | ================== | ================== | ================== | ================== | ================== | ==================
      3017.69+-20% |          8.28+-25% |       1076.52+-57% |          3.47+-58% |         0.00+-154% |          37.62+-5% |          41.11+-4% |          37.62+-5%

After

    forcedGcAmount |       forcedGcTime |           gcAmount |             gcTime |        majorGcTime |     pureScriptTime |         renderTime |         scriptTime
------------------ | ------------------ | ------------------ | ------------------ | ------------------ | ------------------ | ------------------ | ------------------
           2657.98 |               8.80 |             814.88 |               9.56 |               0.02 |              67.89 |              91.84 |              67.89
           3743.12 |               8.59 |               0.00 |               0.00 |               0.00 |              56.63 |              38.31 |              56.63
           3740.98 |               8.37 |               0.00 |               0.00 |               0.00 |              54.71 |              35.99 |              54.71
           3738.70 |               8.92 |               0.00 |               0.00 |               0.00 |              52.56 |              38.56 |              52.56
           3732.78 |               8.56 |               0.00 |               0.00 |               0.00 |              52.15 |              36.12 |              52.15
           3729.24 |               8.81 |               0.00 |               0.00 |               0.00 |              46.42 |              36.61 |              46.42
           3730.22 |               8.84 |               0.00 |               0.00 |               0.00 |              45.24 |              36.55 |              45.24
           3728.74 |               8.43 |               0.00 |               0.00 |               0.00 |              43.65 |              35.53 |              43.65
           3728.16 |               8.88 |               0.00 |               0.00 |               0.00 |              39.60 |              37.09 |              39.60
           3730.10 |               9.44 |               0.00 |               0.00 |               0.00 |              43.61 |              36.22 |              43.61
           3747.30 |               8.49 |               0.00 |               0.00 |               0.00 |              41.06 |              39.59 |              41.06
           3728.82 |               8.71 |               0.00 |               0.00 |               0.00 |              44.10 |              41.00 |              44.10
           3726.43 |               8.49 |               0.00 |               0.00 |               0.00 |              38.47 |              36.58 |              38.47
           3729.98 |               9.29 |               0.00 |               0.00 |               0.00 |              38.98 |              38.02 |              38.98
           3726.70 |               8.85 |               0.00 |               0.00 |               0.00 |              44.11 |              36.72 |              44.11
           3851.16 |               9.45 |               0.00 |               0.00 |               0.00 |              43.64 |              35.77 |              43.64
           3727.94 |               9.26 |               0.00 |               0.00 |               0.00 |              40.93 |              36.53 |              40.93
           3725.86 |               8.98 |               0.00 |               0.00 |               0.00 |              42.74 |              39.56 |              42.74
           3723.42 |               9.01 |               0.00 |               0.00 |               0.00 |              37.40 |              38.70 |              37.40
           3729.53 |               9.07 |               0.00 |               0.00 |               0.00 |              42.23 |              37.71 |              42.23
           3722.47 |              10.08 |               0.00 |               0.00 |               0.00 |              42.11 |              36.71 |              42.11
           3746.98 |               8.61 |               0.00 |               0.00 |               0.00 |              41.85 |              38.77 |              41.85
           3725.44 |               8.48 |               0.00 |               0.00 |               0.00 |              34.85 |              37.34 |              34.85
           3727.87 |               8.58 |               0.00 |               0.00 |               0.00 |              41.43 |              37.59 |              41.43
           3730.45 |               8.65 |               0.00 |               0.00 |               0.00 |              39.54 |              36.76 |              39.54
           3728.21 |               8.91 |               0.00 |               0.00 |               0.00 |              41.81 |              36.22 |              41.81
           3726.01 |               9.44 |               0.00 |               0.00 |               0.00 |              39.23 |              37.18 |              39.23
           3722.14 |               9.28 |               0.00 |               0.00 |               0.00 |              39.87 |              36.85 |              39.87
           3721.42 |               9.07 |               0.00 |               0.00 |               0.00 |              37.79 |              36.25 |              37.79
           3723.46 |               8.67 |               0.00 |               0.00 |               0.00 |              43.96 |              37.34 |              43.96
           3726.66 |               9.85 |               0.00 |               0.00 |               0.00 |              40.70 |              38.90 |              40.70
           3726.64 |               8.61 |               0.00 |               0.00 |               0.00 |              39.62 |              37.42 |              39.62
           3746.09 |               8.89 |               0.00 |               0.00 |               0.00 |              41.13 |              37.25 |              41.13
           3726.61 |               9.09 |               0.00 |               0.00 |               0.00 |              38.17 |              36.51 |              38.17
           3738.92 |               8.80 |               0.00 |               0.00 |               0.00 |              43.38 |              35.92 |              43.38
           3722.33 |               9.91 |               0.00 |               0.00 |               0.00 |              37.41 |              37.41 |              37.41
           3725.46 |               9.05 |               0.00 |               0.00 |               0.00 |              33.45 |              39.32 |              33.45
           3727.57 |               9.29 |               0.00 |               0.00 |               0.00 |              37.04 |              37.02 |              37.04
           3721.41 |               9.68 |               0.00 |               0.00 |               0.00 |              37.99 |              37.04 |              37.99
           3724.90 |               8.61 |               0.00 |               0.00 |               0.00 |              41.60 |              37.32 |              41.60
           3725.30 |               9.00 |               0.00 |               0.00 |               0.00 |              38.15 |              37.67 |              38.15
           3726.19 |               8.95 |               0.00 |               0.00 |               0.00 |              40.59 |              37.35 |              40.59
           3725.47 |               8.50 |               0.00 |               0.00 |               0.00 |              37.08 |              35.85 |              37.08
           3746.37 |               8.61 |               0.00 |               0.00 |               0.00 |              39.78 |              37.39 |              39.78
           3726.55 |               9.84 |               0.00 |               0.00 |               0.00 |              40.12 |              37.39 |              40.12
           3726.22 |               8.78 |               0.00 |               0.00 |               0.00 |              35.97 |              37.52 |              35.97
           3724.99 |               8.99 |               0.00 |               0.00 |               0.00 |              38.80 |              38.97 |              38.80
           3724.76 |               8.81 |               0.00 |               0.00 |               0.00 |              39.23 |              36.91 |              39.23
           3724.21 |               9.80 |               0.00 |               0.00 |               0.00 |              35.72 |              38.63 |              35.72
           3725.84 |               9.03 |               0.00 |               0.00 |               0.00 |              41.88 |              38.15 |              41.88
           3723.70 |               9.63 |               0.00 |               0.00 |               0.00 |              36.33 |              37.10 |              36.33
           3743.74 |               8.76 |               0.00 |               0.00 |               0.00 |              38.72 |              38.75 |              38.72
           3724.31 |               9.23 |               0.00 |               0.00 |               0.00 |              34.96 |              39.13 |              34.96
           3725.19 |               8.61 |               0.00 |               0.00 |               0.00 |              37.11 |              39.13 |              37.11
           3744.38 |               8.75 |               0.00 |               0.00 |               0.00 |              41.69 |              36.02 |              41.69
================== | ================== | ================== | ================== | ================== | ================== | ================== | ==================
       3727.94+-0% |           9.09+-4% |               0.00 |               0.00 |               0.00 |          38.18+-5% |          37.70+-2% |          38.18+-5%

Summary

We can see 10+% improvement on the pureScript time which quite frankly is more than I've expected :-) But GC might be the factor here as GC amounts and times dropped dramatically!

Once again, thnx for all the efforts here!

JiaLiPassion · 2017-07-13T13:14:51Z

@pkozlowski-opensource , thank you very much! very glad to see that the performance improved!

JiaLiPassion · 2017-07-14T09:20:56Z

@pkozlowski-opensource , could you post your sample app for testing the perf of angular? I just fixed some bugs because of this changes, I want to check the performance changes again, thank you.

JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 11, 2017

feat(eventListener): fix angular#798, improve EventTarget.addEventLis…

7df50c7

…tener performance

JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 11, 2017

feat(eventListener): fix angular#798, improve EventTarget.addEventLis…

b87b3fe

…tener performance

JiaLiPassion mentioned this issue Jun 11, 2017

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

Merged

JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2017

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

5c2fc04

…ner performance

JiaLiPassion added a commit to JiaLiPassion/zone.js that referenced this issue Jun 22, 2017

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

fb03d4c

…ner performance

mhevery closed this as completed in #812 Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slow addEventListener performance. #798

Slow addEventListener performance. #798

mhevery commented Jun 2, 2017 •

edited

Loading

JiaLiPassion commented Jun 2, 2017 •

edited

Loading

mhevery commented Jun 3, 2017

JiaLiPassion commented Jun 3, 2017 •

edited

Loading

mhevery commented Jun 6, 2017

JiaLiPassion commented Jun 6, 2017 •

edited

Loading

mhevery commented Jun 6, 2017 •

edited

Loading

JiaLiPassion commented Jun 6, 2017

JiaLiPassion commented Jun 10, 2017 •

edited

Loading

mhevery commented Jun 11, 2017

pkozlowski-opensource commented Jul 13, 2017

JiaLiPassion commented Jul 13, 2017

JiaLiPassion commented Jul 14, 2017

Slow addEventListener performance. #798

Slow addEventListener performance. #798

Comments

mhevery commented Jun 2, 2017 • edited Loading

Issue

Root Cause

Naive Approach

Suggested fix: Modification of Naive Fix.

Related Code

JiaLiPassion commented Jun 2, 2017 • edited Loading

Suggested fix: Modification of Naive Fix.

addEventListener

mhevery commented Jun 3, 2017

JiaLiPassion commented Jun 3, 2017 • edited Loading

Case1

Current version of zone.js

Case2

Current version of zone.js

mhevery commented Jun 6, 2017

JiaLiPassion commented Jun 6, 2017 • edited Loading

mhevery commented Jun 6, 2017 • edited Loading

JiaLiPassion commented Jun 6, 2017

JiaLiPassion commented Jun 10, 2017 • edited Loading

But if we scheduleEventTask like this

So maybe the original idea below means don't using EventTask...?

mhevery commented Jun 11, 2017

pkozlowski-opensource commented Jul 13, 2017

Before

After

Summary

JiaLiPassion commented Jul 13, 2017

JiaLiPassion commented Jul 14, 2017

mhevery commented Jun 2, 2017 •

edited

Loading

JiaLiPassion commented Jun 2, 2017 •

edited

Loading

JiaLiPassion commented Jun 3, 2017 •

edited

Loading

JiaLiPassion commented Jun 6, 2017 •

edited

Loading

mhevery commented Jun 6, 2017 •

edited

Loading

JiaLiPassion commented Jun 10, 2017 •

edited

Loading

But if we `scheduleEventTask` like this

So maybe the original idea below means don't using `EventTask`...?