Closed Bug 663547 Opened 9 years ago Closed 9 years ago

Something made ecma_5/Object/* much slower in the past couple weeks


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)


(Keywords: regression, Whiteboard: fixed-in-tracemonkey)


(1 file)

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 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").
Attached patch PatchSplinter Review
Er, right.  I fail at terminology.  :-)
Attachment #539391 - Flags: review?(luke)
Comment on attachment 539391 [details] [diff] [review]

r+ putting init in constructor.
Attachment #539391 - Flags: review?(luke) → review+
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla7
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.