New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize events, support objects with `handleEvent` #1949

Merged
merged 6 commits into from Sep 18, 2017

Conversation

Projects
None yet
3 participants
@isiahmeadows
Copy link
Collaborator

isiahmeadows commented Aug 25, 2017

Description

Changed/optimized event handling, and added support for objects with handleEvent. I also made a few drive-by fixes, as detailed in the last two commits' messages.

Each of the two main commits could be taken standalone, and if the second is dropped, I can redo the included fixes in a separate PR (they're all mostly trivial).

In addition, the first commit can be backported with little modification to v1 without breakage (unless people actually rely on being able to wrap Mithril-created event handlers).

Motivation and Context

See #1939 for handleEvent. The event subscription changes are to reduce the memory overhead for adding multiple events and changing existing events.

As a result, I had to switch to using addEventListener and removeEventListener exclusively.

How Has This Been Tested?

Reused @spacejack's tests for handleEvent. (I could've cherry-picked them from his branch, but I was too lazy to.)

And yes, I've run the tests locally. 馃槈

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

isiahmeadows added some commits Aug 25, 2017

Reduce memory allocation/usage across multiple event handlers
- `handleEvent` is a very useful tool.
- Always use `addEventListener`/`removeEventListener`, since it's
  required for this optimization.
- Change log updated.
- Drive-by: make DOM mock work with both event listener types.
- Drive-by: eliminate possibility of `Object.prototype` interference.
Add support for object event handlers (using `handleEvent`)
- `handleEvent` is checked on dispatch, like in the DOM.
- Had to reorder attribute key checking so `undefined` events still got
  removed.
- Drive-by: Optimize the initial attribute key checking a little.
- Drive-by: Fix changelog v2.0.0 link in TOC.
o(spy.callCount).equals(0)
})

