lib: introduce constant MAX_TICKS #11199

danbev · 2017-02-06T15:07:04Z

Currently the maximum number of tick is duplicated in two places. This
commit introduces a constant that both can use.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

lib

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use.

jasnell · 2017-02-06T15:10:43Z

lib/internal/process/next_tick.js

@@ -5,6 +5,7 @@ exports.setup = setupNextTick;
 function setupNextTick() {
  const promises = require('internal/process/promises');
  const emitPendingUnhandledRejections = promises.setup(scheduleMicrotasks);
+  const MAX_TICKS = 1e4;


perhaps this should just be a top level const?

If I’m reading the code correctly this should be the maximum number of callbacks that are run in a single event loop iteration – is that correct? Maybe there is a better name than MAX_TICKS, or maybe there should be a comment explaining what this does…

@bnoordhuis or @trevnorris for more specifics on the exact semantics. Whichever it is, a comment is good

@addaleax's interpretation is correct. However, I'm not sure if breaking out in this way runs more nextTicks until the next turn of the event loop or if it runs another loop if more nextTicks remain/are scheduled.

A name like kMaxCallbacksPerTick is arguably more self-documenting - even if 'tick' is a misnomer by now, because...

if breaking out in this way runs more nextTicks until the next turn of the event loop or if it runs another loop if more nextTicks remain/are scheduled

...the remaining ones run when the next call into the VM is made. Anywhere you see a call to MakeCallback() in the C++ code, that's where nexttick callbacks run.

@Fishrock123 that interpretation is incorrect. it guards against the nextTickQueue array becoming too large and running the process out of memory. There's a simple test for this which shows the event loop not being allowed to proceed. This is by design, as the alternative was a bit of a pain to work around.

setImmediate(() => process._rawDebug('hi')); (function runner() { process.nextTick(runner) })();

danbev · 2017-02-07T06:59:25Z

Thanks for the feedback and explanations!

CI: https://ci.nodejs.org/job/node-test-pull-request/6252/

danbev · 2017-02-08T06:41:38Z

@Fishrock123 I don't think you are opposing these changes in your review comment, but wanted to double check if you approve of the changes made?

trevnorris · 2017-02-10T22:55:07Z

kMaxCallbacksPerTick is misleading. The nextTickQueue will always be drained before proceeding (except in the case of an exception). That value is more kMaxCallbacksUntilQueueIsShortened.

trevnorris · 2017-02-10T22:59:26Z

lib/internal/process/next_tick.js

@@ -1,5 +1,7 @@
 'use strict';

+const kMaxCallbacksPerTick = 1e4;


I'm fine with the const, but the name needs to be changed.

Thanks for the feedback. I've changed the name of the const now and also added a comment.

@trevnorris Does the latest change look alright to you? Thanks

Heh. Sorry. I wasn't being literal in the name kMaxCallbacksUntilQueueIsShortened. For an actual name, possibly kMaxCallbacksPerLoop. Though I won't wait until you make the change to sign-off so you won't have to wait for me.

jasnell · 2017-02-15T18:28:43Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/6433/

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use. PR-URL: #11199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

jasnell · 2017-02-17T00:34:09Z

Landed in aa05209

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use. PR-URL: nodejs#11199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris · 2017-02-20T21:24:24Z

@jasnell I had mentioned in a comment that this should have waited until the variable name was changed:

Heh. Sorry. I wasn't being literal in the name kMaxCallbacksUntilQueueIsShortened. For an actual name, possibly kMaxCallbacksPerLoop. Though I won't wait until you make the change to sign-off so you won't have to wait for me.

And I simply signed off early because I'm technically on paternity leave ATM and didn't want to hold up this PR any longer.

JungMinu · 2017-02-21T04:16:40Z

opened a PR to rename kMaxCallbacksUntilQueueIsShortened : #11473

danbev · 2017-02-21T06:34:20Z

Heh. Sorry. I wasn't being literal in the name kMaxCallbacksUntilQueueIsShortened.

Ah sorry I misunderstood. Thanks for clearing that up despite being on parental leave!

danbev · 2017-02-21T06:38:44Z

Thanks for all the feedback on this issue! I appreciate you all taking time to help me better understand this part of the code base.

jasnell · 2017-02-21T07:14:58Z

@trevnorris Sorry about that I had completely overlooked your comment!

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use. PR-URL: #11199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #11473 Ref: #11199 (comment) Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use. PR-URL: #11199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #11473 Ref: #11199 (comment) Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use. PR-URL: #11199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #11473 Ref: #11199 (comment) Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib: introduce constant MAX_TICKS

b77745c

Currently the maximum number of tick is duplicated in two places. This commit introduces a constant that both can use.

jasnell reviewed Feb 6, 2017

View reviewed changes

addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 6, 2017

danbev added 2 commits February 7, 2017 07:48

rename MAX_TICKS -> kMaxCallbacksPerTick

2d266c0

make kMaxCallbacksPerTick a top level const

8788a97

addaleax approved these changes Feb 7, 2017

View reviewed changes

bnoordhuis approved these changes Feb 7, 2017

View reviewed changes

cjihrig approved these changes Feb 7, 2017

View reviewed changes

jasnell approved these changes Feb 7, 2017

View reviewed changes

trevnorris suggested changes Feb 10, 2017

View reviewed changes

rename to kMaxCallbacksUntilQueueIsShortened

eda4d32

trevnorris approved these changes Feb 15, 2017

View reviewed changes

jasnell closed this Feb 17, 2017

danbev deleted the introduce-max-ticks-constant branch February 17, 2017 06:33

JungMinu mentioned this pull request Feb 21, 2017

lib: rename kMaxCallbacksUntilQueueIsShortened #11473

Closed

2 tasks

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: introduce constant MAX_TICKS #11199

lib: introduce constant MAX_TICKS #11199

danbev commented Feb 6, 2017

jasnell Feb 6, 2017

addaleax Feb 6, 2017

jasnell Feb 6, 2017

Fishrock123 Feb 6, 2017 •

edited

Loading

bnoordhuis Feb 6, 2017

trevnorris Feb 10, 2017

danbev commented Feb 7, 2017

danbev commented Feb 8, 2017

trevnorris commented Feb 10, 2017

trevnorris Feb 10, 2017

danbev Feb 12, 2017

danbev Feb 14, 2017

trevnorris Feb 15, 2017

jasnell commented Feb 15, 2017

jasnell commented Feb 17, 2017

trevnorris commented Feb 20, 2017

JungMinu commented Feb 21, 2017 •

edited

Loading

danbev commented Feb 21, 2017

danbev commented Feb 21, 2017

jasnell commented Feb 21, 2017

		@@ -1,5 +1,7 @@
		'use strict';

		const kMaxCallbacksPerTick = 1e4;

lib: introduce constant MAX_TICKS #11199

lib: introduce constant MAX_TICKS #11199

Conversation

danbev commented Feb 6, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Feb 7, 2017

danbev commented Feb 8, 2017

trevnorris commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Feb 15, 2017

jasnell commented Feb 17, 2017

trevnorris commented Feb 20, 2017

JungMinu commented Feb 21, 2017 • edited Loading

danbev commented Feb 21, 2017

danbev commented Feb 21, 2017

jasnell commented Feb 21, 2017

Fishrock123 Feb 6, 2017 •

edited

Loading

JungMinu commented Feb 21, 2017 •

edited

Loading