Last Comment Bug 663547 - Something made ecma_5/Object/* much slower in the past couple weeks
: Something made ecma_5/Object/* much slower in the past couple weeks
Status: RESOLVED FIXED
fixed-in-tracemonkey
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-10 15:13 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-06-20 17:17 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.26 KB, patch)
2011-06-14 18:23 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-10 15:13:54 PDT
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.
Comment 1 Bill McCloskey (:billm) 2011-06-10 15:16:54 PDT
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.
Comment 2 Bill McCloskey (:billm) 2011-06-10 15:47:40 PDT
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.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-10 16:54:46 PDT
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>
Comment 4 Bill McCloskey (:billm) 2011-06-13 10:12:56 PDT
I think you're right. The regressing changeset is bb9e5496b0ac, which is from bug 656462. Luke, could you take a look at this?
Comment 5 Luke Wagner [:luke] 2011-06-13 10:43:48 PDT
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 15.2.3.6-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?
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-13 11:51:15 PDT
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?
Comment 7 Luke Wagner [:luke] 2011-06-13 11:53:42 PDT
(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()" ?
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-13 12:24:31 PDT
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.
Comment 9 Luke Wagner [:luke] 2011-06-13 13:31:39 PDT
(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").
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-14 18:23:37 PDT
Created attachment 539391 [details] [diff] [review]
Patch

Er, right.  I fail at terminology.  :-)
Comment 11 Luke Wagner [:luke] 2011-06-14 18:27:11 PDT
Comment on attachment 539391 [details] [diff] [review]
Patch

r+ putting init in constructor.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-16 11:49:32 PDT
http://hg.mozilla.org/tracemonkey/rev/8e0305959163
Comment 13 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:17:05 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/8e0305959163

Note You need to log in before you can comment on or make changes to this bug.