Closed
Bug 533148
Opened 15 years ago
Closed 15 years ago
OOM trace-test takes minutes to complete
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: bzbarsky)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•15 years ago
|
||
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?
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
Oh, and MAX_LENGTH did change: from 0x0FFFFFFF to 0x10000000
Reporter | ||
Comment 4•15 years ago
|
||
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•15 years ago
|
||
This test takes forever for me to run now too.
Comment 6•15 years ago
|
||
bz: any issues you can see in going back one to the old MAX_LENGTH?
/be
Assignee | ||
Comment 7•15 years ago
|
||
Not at all, apart from this all-hands eating all my time issue. ;)
Comment 8•15 years ago
|
||
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
Attachment #416668 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 9•15 years ago
|
||
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! ;)
Whiteboard: fixed-in-tracemonkey
Comment 10•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•