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

Intermediate values with console.time #84

Closed
mattzeunert opened this issue Jan 24, 2017 · 18 comments
Closed

Intermediate values with console.time #84

mattzeunert opened this issue Jan 24, 2017 · 18 comments
Assignees
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@mattzeunert
Copy link

There should be a way to print how much time has passed since a timer has started without removing the timer.

In Chrome calling timeEnd currently doesn't remove the timer, so you can call timeEnd multiple times.

console.time("abc")
console.timeEnd("abc")
console.timeEnd("abc")
// abc: 0.006ms
// abc: 0.281ms

The timer is reset only when time is called again with the same label. Firefox and Node remove the timer when timeEnd is called, as the spec says.

To be able to see intermediate values the spec could either be changed to recommend Chrome's behavior, or a new timeLog (or similar) function could be introduced, which prints the duration without affecting the timer itself.

console.time("abc")
console.timeLog("abc")
console.timeEnd("abc")
@JosephPecoraro
Copy link

I think this is a great idea. It would be easy to implement this in WebKit. I'd be happy to do it.

@terinjokes terinjokes added the addition/proposal New features or enhancements label Jan 27, 2017
@domenic
Copy link
Member

domenic commented Jan 27, 2017

I think this is a great idea. It would be easy to implement this in WebKit. I'd be happy to do it.

The new timeLog function, or the timeEnd-can-be-called-multiple-times behavior?

/cc @paulirish @auchenberg

@JosephPecoraro
Copy link

JosephPecoraro commented Jan 28, 2017

Allowing timeEnd to be called multiple times. It is pretty similar to performance.measure as well.

@targos
Copy link

targos commented Jan 28, 2017

Anyone knows when (and why) did Chrome's behavior change? According to nodejs/node#3562, it used to remove the timer after a call to timeEnd.

@JosephPecoraro
Copy link

It looks to me like this change / (review). Not sure if that was intention or not as it doesn't appear to be mentioned in the commit message, but code inspection looks like it might have done it (see difference between ConsoleBase::timeEnd and timeEndFunction).

@targos
Copy link

targos commented Jan 28, 2017

/cc @ak239

@domfarolino domfarolino added the needs implementer interest Moving the issue forward requires implementers to express interest label Apr 29, 2018
@domfarolino
Copy link
Member

domfarolino commented Apr 29, 2018

As of at least now, Chrome's behavior is back to removing the timer from the associated timer table. In other words, timeEnd cannot be called more than once (effectively) in any implementations. With this, would it be more sensible to:

  • Ask all implementations to change their behavior of timeEnd
  • Add a timeLog function

I'd be happy to implement either in Chrome, but I'd like to here from @JosephPecoraro (who likely still prefers multiple calls to timeEnd) and @paulirish, @bgrins, and @xirzec

@xirzec
Copy link

xirzec commented Apr 30, 2018

I prefer adding a new timeLog function, since then it can be more easily feature detected.

@bgrins
Copy link
Contributor

bgrins commented May 1, 2018

cc: @nchevobbe @bakulf

I also prefer a new function because:

  1. the naming of timeEnd implies it the timer is going away
  2. as @xirzec mentions, devs could detect if an implementation supports it and do something else if not

@bakulf
Copy link

bakulf commented May 2, 2018

Makes sense to me and it seems we have consensus here.
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1458466 to implement it in firefox.

@mattzeunert
Copy link
Author

I'm wondering, perhaps it might be helpful to have an extra label as well that gets printed on the same line:

console.time("Load Page")
console.timeLog("Load Page", "JSON loaded")
console.timeLog("Load Page", "App rendered")
console.timeEnd("Load Page")

@nchevobbe
Copy link

I'm wondering, perhaps it might be helpful to have an extra label as well that gets printed on the same line

I think that would make sense. I used to do things like :

console.group("Load Page");
console.time("load json");
console.timeEnd("load json");
console.time("app rendered");
console.timeEnd("app rendered");
console.groupEnd("Load Page");

so with timeLog + an additional label that could be really nice.
The secondary label could be rendered with a grey-ish color so the emphasis would still be on the primary one.

@domfarolino
Copy link
Member

The extra label seems pretty helpful with this intermediate timeLog idea. Seems like we at least have consensus (or just about all the way there) on timeLog. Seems like @bakulf has started a FF implementation, are you comfortable updating it to support multiple labels? Would like to double-check with @xirzec as well as @JosephPecoraro and then I'll start speccing it out!

@domfarolino domfarolino self-assigned this May 4, 2018
@bakulf
Copy link

bakulf commented May 4, 2018

Instead of having an extra DOMString parameter, I think it would be better to have:
void timeLog(optional DOMString label = "default", optional any... data);
This makes timeLog similar to assert(), log() and so on.

@nchevobbe
Copy link

For the record, this landed in Firefox 62.0a1 (2018-05-22) (See Bug 1461001)

@domfarolino
Copy link
Member

@nchevobbe Great to hear, thanks for the update! I'll get a spec PR going.

@domfarolino
Copy link
Member

I've posted a PR for this, would welcome anyone taking a look and providing feedback.

@domfarolino
Copy link
Member

If anyone (@nchevobbe perhaps since FF has implemented) wants to check out the spec PR #138 for this that'd be great! We should be able to land this relatively soon, just want to get some eyes on the PR before I write some tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

10 participants