o("removes event added via addEventListener when null", function() {

This comment has been minimized.

@spacejack

spacejack Aug 25, 2017

Contributor

Some of these tests may be redundant if everything is added with addEventListener unless you want to test MouseEvents and TouchEvents.

This comment has been minimized.

@isiahmeadows

isiahmeadows Aug 26, 2017

Collaborator

I'd rather stay safe, in case someone down the road decides it's a good idea to start using the raw properties again. That way, we're less likely to see regressions.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 31, 2017

@pygy @tivac Could either one of you take a look at this PR? (I'd rather not merge a non-trivial PR like this without review.)

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 31, 2017

Putting some 馃憖 on perf. Changes look fine otherwise.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 31, 2017

@tivac I didn't post my results here, but I did run the suite. It was a mild improvement in speed (and made it more consistent), but that wasn't the primary focus of the optimization - it was to reduce memory/GC pressure.

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 31, 2017

Weird, I was seeing on the order of 1-5% slower perf. I got lost in the weeds some on making some tooling around comparing perf of branches easier though.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 31, 2017

@tivac That's odd...I was seeing that in gain, so I guess we can assume it's within the margin of error. I'll note a pretty important detail: adding/removing event listeners is much more expensive in browsers than with our mock. Did you run the benchmarks in an actual browser or just Node? (I'd expect that if you're benching based on the mock, you should get a mild slowdown due to a slightly increased overhead.)

Another item of note: it no longer fast-paths things like onclick or onsubmit, where there are associated properties, but that's to simplify the combining logic and avoid creating closures where those APIs don't support the EventHandler interface. That part could change the performance curve a little bit.

@spacejack

This comment has been minimized.

Copy link
Contributor

spacejack commented Aug 31, 2017

Using the onclick etc. properties was pretty crude in the way that it would always assign them when it found them on the element, whether or not the callback had changed. Though I don't know how expensive those read/writes are in practice.

Either way, the diff was pretty sketchy due to not recording them in the old vnode.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 31, 2017

@spacejack It's actually almost as cheap as normal property assignment in practice to modify the on{event} properties, since the native logic is so simple. With them, it's faster to just unconditionally overwrite them than to actually diff them. (With non-rendering attributes like data- attributes, it's generally pretty similar.)

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Sep 1, 2017

Ah, yeah that explains it. If it's faster in browsers than I'm fine w/ this.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 1, 2017

@tivac Hold off on the merge until I get a better diagnosis on why I'm getting inconsistent perf benchmarks in the browser. I also need to do a proper memory profile check to verify that. I'll come back once I've got results (and potential fixes).

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 1, 2017

To clarify, the inconsistency is just wider variance in speed. (And it's slightly slower, but I'm going to look into the diff a little bit to see what happened.)

Rework event diffing for better optimizability
Re-ordered the type checks so that I can avoid polymorphic property
lookups in event updates. (It improved the common case of no change by
a little over ~40%.)
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 1, 2017

Okay...so here's some numbers, from benchmarking in Chrome:

Before this patch:

rerender identical vnode x 3,537,534 ops/sec 卤22.62% (52 runs sampled)
rerender same tree x 38,454 ops/sec 卤11.56% (40 runs sampled)
construct large VDOM tree x 24,846 ops/sec 卤4.39% (48 runs sampled)
mutate styles/properties x 14,497 ops/sec 卤6.56% (34 runs sampled)
repeated trees (recycling) x 325 ops/sec 卤10.42% (43 runs sampled)
repeated trees (no recycling) x 336 ops/sec 卤9.99% (45 runs sampled)
Completed perf tests in 95835ms

After this patch:

rerender identical vnode x 3,457,798 ops/sec 卤30.39% (50 runs sampled)
rerender same tree x 77,396 ops/sec 卤5.30% (44 runs sampled)
construct large VDOM tree x 24,041 ops/sec 卤7.03% (43 runs sampled)
mutate styles/properties x 14,640 ops/sec 卤5.48% (40 runs sampled)
repeated trees (recycling) x 329 ops/sec 卤10.01% (39 runs sampled)
repeated trees (no recycling) x 336 ops/sec 卤11.05% (42 runs sampled)
Completed perf tests in 94860ms

I'll come back with memory numbers later.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 3, 2017

From running the memory profiler (to create a timeline), it appears that the memory usage is slightly increased (1.3MB to 1.4MB over 25s or about ~25KB to ~28KB per sample) in the faster benchmark due to more strings being retained. So my memory claims are invalid (at least for 1-2 listeners). 馃槃

Most of the increase is somehow system-related, though, not JS-land stuff - it says the total allocated went from ~40MB to ~50MB, but only about 2MB of that difference was directly JS-related, so I don't know (maybe the profiler itself poisoning the report?). 馃槙

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 3, 2017

@tivac Mind taking another look?

@tivac

tivac approved these changes Sep 17, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 18, 2017

Full merge ahead!

@isiahmeadows isiahmeadows merged commit 98933b8 into MithrilJS:next Sep 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Sep 1, 2018

I added the "backport to v1" label. We can still keep most of the same code, just if it returns false, call ev.preventDefault() + ev.stopPropagation(). That's the only difference to retain compatibility.

Edit: I should've done that in the first place.

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Sep 3, 2018

Unbreak accidental back-compat break with event optimization
This was supposed to be purely additive. See here for more details:

MithrilJS#1949 (comment)

@isiahmeadows isiahmeadows referenced this pull request Sep 3, 2018

Merged

Fix accidental back compat break for event optimization #2222

6 of 11 tasks complete

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Sep 3, 2018

Unbreak accidental back-compat break with event optimization
This was supposed to be purely additive. See here for more details:

MithrilJS#1949 (comment)

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Sep 3, 2018

Unbreak accidental back-compat break with event optimization
This was supposed to be purely additive. See here for more details:

MithrilJS#1949 (comment)

isiahmeadows added a commit that referenced this pull request Sep 18, 2018

Unbreak accidental back-compat break with event optimization (#2222)
This was supposed to be purely additive. See here for more details:

#1949 (comment)

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 12, 2018

Merge pull request MithrilJS#1949 from isiahmeadows/optimize-events
Optimize events, support objects with `handleEvent`

isiahmeadows added a commit to isiahmeadows/mithril.js that referenced this pull request Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment