533148 - OOM trace-test takes minutes to complete

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

trace-test/tests/basic/testBug507425.js went from taking ~2s to 2 minutes on my Ubuntu laptop with 1GB RAM starting with the cset http://hg.mozilla.org/tracemonkey/rev/c27f496d0d7a from bug 521423.  The test grows a string until OOM.  I see that the value of JSString::MAX_LENGTH was not changed, but perhaps something else was changed that caused OOMs to happen much later?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Looking at the code this should run, I wouldn't have thought it would be affected by those changes.

Does which iteration of that loop we throw on change across that changeset?  If so, from what to what?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So over here I do see it change from i == 27 to i == 28 on that changeset (Mac laptop, 4GB of RAM).

But it completes quite fast, at least in shell.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and MAX_LENGTH did change: from 0x0FFFFFFF to 0x10000000

Comment 4

[Luke Wagner [:luke]]

Heh, it would appear that the one-greater MAX_LENGTH is the reason; before the test would OOM while still in RAM and now it has to do ~1GB pagefile i/o before OOMing.

I'm guessing the same issue caused a random failure on a similar OOM test in bug 523793 comment 24.  This may be a problem with OOM tests in the tree that use strings with power-of-2 sizes and now get an extra iteration that is bad for old laptops and crowded VMs.

So: (1) testers' fault for using puny hw, (2) tests' faults for walking the line, (3) maybe MAX_LENGTH = (1 << 28) - 1 was better?

Comment 5

[David Mandelin [:dmandelin]]

This test takes forever for me to run now too.

Comment 6

[Brendan Eich [:brendan]]

bz: any issues you can see in going back one to the old MAX_LENGTH?

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Not at all, apart from this all-hands eating all my time issue.  ;)

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 416668 [details] [diff] [review]
Fix

>+    /* Generous but sane length bound; the "-1" is there for backwards-compat. */
>+    static const size_t MAX_LENGTH = ((1 << 28) - 1);

Nits: no need to overparenthesize the whole initializer; instead of "backwards-compat" how about "compatibility with OOM tests" or some such (major comment style as needed).

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/tracemonkey/rev/20e1a6297a97 with the nits.  In my defence on the parens, I missed this being a static and not a macro!  ;)

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/20e1a6297a97