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

Core: Remove delay from setTimeouts in processing code #1246

Merged
merged 1 commit into from
Jan 7, 2018

Conversation

trentmwillis
Copy link
Member

Per #1245 these can build up and cause larger test suites to have some significant performance hits. They don't seem to be needed and I can't find any reason in the history as to why they are needed. In fact, the one comment I can find relating to them is a suggestion to remove it.

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

👍

@trentmwillis
Copy link
Member Author

@qunitjs/qunit-team would appreciate a review. @Krinkle, I think you've been with the project the longest, do you know of any reason to not remove these?

Copy link
Contributor

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

This gets my vote.

@Turbo87
Copy link
Member

Turbo87 commented Jan 5, 2018

Sweet! Nice work!

@jzaefferer
Copy link
Member

I have a very vague memory of these helping the browser UI getting updated between test or even assertion runs. That might have been needed in 2008... As I apparently commented before, let's get rid of them!

@wycats
Copy link

wycats commented Jan 5, 2018

This looks great!

Does anyone know the significance of the magic value 13?

@trentmwillis
Copy link
Member Author

Thanks for chiming in @jzaefferer. I tend to think this may have had value at some point way back when (even the first commit to this repo had the start function using 13 ms timeout), but since then it has likely just been carried forward.

I'm going to land this after a bit more testing. If it breaks anything after the next release, we can always rollback.

@wycats
Copy link

wycats commented Jan 6, 2018

I couldn't put this down, so I kept digging. It looks like the magic number 13 came from jQuery, which had it in its initial commit 13 years ago!

https://github.com/jquery/jquery/blame/130b8a1c030ae7888934fcd8dee6695eac271fcb/fx/fx.js

@Krinkle
Copy link
Member

Krinkle commented Jan 6, 2018

@trentmwillis Sounds good to me. This shouldn't be needed indeed. The implicit default of ~ 4ms should suffice. I couldn't find any evidence either as for why this would need a larger delay.

Within QUnit, the oldest commit is 9b5085a (8 years ago). @wycats Nice find on tracing it back to jquery-core!

@trentmwillis Just one minor clarification in case there is any doubt, this commit will not remove the async delay. It just removes the delay parameter, thereby effectively reducing the duration of the delay from ~13ms to ~4ms.

I suspect you do know that, but I'm just mentioning it here for future reference, and because the linked comment that suggests removal mentions "can run immediately" which seems potentially confusing. setTimeout always runs the callback from a future tick of the event loop, after the existing call stack and any micro tasks finish. With an additional upfront default and minimum delay of approximately 4ms. No parameter (not even 0) can reduce this delay further.

@trentmwillis
Copy link
Member Author

Good points, this one, in particular, is important to call out

...this commit will not remove the async delay. It just removes the delay parameter...

I'll update the commit message to reflect that the delay isn't being removed entirely.

@trentmwillis trentmwillis merged commit 89f3611 into qunitjs:master Jan 7, 2018
@trentmwillis trentmwillis deleted the remove-delay branch January 7, 2018 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants