Closed Bug 968936 Opened 6 years ago Closed 6 years ago

when the operation callback stops script execution, emit a warning message with a stack trace

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: luke, Assigned: jorendorff)

Details

Attachments

(1 file)

This came up while working with some partners debugging iloops in generated code: they have an iloop but, of course, this hangs the browser so they can't just "break" and see where they are.  Instead, they resort to using the profiler :)  Since debugging iloops is not uncommon, we were thinking it would be nice to print a warning message when the operation callback aborts script execution that contained the stack trace.
Is this related to bug 717749? Ideally the slow script dialog would have a "debug" button.
... that sounds even better!
Well, on second thought, it might still be good to have this feature since the debugger won't always be installed (since it significantly impacts perf).  I know you are working to let the debugger not disable ion; that'd definitely help, but would there still be significant perf impact (from hooks called by ion)?
(In reply to Luke Wagner [:luke] from comment #3)
> Well, on second thought, it might still be good to have this feature since
> the debugger won't always be installed (since it significantly impacts
> perf).  I know you are working to let the debugger not disable ion; that'd
> definitely help, but would there still be significant perf impact (from
> hooks called by ion)?

I'm working to have a different "mode" of the Debugger that doesn't disable Ion when you want to debug perf issues (such as deoptimizations). For debugging semantic issues, I think the cost of adding hooks to optimized jitcode is worth it, so for the kind of debugging you want to do for an iloop, Ion will probably still be disabled.

tl;dr the point of your second thought stands.
I forgot the operative word: "the cost of adding hooks to optimized jitcode is worth it" -> "the cost of adding hooks to optimized jitcode is *not* worth it"
If by "debug" button you mean that it would effectively call "debugger;" at the location where the current execution was at the point the dialog popped up? - I think for Emscripten purposes, that's perhaps not so good, since for larger Emscripten codebases (imagine 10M+ lines/300MB JS), the debugger is useless, we cannot use it even for viewing the source .js files, so callstacks even less. I would prefer the mechanism where the callstack was printed to console (plus .js file:line:column info) for easy examination. Of course for non-Emscripten stuff, the "debug" button for popping up the debugger would kick ass.
That's a real problem with the way we handle sources. It really oughtn't die, even when the sources are huge.

We know there's going to be some big work needed to handle dynamically introduced sources (eval, etc.); perhaps we'll be able to do something to manage gigantic sources while we're at it. We'll have to see.
(In reply to Jim Blandy :jimb from comment #7)
OOC, is the issue some O(n) algorithm (I can guess a few usual suspects :) where n is script size?
There might well be issues with O(n) algorithms --- source note processing, say. But the big issue with debugging IonMonkey code is the size of the source code itself. We actually just send the whole darn string over the protocol, drop it into an editor, and display it. So the "document" of the debugger's UI would be gigantic.
The original thing proposed in comment 0 is a good idea. I have a patch.
Assignee: nobody → jorendorff
js> timeout(1); for(;;);
  typein:1:19 warning: Script terminated by timeout at:
  typein:1:19 warning: @typein:1:20
  typein:1:19 warning: 

r?luke rather optimistically. :)  I'm not sure this is really what's wanted.
Attachment #8379974 - Flags: review?(luke)
Comment on attachment 8379974 [details] [diff] [review]
bug-968936-timeout-warning-v1.patch

I think this is exactly what's needed.  Thanks!
Attachment #8379974 - Flags: review?(luke) → review+
Think this is ready to land?  There are a few people excited to use this.
It landed this morning, I just forgot to note it here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c90e26bbcdf5
https://hg.mozilla.org/mozilla-central/rev/c90e26bbcdf5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.