Ticket #2532411 (checkedin enhancement)
Reporter  Satyen Desai |
Opened: 06/12/12 Last modified: 04/18/13 Status: checkedin Type: enhancement |
Owner Satyen Desai |
Target Release: 3.CURRENT.NEXT Priority: P3 (normal) |
|---|---|---|---|
| Summary: | Pursue Low Hanging Event Performance Optimizations | ||
| Description: | a) Optimizing the mix/merge usage - for example, that's the main cost of publish currently - changing it to the _wlmix impl in Y.Base may help. b) The above point may remove the need for lazy publish, but if not, look at lazy publish. c) Revisiting subscriber maps : May be make them arrays [with deprecation path, since they're theoretically public]. 1) We rely on object key iteration to maintain subscriber order, which seems fragile. d) Revisit getType() calls - Aside from mix/merge, cached is the other thing which pops up in a profile. Maybe modify private APIs to take type/prefix instead of re-fetching it each time. e) See if we can do less work if (!broadcast && !(bubbled && hasTargets) && !hasSubs) |
||
| Type: | enhancement | Observed in Version: | development master |
| Component: | Event | Severity: | S3 (normal) |
| Assigned To: | Satyen Desai | Target Release: | 3.CURRENT.NEXT |
| Location: | Library Code | Priority: | P3 (normal) |
| Tags: | Relates To: | #2533088 | |
| Browsers: | N/A | ||
| URL: | |||
| Test Information: | NOTE: This won't be commited for PR3, due to the fact that it's a low level / critical component. However the goal is to have something commitable the first day of 3.7.0 pr1. |
||
Change History
|
Posted: 06/16/12
|
|
Posted: 06/16/12
Forgot to add - worth capturing, so we can track potential regressions. This is the unit test state as of the above commit on Gecko/Chrome/Safari/MobileSafari, for all current event unit tests, plus some of the more complex event usage: Attribute, Base, DD, App, Widget, Widget ParentChild [1]. ✔ Delegate on Safari (5.1.3) / Mac OS 5 passed, 4 failed ✔ ContextMenu on Firefox (12.0) / Mac OS [1] The ContextMenu test failures are not a regression. I assume they are expected to fail on iOS, due to lack of a contextmenu event. The event sequence and event simulation tests don't run under YETI (old version, not NEXT), but do pass manually, so they're not reflected in the list above |
|
Posted: 06/19/12
Event Facade optimizations, checked into the same branch, until we're ready to merge: https://github.com/sdesai/yui3/commit/29f63996f8b69a7bb6d2e27f4d350c320998c0b2 |
|
Posted: 06/20/12
cc'ing folks to gauge if it's worth trying to get this into 3.6.0 as work progresses. IMO it's risky, since it's relatively late in 3.6.0 - highly likely to introduce a 3.6.1. Whether that risk is worth it or not can be discussed. It may be, seems like it has major benefits. |
|
Posted: 06/20/12
Yes please! |
|
Posted: 06/20/12
I did some more performance benchmarks on iOS 5, iOS 6, Safari 6, and IE9; the details are here: |
|
Posted: 06/21/12
Ryan - As an extra confidence data point on top of the unit tests I mention above, would you be able to take my fork and run your app against it, to see if anything nasty jumps out over the next week or two? May be a useful data point, especially if it uses CustomEvents extensively. |
|
Posted: 06/22/12
It'd be next to impossible for me to test the entire app against your fork, but I can try it out and look for obvious breakage. |
|
Posted: 08/2/12
|
|
Posted: 08/2/12
|
|
Posted: 08/2/12
|
|
Posted: 08/2/12
Eric - this will include the _facade reset for memory optimization (couldn't find another bug for it) |
|
Posted: 08/2/12
Satyen, there are like a million Kit-Kats in your future if you can plug all the Event memory leaks. P.S. Two million if you plug all the leaks *and* make it faster. |
|
Posted: 08/2/12
Ah, funny you mention that. We were just discussing my KitKat based economy today and how it has been diluted by Y!'s now free vending machines (wanna come back - we have free pie?). I now have an unlimited supply of kitkats. I'll need to move to another unit of currency. |
|
Posted: 08/2/12
When I dug into why the facades were being held onto, there are a couple of things which hold onto the EventFacade instance: `_facade` and `firedWith`. The `firedWith` array needs to continue holding onto the facade instance when an event is configured to be `fireOnce: true`. |
|
Posted: 08/2/12
Yeah, I was referring to the transient _facade property, where it seems like the intention was to hold onto it for run-time performance reasons, but doesn't seem like it's ever re-used. I want to test bubbling etc. for this case. |
|
Posted: 08/2/12
My point is that the even nulling out the transient `_facade` prop doesn't fix the memory leak, because the facade is still held on to in the `firedWith` property. |
|
Posted: 08/2/12
Yeah, so we'll clean that up for non-fireOnce events. For fireOnce events, we need to hold onto it (as currently designed) |
|
Posted: 08/2/12
|
|
Posted: 08/7/12
Based on the planning meeting, I'm redefining this task, to the scope of work I can commit by EOM with the hope that in can hit a PR, off of master, at the end of this monthly sprint. This is the specific set of tasks which I'm planning to address in the next month, and in time for a PR (regardless of what we name the release at the end of the month): a) Fill out support for an event-custom-base-deprecated to populate the original et.subscribers, et.afters objects. |
|
Posted: 08/14/12
Merged into the 3.x branch. Ready to PR whenever we're ready to create one. Continuing to look at more event optimizations ( d) and e) in list provided in the description ). Also maybe see if it's safe to remove some hasOwnProperty checks in some (heavily trodden) cases. For example, I may be missing something (let me know if I am), but I can't see the value in a hasOwnProperty check for the following snippet: var o1 = { for (prop in o2) { |
|
Posted: 08/15/12
This looks promising. Many 20% paths we could clean up... So in a profile of a publish + 10000 fires (no listeners, for this test case): ET.fire = 904ms So, + ET._monitor is definitely very rarely used, so we could tweak that to avoid that cost for the common case. + ET.getSibiling should also be rare (it finds * subscribers for this event). If we can add flags to avoid monitoring and looking for "*" siblings/subscribers until someone actually uses the monitor API, or uses * to subscribe, we can save on these chunks for the common use case. + For Y.cached, the strategy is outlined above - try and change the private signature to accept a pre-parse prefix/event, so we don't go to parse it multiple times. NOTE: This cost only bubbles up if you have a "prefix" configured - so it's centered around that support. + For getFacade, we can continue to look at mix optimizations. + For Queue - I need to understand if it's really required for all use cases, or just for async support. For all the above, maybe we can short circuit it (at least for the no bubble/no broadcast case) if there are no listeners. Digging in. [ Forgot to add, this is with the latest 3.x code, with the other perf. optimizations from this bug in place already ] |
|
Posted: 08/15/12
Current Numbers, before I move on to the stuff above: https://gist.github.com/3364530 |
|
Posted: 08/17/12
|
|
Posted: 08/17/12
Further event facade mix optimization : https://github.com/sdesai/yui3/commit/627216ec336d9e01a26cfb30f6d0a22bbea8ef3e Benchmark: https://gist.github.com/3374909 |
|
Posted: 08/18/12
Go Satyen go! |
|
Posted: 08/20/12
For all the new cc's: Since this is/will be a pretty broad set of changes to a very complex core layer, your feedback on the next 3.x PR would be *very* valuable in 2 areas: a) Of course, any regressions you see. b) Also, since the art of performance measurement seems to have it's own set of per-run-variances and tool idiosyncrasies, and some of the changes are pretty low-level micro optimizations, if you notice any of these tweaks not showing benefits for your use case, please chime in. From the benchmark tests checked into the code-base, it seems to provide substantial benefits overall so far. The process I've been using is to profile a small snippet using native profiling tools, use it to target methods to fix, and then run benchmark.js for more real-world/complex use cases to make sure we see benefits at the macro level. |
|
Posted: 08/31/12
Please give the PR as spin when you get a chance and let us know if you see anything borked. |
|
Posted: 09/13/12
If you see anything on PR2, speak now, or foreve.... well file a bug against 3.7.0 when it's out. But seriously, this is the last call before GA. Please give PR2 a spin if you can in the next couple of days. No known issues to date. If you're a heavy Custom Event user, and have tried PR1 or PR2, and don't see any (event) regressions, that would be good data to have also. As mentioned above, I don't believe this is it for Custom Event optimizations. There are a couple of bigger chunks to pursue. |
|
Posted: 09/19/12
Moving from 3.6.x to 3.CURRENT.NEXT |
|
Posted: 10/16/12
Since there are a bunch of existing followers on this ticket, using it to continue tracking progress on Event performance for the next sprint. Updating remaining days time estimate and sprint number accordingly. The key focus for this sprint is going to be on: a) Minimizing work done for no target subscribers, no bubbling, no broadcasting use case. |
|
Posted: 10/16/12
Messed up the sprint number. Should be Sprint 3. |
|
Posted: 10/24/12
Pulled off onto something else. Not going to get to this in the next sprint. Someone may pick it up. |
|
Posted: 04/18/13
Moved the details over to here, which is going out with 3.10.0 hopefully: http://yuilibrary.com/projects/yui3/ticket/2533088 |
Initial drop into my fork, on branch event-perf:
https://github.com/sdesai/yui3/commit/830d61b7777d07a909b36fa2d55a05545356465d
[NOTE: This commit is not backwards compatible with 3.5.1. The .subscribers, and .afters hashes on CustomEvent have been removed/replaced with private arrays. When complete, we aim to have an event-custom-deprecated which will restore these properties for folks using them, but will introduce a performance hit]
Numbers, from src/event-custom/tests/benchmark/event-custom-benchmark.js:
Firefox 12/OSX
EventTarget with attribute style publish, fire, with on/after listeners x 7,391 ops/sec [3.6.0pr2 = 5126 ops/sec]
EventTarget with attribute style publish, fire, no listeners x 13,335 ops/sec [3.6.0pr2 = 9,202 ops/sec]
Chrome 19/OSX
EventTarget with attribute style publish, fire, with on/after listeners x 23,642 ops/sec [3.6.0pr2 = 7,623 ops/sec]
EventTarget with attribute style publish, fire, no listeners x 47,977 ops/sec [3.6.0pr2 = 24,685 ops/sec]