Bug 30273

Summary: REGRESSION: Dromaeo DOM test is 14% slower
Product: WebKit Reporter: David Smith <catfish.man>
Component: DOMAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bweinstein, darin, gavin.sharp, jens, levin, mjs, mrowe, oliver, slewis
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.5   
URL: http://dromaeo.com/?id=75494,75493
Attachments:
Description Flags
Make the DOM bindings use StructureFlags none

Description David Smith 2009-10-10 02:24:15 PDT
The URL is comparing the result of running dromaeo.com/?dom on stock Safari 4.0.3 and Safari 4.0.3 + r49412, with the nightly build coming in significantly behind. 2.4GHz/2GB MacBook running 10.6.1.
Comment 1 Maciej Stachowiak 2009-10-10 02:29:28 PDT
The cited link shows a 14% regression.
Comment 2 David Kilzer (:ddkilzer) 2009-10-10 08:27:21 PDT
<rdar://problem/7292968>
Comment 3 Darin Adler 2009-10-12 07:58:04 PDT
Brian Weinstein tells me that some of the slowdown is from the recent changes to lower() although I don't know the details.
Comment 4 Jens Alfke 2009-10-12 09:41:15 PDT
I'd like to see any profiling data, preferably with backtraces, that implicates lower(). There's certainly potential for the optimization I made to backfire if there are huge numbers of non-no-op calls to that method; I just wasn't seeing many such calls when I was profiling.
Comment 5 Darin Adler 2009-10-12 10:00:14 PDT
Jens, our primary technique is actually narrowing down which revision caused the performance change. We typically don't start profiling until we have pinpointed the revision number.
Comment 6 Jens Alfke 2009-10-12 11:57:46 PDT
OK. Just now I ran the core DOM tests on r49402 and r49403 (just before and after my StringImpl::lower change), and found a 1.5% speedup.

I also did some profiling and saw StringImpl::isLower showing up in some of the tests; the patch I'm working on right now (bug 30261) eliminates those tests, which will help slightly.
Comment 7 Darin Adler 2009-10-12 11:59:36 PDT
Brian, let us know when you've pinpointed which revisions caused the slowdown.

Here's hoping Jens's changes are not to blame. I suspect they may even provide a speedup!
Comment 8 Brian Weinstein 2009-10-12 12:03:11 PDT
I had a 10.8% regression between 4.0.3 and revision 49414 on my machine. I was able to track down an 8-9% regression between revisions 48316 and 48351, and I will comment again when I have it down to the exact revision/revision + build fixes around it.
Comment 9 Brian Weinstein 2009-10-12 13:02:20 PDT
Here are my results after testing on Mac:

4.0.3: 670.73 runs/second

r48325: 671.44 runs/second
r48330: 663.40 runs/second
r48334: 613.25 runs/second

Tip of Tree (r49414): 598.18 runs/second

It looks like there is a slight regression between r48325 and r48330, and a big regression between 48330 and 48334. 

However, the mac doesn't have any builds between these two revisions, so I will move over to Windows to see if I can reproduce the regressions there, and get an exact revision number.
Comment 10 Darin Adler 2009-10-12 13:06:46 PDT
(In reply to comment #9)
> a big regression between 48330 and 48334. 

That points to <http://trac.webkit.org/changeset/48331> then.
Comment 11 Brian Weinstein 2009-10-12 13:10:02 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > a big regression between 48330 and 48334. 
> 
> That points to <http://trac.webkit.org/changeset/48331> then.

That was my guess, but I'm going to verify on Windows to double check.
Comment 12 Brian Weinstein 2009-10-12 13:34:26 PDT
I confirmed that this is also a regression on Windows, and that it is from http://trac.webkit.org/changeset/48331, assigning to Oliver.
Comment 13 Darin Adler 2009-10-12 13:46:00 PDT
Brian: So if that’s 8% of the regression, we still have a significant regression to track down. Over 5%, right?
Comment 14 Brian Weinstein 2009-10-12 13:48:10 PDT
I never saw 14%, so in my tests, there's about 2-3% of the regression that still needs to be spoken for, but I'll take a look at that later today.
Comment 15 Stephanie Lewis 2009-10-15 17:28:29 PDT
Tracked dow the rest of this.  There was a 4.5% regression from <http://trac.webkit.org/changeset/49409>
Comment 16 Oliver Hunt 2009-10-19 20:55:35 PDT
Created attachment 41477 [details]
Make the DOM bindings use StructureFlags
Comment 17 Oliver Hunt 2009-10-19 21:06:03 PDT
Am now adding logging to see what cases we're hitting where we believe things aren't cachable
Comment 18 Oliver Hunt 2009-10-19 21:06:43 PDT
Comment on attachment 41477 [details]
Make the DOM bindings use StructureFlags

Landed in r49835
Comment 19 Oliver Hunt 2009-10-19 23:58:27 PDT
Accidentally removed important part of that patch, with it actually present the regression caused by r48331 goes away.

Fixed in r49841.
Comment 20 Darin Adler 2009-10-20 10:43:18 PDT
Why is this marked fixed if there was a 4.5% regression from <http://trac.webkit.org/changeset/49409>? Was that fixed?
Comment 21 Mark Rowe (bdash) 2009-10-20 11:17:27 PDT
(In reply to comment #20)
> Why is this marked fixed if there was a 4.5% regression from
> <http://trac.webkit.org/changeset/49409>? Was that fixed?

That was almost certainly due to Geoff disabling the JIT in that revision (<http://trac.webkit.org/changeset/49409/trunk/JavaScriptCore/wtf/Platform.h>).It was reenabled shortly after.