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

console: prevent internal timers storage from growing #3562

Closed
wants to merge 1 commit into from

Conversation

narqo
Copy link

@narqo narqo commented Oct 28, 2015

The instance of Console class doesn't remove links to arrays that come from hrtimer after timeEnd had been called.

As far as I understand, such links won't be collected by v8's gc.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 28, 2015

Changes LGTM, although a regression test would be nice. This is a behavior change (semver-major) that gets us in line with what Chrome does.

@mscdex mscdex added the console Issues and PRs related to the console subsystem. label Oct 28, 2015
@evanlucas
Copy link
Contributor

Agree with @cjihrig. A test is necessary. Otherwise LGTM though

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2015
@narqo
Copy link
Author

narqo commented Oct 28, 2015

@cjihrig @evanlucas the only test I can imagine is one that checks internal _times map length (size) on timeEnd call (or checks the particular key was removed from the map). Does such checks of module's internal behaviour are suitable?

@evanlucas
Copy link
Contributor

yea I would say just call console.time a few times and then verify the size of this._times and then call console.timeEnd a few times and do another assert

@narqo
Copy link
Author

narqo commented Oct 28, 2015

I have added a test which verifies that timeEnd() calls doesn't leave any new links in the map.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 28, 2015

Should probably use strictEqual() in the test. Other than that, LGTM if the CI is happy.

@@ -57,6 +57,16 @@ console.timeEnd('hasOwnProperty');

global.process.stdout.write = stdout_write;

// verify that console.timeEnd() doesn't leave dead links
var timesMapSize = console._times.size;
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, please make this const too.

@narqo
Copy link
Author

narqo commented Oct 28, 2015

@cjihrig yep!

@bnoordhuis
Copy link
Member

@bnoordhuis
Copy link
Member

@cjihrig @evanlucas Can you explain why this is semver-major?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 28, 2015

Currently, you can reuse a timer, for example after each iteration of a loop. With this change you would have to create a new timer each time.

@narqo
Copy link
Author

narqo commented Oct 28, 2015

@cjihrig do you mean the cases like this:

console.time('label');
console.timeEnd('label');
console.timeEnd('label');
console.timeEnd('label');

@cjihrig
Copy link
Contributor

cjihrig commented Oct 28, 2015

Yes. I'd be willing to bet there are people out there relying on that behavior.

@narqo
Copy link
Author

narqo commented Oct 28, 2015

To be honest I've never thought about such usecase. At least Blink's implementation of console.time() doesn't work this way: undefined is returned after first timeEnd() call.

@Fishrock123
Copy link
Contributor

To be honest I've never thought about such usecase. At least Blink's implementation of console.time() doesn't work this way: undefined is returned after first timeEnd() call.

Note: we don't follow the spec anyways. See: #3473

@cjihrig
Copy link
Contributor

cjihrig commented Nov 9, 2015

@Fishrock123 did we decide to move forward with this change?

@ChALkeR
Copy link
Member

ChALkeR commented Nov 9, 2015

  1. The spec says that the timer should be stopped.
  2. Both Chrome and Firefox stop the timer, and don't output anything on subsequent console.timeEnd calls.
  3. The current implementation actually leaks memory if one creates and stops a significant amount of timers.

+1 for this change, a semver-major though.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 13, 2015

@narqo could you rebase this please.

The instance of `Console` class doesn't remove links to arrays that
come from hrtimer after `timeEnd` had been called.
@narqo
Copy link
Author

narqo commented Nov 13, 2015

🆙

@targos
Copy link
Member

targos commented Nov 13, 2015

LGTM

We may also want to stop throwing on console.timeEnd calls with inexistant labels but that can be done in another PR.

cjihrig pushed a commit that referenced this pull request Nov 13, 2015
Currently, console timers that have been ended with timeEnd()
are not removed. This has the potential to leak memory. This
commit deletes ended timers from the containing Map.

PR-URL: #3562
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Nov 13, 2015

Thanks! Landed in a5cce79

@jasnell jasnell mentioned this pull request Mar 17, 2016
ben-page added a commit to ben-page/node that referenced this pull request Apr 28, 2016
Unintended functionality was removed from console.endTime by
nodejs#3562. Prior to that, you could
call console.endTime multiple times for the same label.
jasnell pushed a commit that referenced this pull request Apr 30, 2016
Unintended functionality was removed from console.endTime by
#3562. Prior to that, you could
call console.endTime multiple times for the same label.

PR-URL: #6454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
Unintended functionality was removed from console.endTime by
#3562. Prior to that, you could
call console.endTime multiple times for the same label.

PR-URL: #6454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
Unintended functionality was removed from console.endTime by
nodejs#3562. Prior to that, you could
call console.endTime multiple times for the same label.

PR-URL: nodejs#6454
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants