New issue
Advanced search Search tips

Issue 752127 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Adding/removing event listeners is slow

Project Member Reported by jbroman@chromium.org, Aug 3 2017

Issue description

It shows up as ~5% of time spent in Blink during V8 execution for a number of Speedometer benchmarks.
 
Didn't expect this to turn into so many CLs. Here's the related CLs so far:

Skip v8::Private::ForApi when adding/removing event listeners.
https://chromium.googlesource.com/chromium/src/+/9ecc7ea64164c15ef9d6e977903a40c270ef8441

Make EventListenerMap::ContainsCapturing after a failed search of the listener vector.
https://chromium.googlesource.com/chromium/src/+/1dcc86ee649345149a5a2351037d8ce9ad8d21fb

Remove hidden value tracking of event listeners.
https://chromium.googlesource.com/chromium/src/+/d68025ec1b431429a939e80cacceeae20dd9ff4a

Use find-only GetEventListener for removeEventListener.
https://chromium.googlesource.com/chromium/src/+/41f021e651f0f97f60af0a131970e81de1d14790
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7d575ed02a7ec6f9dc679b8bbc463fb7a6a47ad

commit f7d575ed02a7ec6f9dc679b8bbc463fb7a6a47ad
Author: Jeremy Roman <jbroman@chromium.org>
Date: Fri Aug 04 13:38:14 2017

Refactor V8EventListenerHelper for speed and simplicity.

This refactor simplifies the header and puts the work into a
the source file, where the bulk of the work is in a file-local
template (which primarily exists to factor out the listener creation
logic).

This sets up for a number of follow-up CLs, which combined with
this one give a 20% perf improvement on a microbenchmark of adding
and removing event listeners.

Bug:  752127 
Change-Id: I3d5b8cea1560833b6ec867d9d35d44318b6db190
Reviewed-on: https://chromium-review.googlesource.com/599427
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492011}
[modify] https://crrev.com/f7d575ed02a7ec6f9dc679b8bbc463fb7a6a47ad/third_party/WebKit/Source/bindings/core/v8/V8EventListenerHelper.cpp
[modify] https://crrev.com/f7d575ed02a7ec6f9dc679b8bbc463fb7a6a47ad/third_party/WebKit/Source/bindings/core/v8/V8EventListenerHelper.h
[modify] https://crrev.com/f7d575ed02a7ec6f9dc679b8bbc463fb7a6a47ad/third_party/WebKit/Source/bindings/scripts/v8_attributes.py

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2e973ac021c770fbc5b21dd51aa4d5d036024c5

commit e2e973ac021c770fbc5b21dd51aa4d5d036024c5
Author: Jeremy Roman <jbroman@chromium.org>
Date: Sat Aug 05 03:03:41 2017

V8EventListenerHelper: Check world ID to determine if this is a worker context.

Currently we use ToLocalDOMWindow on the global object, but that is
something other than a DOMWindow iff this is a worker or worklet, which is
the case iff the script state is associated with a worker world.

This check is faster (follow a pointer and compare an integer).

Bug:  752127 
Change-Id: I7af0f93959a8fb96989c6c2b730da68fafaef421
Reviewed-on: https://chromium-review.googlesource.com/600110
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492223}
[modify] https://crrev.com/e2e973ac021c770fbc5b21dd51aa4d5d036024c5/third_party/WebKit/Source/bindings/core/v8/V8EventListenerHelper.cpp

Whoops, wrong bug on a CL:

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e6d71fdd942a4603a64792591a1a0d79b5bd7e6

commit 1e6d71fdd942a4603a64792591a1a0d79b5bd7e6
Author: Jeremy Roman <jbroman@chromium.org>
Date: Sat Aug 05 01:41:40 2017

V8EventListenerHelper: Replace GetOrEmpty with GetOrUndefined.

The latter only needs to do property lookup once rather than twice.
Undefined can easily be distinguished from a v8::External anyhow.

Bug:  752126 
Change-Id: Id2094d3768ec8e0c7e2567e9fefc32e8a5900e7b
Reviewed-on: https://chromium-review.googlesource.com/600253
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492210}
[modify] https://crrev.com/1e6d71fdd942a4603a64792591a1a0d79b5bd7e6/third_party/WebKit/Source/bindings/core/v8/V8EventListenerHelper.cpp

Status: Fixed (was: Started)

Sign in to add a comment