Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow internal nextTick utility to use microtasks when possible. #370

Merged
merged 1 commit into from Oct 23, 2018

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 26, 2018

No description provided.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my limited understanding of these things this looks okay to me in general. it does slightly change the semantics of the lib though, so might need a bit of real world testing before getting merged/released.

@cibernox
Copy link
Contributor

On that topic, may this be a problem if we don't do the same in https://github.com/emberjs/ember.js/blob/master/packages/ember-testing/lib/test/pending_requests.js#L16-L23 ?

@@ -1,6 +1,7 @@
import { Promise } from 'rsvp';

export const nextTick = setTimeout;
export const nextTick =
typeof Promise === 'undefined' ? setTimeout : cb => Promise.resolve().then(cb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't that conflict with the nextTickPromise() description below which talks about "the next turn of the event loop"?

@Turbo87
Copy link
Member

Turbo87 commented Apr 26, 2018

I've just tried this branch out on one of our larger apps, and it worked without any issues. Unfortunately I also did not see any significant speed improvements though 🤔

@rwjblue
Copy link
Member Author

rwjblue commented Oct 8, 2018

Rebased against current master (which is finally green against various dep changes dropping Node 4 without major bumps)...

@scalvert
Copy link
Contributor

scalvert commented Oct 8, 2018

This seems like a good change.

@Turbo87 it surprises me that you didn't see an improvement in speed, considering we're moving from macro to microtasks. how big was your test suite?

@rwjblue
Copy link
Member Author

rwjblue commented Oct 9, 2018

I think it depends on the suite actually. For example, if the suite was still using a large number of moduleForAcceptance tests or legitimately needs additional time for the various waitUntil calls (e.g. using animations with set durations) I don't think you would necessarily see a decrease.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 9, 2018

I do agree with #370 (review) though, I'll try to test in some OSS apps (and internally) soon.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 23, 2018

Tested in a few apps without issue, going to :shipit:...

@rwjblue rwjblue changed the title [WIP] Allow internal nextTick utility to use microtasks when possible. Allow internal nextTick utility to use microtasks when possible. Oct 23, 2018
@rwjblue rwjblue merged commit 2fbd581 into emberjs:master Oct 23, 2018
@rwjblue rwjblue deleted the micro-task-next-tick branch October 23, 2018 13:34
@Turbo87
Copy link
Member

Turbo87 commented Oct 23, 2018

@scalvert sorry, didn't see your comment. I'm not entirely sure anymore which app it was that I tested but I think it was about 3000 tests. The time it takes for the test suite to run was about 5min using ember-exam with 2 or 3 partitions and it did not improve by a significant amount.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 23, 2018

Ya, FWIW, I don't expect this to dramatically improve all scenarios, but it does reduce at least one of the sources of significant idle times.

I made a quick set of JSBin's to demo the difference:

This is clearly not doing anything useful, I mainly made them this way to show the general overhead of running a "simple" test. After doing about 10 runs of each of those, the microtask one is generally ~ 4s (40%) faster.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 23, 2018

@Turbo87 - You also see no change because this code was wrong 😝. Basically the feature detect for Promise was being overriden by the import { Promise } from 'rsvp' bit 🕵️ . Fixing in #443.

@Turbo87
Copy link
Member

Turbo87 commented Oct 23, 2018

interesting, I didn't know that it wouldn't work with RSVP Promises. the app that I probably tried this on is using the unforunate window.Promise = RSVP.Promise; workaround, so that might also be the reason why I did not see any improvements

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

Successfully merging this pull request may close these issues.

None yet

4 participants