Adding basic long stack traces in V8 when `end`ing. #66

Merged
merged 2 commits into from May 14, 2012

Conversation

Projects
None yet
3 participants
@domenic
Collaborator

domenic commented May 8, 2012

This needs review in a few ways, but just might be good enough to merge as a starting point. Notably there should be very little performance impact since Error.captureStackTrace gives us a lazily-evaluated getter and we defer parsing/filtering/assembling until .end() is called.

Points of concern:

  • Copied a bunch of Google code from tlrobinson/long-stack-traces. Not sure how we want to handle that, e.g. where the copyright notice/disclaimer goes, whether I should have left the style as-is or integrated it as I did, how to handle the insertion I made (<inserted by @domenic>), etc. Please advise.
  • Promises are now carrying around a promise.stack getter. This might have security implications. It seemed like the only way to do this, without a weak map/private names. And it's rather convenient.

Going forward:

  • We should record some or all messages sent to a promise, most importantly thens. See discussion in #57 for an example scenario where more stuff could be caught. This is quite possible; I managed to log some of the stack frames we'd want to the console. The only tricky part is carrying them around until they're desired, especially while preserving the performant laziness advantage.
  • We should update rejection reason stacks (when the rejection reasons are error instances, as they should be) to be in this format as well.
+ var oldPrepareStackTrace = Error.prepareStackTrace;
+
+ Error.prepareStackTrace = function (error, frames) {
+ // Filter out frames from the innards of Node and Q.

This comment has been minimized.

@kriskowal

kriskowal May 8, 2012

Owner

I am curious whether we could make this less anti-social. Consider folks who have provided their own overrides, or folks debugging Node core. Good enough for now though!

@kriskowal

kriskowal May 8, 2012

Owner

I am curious whether we could make this less anti-social. Consider folks who have provided their own overrides, or folks debugging Node core. Good enough for now though!

This comment has been minimized.

@domenic

domenic May 8, 2012

Collaborator

I'm not sure I understand 100% of the concerns, but perhaps we could blacklist only specific function name + filename combinations.

@domenic

domenic May 8, 2012

Collaborator

I'm not sure I understand 100% of the concerns, but perhaps we could blacklist only specific function name + filename combinations.

This comment has been minimized.

@domenic

domenic May 8, 2012

Collaborator

I should try this in a browser too. I bet I want to filter out "native code" or something like that.

@domenic

domenic May 8, 2012

Collaborator

I should try this in a browser too. I bet I want to filter out "native code" or something like that.

@kriskowal

This comment has been minimized.

Show comment
Hide comment
@kriskowal

kriskowal May 8, 2012

Owner

Looking forward to trying this out!

Owner

kriskowal commented May 8, 2012

Looking forward to trying this out!

Annoyingly-complicated code to get the q.js filename in Chrome.
The regex being used failed for file:///C:/whatever/q.js:1:2.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 8, 2012

Collaborator

Works in Chrome now although that code is kind of horrible. Notably Chrome shows the short stack trace when you click the little arrow in the console, even though if you break on error and do error.stack in the REPL, it will give the long stack trace. Investigating.

Collaborator

domenic commented May 8, 2012

Works in Chrome now although that code is kind of horrible. Notably Chrome shows the short stack trace when you click the little arrow in the console, even though if you break on error and do error.stack in the REPL, it will give the long stack trace. Investigating.

@kriskowal kriskowal merged commit 8f2bc5e into master May 14, 2012

@sp

This comment has been minimized.

Show comment
Hide comment
@sp

sp Oct 3, 2012

This has the effect of introducing a colossal memory leak for me. I have a situation where I call Q.ncall() twice per line while processing files of ~30k lines and the memory grows as (n*20kb), it's just totally killing me. A 30k line job hits >700Mb; with this line commented out it doesn't break 80Mb. Giving the gc time to catch up doesn't help.

sp commented on q.js in 2ccccb7 Oct 3, 2012

This has the effect of introducing a colossal memory leak for me. I have a situation where I call Q.ncall() twice per line while processing files of ~30k lines and the memory grows as (n*20kb), it's just totally killing me. A 30k line job hits >700Mb; with this line commented out it doesn't break 80Mb. Giving the gc time to catch up doesn't help.

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Oct 4, 2012

Collaborator

Ouch, yeah, see #111 for a similar problem. Will try to prioritize putting in some kind of limit. Thanks for letting us know.

Collaborator

domenic replied Oct 4, 2012

Ouch, yeah, see #111 for a similar problem. Will try to prioritize putting in some kind of limit. Thanks for letting us know.

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