Closed
Bug 538293
Opened 15 years ago
Closed 14 years ago
remove inlineCallCount
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
93.12 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
It seems like inlineCallCount is subsumed by scriptStackQuota in its role of preventing DoS by recursion. inlineCallCount is also used in a few places to check "are there any more inline frames?", but perhaps these uses can be replaced by |cx->fp->down == NULL| (what about js_Execute with down != NULL?).
Comment 1•15 years ago
|
||
fp->down links across js_Interpret activations, so while null down does mean no more inline (or "out of line") frames, non-null does not tell you anything about inlineCallCount != 0.
I should have remembered the obvious non-DOS-prevention aspect here, of routing control flow out of js_Interpret when inlineCallCount decrements to 0 on return from JSOP_STOP and JSOP_RETURN/JSOP_RETRVAL (latter two if js_Invoke is calling js_Interpret). These need something tantamount to inlineCallCount, but a frame flag would suffice, maybe.
/be
Comment 2•15 years ago
|
||
(In reply to comment #0)
> It seems like inlineCallCount is subsumed by scriptStackQuota in its role of
> preventing DoS by recursion.
At some point js_Interpret has relied only on scriptStackQuota for infinite recursion detection. But that have not worked well. The problem is that it takes some time before a bogus recursion exhausts scriptStackQuota leading to usability problems for web developers. I.e. inlineCallCount allows to detect much faster a run-away recursion.
![]() |
Assignee | |
Comment 3•15 years ago
|
||
(In reply to comment #2)
> The problem is that it
> takes some time before a bogus recursion exhausts scriptStackQuota leading to
> usability problems for web developers. I.e. inlineCallCount allows to detect
> much faster a run-away recursion.
Just so I can understand: is the issue when the recursive function does a lot of computation before recursing? Like "function f() { expensive(); f() }". Because a simple "function f() { f() }" seems to error out instantly with/without the inlineCallCount check.
Comment 4•15 years ago
|
||
(In reply to comment #3)
> Just so I can understand: is the issue when the recursive function does a lot
> of computation before recursing?
See bug 424683 for details
![]() |
Assignee | |
Comment 5•15 years ago
|
||
Ah, thanks. It looks like the JS_MAX_INLINE_CALL_COUNT test can't go then.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
![]() |
Assignee | |
Comment 6•14 years ago
|
||
Fast forward in time and now there is a max stack depth (4MB) and much faster calls (even from the interpreter). With the patch I'll attach shortly, we can hit the end of the stack and throw in 28ms for an interp-only opt build and 17ms with the methodjit. Even a debug interp-only shell only takes 138ms.
To make this an even more obvious choice: measuring the max recursion depth before an exception is thrown, while we bottom out at 3000-8000, all other engines (ie9, jsc, v8, opera) go to at least 20,000. With the patch, we bottom out at 58,000 and win the who-goes-deepest recursion game.
This was motivated by bug 653785 and the never ending sadness that is the methodjit stackLimit heuristic.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
![]() |
Assignee | |
Comment 7•14 years ago
|
||
Two notes:
1) currently, fp->pc(cx) is O(n) where n is the distance from fp to the top of the top of the stack. In a few cold paths (error handling), we iterate down the stack and call fp->pc, which is quadratic. Increasing the max n from 8000 to 50000 is quite noticeable (milliseconds turn to seconds), so I switched these sites to use FrameRegsIter, changed the name of 'pc' to 'pcQuadratic', and audited where we use it.
2) some of the jit-tests intend to do infinite recursion and do something slow on each step. to keep jit-tests fast, I made their frames fatter (via apply).
Also, I couldn't help myself with the Helter Skelter reference on the pcQuadratic comment. Let me know if its too weird.
![]() |
Assignee | |
Comment 8•14 years ago
|
||
Remove 'next' default parameter of StackFrame::pcQuadratic (all uses replaced by FrameRegsIter).
Attachment #535801 -
Attachment is obsolete: true
Attachment #536775 -
Flags: review?(dvander)
Attachment #535801 -
Flags: review?(dvander)
Comment on attachment 536775 [details] [diff] [review]
kill inlineCallCount and the stackLimit quota hack
Review of attachment 536775 [details] [diff] [review]:
-----------------------------------------------------------------
Wow it's good to see that go away.
Attachment #536775 -
Flags: review?(dvander) → review+
![]() |
Assignee | |
Comment 11•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/abd2dcd555f4
and http://hg.mozilla.org/tracemonkey/rev/c72fed47c034 to dial down the recursion in test-apply-many-args.js.
Whiteboard: fixed-in-tracemonkey
Comment 13•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/abd2dcd555f4
http://hg.mozilla.org/mozilla-central/rev/c72fed47c034
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•