803182 - Figure out why we have sporadic too-much-recursion failures on js1_8/genexps/regress-380237-01.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

We should figure out exactly what is going wrong so we can decide what to do about it.

Comment 1

[Terrence Cole [:terrence]]

Attachment: test

Approximate stack size used by js::Interpret in bytes:
opt-gcc-x86: 36
dbg-gcc-x86: 51
opt-gcc-x64: 3
dbg-gcc-x64: 80

opt-clang-x86: 262
dbg-clang-x86: 1051
opt-clang-x64: 162
dbg-clang-x64: 1319

Well, it's pretty easy to see /what/ is happening.  Now to try to figure out why.

Comment 2

[Terrence Cole [:terrence]]

Correction, Bill just pointed out to me that I wrote bytes as the type: these are actually # of ints.

Comment 3

[Terrence Cole [:terrence]]

And it looks like there is re-ordering happening too: the stack extent for the "3" above is actually:
  sub    $0x6c8, %rsp

I'm going to go dig into these binaries and find out what the real stack sizes are.

Comment 4

[Terrence Cole [:terrence]]

These numbers are taken directly from the sub instruction that sets up the stack in js::Interpret.  I have transcribed them all into decimal on the right of the bar to make the SI magnitude obvious too.

opt-gcc-x86: 0x52c    | 1324
dbg-gcc-x86: 0xaec    | 2796
opt-gcc-x64: 0x6c8    | 1736
dbg-gcc-x64: 0x1910   | 6416

opt-clang-x86: 0x10bc | 4284
dbg-clang-x86: 0x1c7c | 7292
opt-clang-x64: 0x638  | 1592
dbg-clang-x64: 0x2240 | 8768

Comment 5

[Brendan Eich [:brendan]]

See also bug 794427.

/be

Comment 6

[Terrence Cole [:terrence]]

Attachment: v0: fix

The shell sets a 5e5 stack limit for all builds.  The x86 browser sets the same (2^19) limit, but scaled by size_t so that it automatically doubles to a full 1 MiB on 64bit platforms.  This explains why we are only ever seeing this problem in the shell and not on TBPL.

Thus, I think this is just a bug in the shell: we should have the same stack limit as the browser so that we can know that any too-much-recursion errors we see represent real problems.  Moreover, we should scale the limit by the 4x swing we see between opt and debug builds for both the browser and shell.

Try run up at: https://tbpl.mozilla.org/?tree=Try&rev=38bd7a313e8a

Comment 7

[Brendan Eich [:brendan]]

Terrence: agree there's a shell stack limit configury bug, but also:

Firefox 3-era js shell:

...
420
430
typein:8: InternalError: too much recursion


TraceMonkey repo just before it was retired js shell:

...
290
300
typein:6:0: InternalError: too much recursion


My trunk m-c js shell:

...
100
110
typein:8:0 InternalError: too much recursion 

We have lost by ~300 frames or so on Fedor's benchmark from bug 794427 since Firefox 3 era.

Idea: make this bug about shell configury and bug 794427 about fixing the interpreter? Or vice versa, since this bug has more clang-implicating data.

/be

Comment 8

[Terrence Cole [:terrence]]

(In reply to Brendan Eich [:brendan] from comment #7)
> Terrence: agree there's a shell stack limit configury bug, but also:
> 
snip
> 
> We have lost by ~300 frames or so on Fedor's benchmark from bug 794427 since
> Firefox 3 era.

I'd expect us to be losing an ever growing pile of stack to the extra assertions we are continually adding to debug builds.  I'm guessing you aren't in a debug shell, however, which is indeed troublesome.
 
> Idea: make this bug about shell configury and bug 794427 about fixing the
> interpreter? Or vice versa, since this bug has more clang-implicating data.

I'm fine with either.

Comment 9

[Brendan Eich [:brendan]]

Those were all opt shells, yeah.

Totally your call on which bug is for fixing what problem. I'm thinking this one's summary is best for the easy shell fix.

/be

Comment 10

[David Mandelin [:dmandelin]]

Comment on attachment 672945 [details] [diff] [review]
v0: fix

Review of attachment 672945 [details] [diff] [review]:
-----------------------------------------------------------------

An email just nagged me about this review, which is interesting. Anyway, I believe I was waiting because there's an extra fprintf and I mentioned it to you in person. Let's call it r+ with the debugging changes in jsinterp.cpp removed.

Comment 11

[Terrence Cole [:terrence]]

Ah, I'm sorry, I assumed you were just going to tell me to remove it in the review :-).  I certainly didn't mean to include it, and it looks like the try run was totally busted because of it.

Comment 12

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=f1e71d2090cf

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd844b7152e

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9dd844b7152e