538293 - remove inlineCallCount

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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

[Brendan Eich [:brendan]]

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

[Igor Bukanov]

(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.

Comment 3

[Luke Wagner [:luke]]

(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

[Igor Bukanov]

(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

Comment 5

[Luke Wagner [:luke]]

Ah, thanks.  It looks like the JS_MAX_INLINE_CALL_COUNT test can't go then.

Comment 6

[Luke Wagner [:luke]]

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.

Comment 7

[Luke Wagner [:luke]]

Attachment: kill inlineCallCount and the stackLimit quota hack

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.

Comment 8

[Luke Wagner [:luke]]

Attachment: kill inlineCallCount and the stackLimit quota hack

Remove 'next' default parameter of StackFrame::pcQuadratic (all uses replaced by FrameRegsIter).

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 11

[Luke Wagner [:luke]]

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.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/abd2dcd555f4
http://hg.mozilla.org/mozilla-central/rev/c72fed47c034