Closed Bug 663547 Opened 9 years ago Closed 9 years ago
Something made ecma
_5/Object/* much slower in the past couple weeks
I regularly run the entire shell test suite to check my patches for correctness. Up until about two weeks ago, it was taking 450s or so for the entire run, debug shell, -jmp. Just recently, it started taking 900s. Just running the ecma_5/Object/ directory now takes 709.5s, which is definitely far longer than it once took. Something regressed that. We need to figure out what regressed it and why, because something got massively worse and it might well affect more code than just that.
This could be my fault. The gc zeal patch (bug 650978) had problems with timeouts. I thought I fixed this, but I'll try timing the whole run before and after the patch landed.
Looks like it wasn't my patch. The time to run ecma_5/Object/ before and after the zeal patch was 94 seconds. I also tried tip, and it does indeed take much longer. So this happened in the last 10 days.
I have a very lurky suspicion that maybe something in the stack refactoring triggers a quadratic case, or something like that, and caused this. But I have absolutely no proof whatsoever of it. And I reviewed that patch, and I didn't recall seeing anything where it would have changed any existing code's algorithmic complexity.</utterly-unfounded-speculation>
I think you're right. The regressing changeset is bb9e5496b0ac, which is from bug 656462. Luke, could you take a look at this?
Ah, that cset changed FrameRegsIter to (in debug-mode only) call js_ReconstructStackDepth on every frame to assert that the sp offset matched the one computed by the iterator. FrameRegsIter is used to iterate over frames when each exception is thrown which, for 18.104.22.168-redefinition1-of-4.js is apparently all the time. Running under gdb and breaking at random places confirms this. I guess we could turn off this assert; but that's kinda sad, its a really strong assert. Jeff, any way we could cut down the number of exceptions thrown in these tests?
Not really. The cases where exceptions are thrown are the core complexity of Object.defineProperty. I *did* already cut out all the cases where the exception's trivial (because the descriptor object passed in is self-contradicting). But I can't cut out others, at least not in any clear way that still preserves all-paths, all-transitions coverage in the algorithm. And I don't want to give that up, because the algorithm's simply too complex to test it otherwise. There has to be a way to preserve this, yeah. Maybe have it conditioned on a debug member of something, that those tests can then flip off with a shell helper?
(In reply to comment #6) > There has to be a way to preserve this, yeah. Maybe have it conditioned on > a debug member of something, that those tests can then flip off with a shell > helper? Haha, devious. So, like, a context flag that gets set through a shell-only "dontAssertStackDepth()" ?
Yeah, but definitely not a context flag. It'd be pure hack, definitely not exposed via jsapi.h and ideally not even exposed by an installed header. I think the shell can access even pure-internal headers that aren't installed, so putting it somewhere in such a file should work.
(In reply to comment #8) That's what I meant; just an internal-only flag stashed in JSContext, nothing in the JSAPI (which, btw, calls them "options" not "context flags").
Er, right. I fail at terminology. :-)
Attachment #539391 - Flags: review?(luke)
Comment on attachment 539391 [details] [diff] [review] Patch r+ putting init in constructor.
Attachment #539391 - Flags: review?(luke) → review+
Target Milestone: --- → mozilla7
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/8e0305959163
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.