Bug 157952 - We should have JSBench in PerformanceTests
Summary: We should have JSBench in PerformanceTests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-20 12:25 PDT by Keith Miller
Modified: 2016-05-25 02:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (deleted)
2016-05-20 12:40 PDT, Keith Miller
saam: review+
keith_miller: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-05-20 12:25:24 PDT
We should have JSBench in PerformanceTests
Comment 1 Keith Miller 2016-05-20 12:40:43 PDT
Created attachment 279496 [details]
Patch
Comment 2 WebKit Commit Bot 2016-05-20 14:58:38 PDT
Attachment 279496 [details] did not pass style-queue:


ERROR: PerformanceTests/JSBench/harness.py:5:  trailing whitespace  [pep8/W291] [5]
ERROR: PerformanceTests/JSBench/harness.py:8:  trailing whitespace  [pep8/W291] [5]
ERROR: PerformanceTests/JSBench/harness.py:14:  trailing whitespace  [pep8/W291] [5]
ERROR: PerformanceTests/JSBench/harness.py:40:  whitespace before '}'  [pep8/E202] [5]
ERROR: PerformanceTests/JSBench/harness.py:91:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: PerformanceTests/JSBench/harness.py:124:  whitespace before '}'  [pep8/E202] [5]
ERROR: PerformanceTests/JSBench/harness.py:144:  whitespace before ')'  [pep8/E202] [5]
ERROR: PerformanceTests/JSBench/harness.py:182:  trailing whitespace  [pep8/W291] [5]
ERROR: PerformanceTests/JSBench/harness.py:202:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 9 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ryosuke Niwa 2016-05-20 15:13:45 PDT
Wait, we already have this. Just run:
run-benchmark --plan jsbench --browser safari --platform osx
Comment 4 Keith Miller 2016-05-20 15:23:15 PDT
(In reply to comment #3)
> Wait, we already have this. Just run:
> run-benchmark --plan jsbench --browser safari --platform OS X

This for run-jsc-benchmarks, which AB tests two different builds for regressions directly. It's easier to have everything managed by the one script rather than have to compute results on our own.
Comment 5 Saam Barati 2016-05-20 18:47:12 PDT
Comment on attachment 279496 [details]
Patch

rs=me
Comment 6 Yusuke Suzuki 2016-05-24 02:51:17 PDT
Hm, it seems that cq does not work well...
Comment 7 Keith Miller 2016-05-24 09:32:45 PDT
(In reply to comment #6)
> Hm, it seems that cq does not work well...

Yeah, I guess I'll have to land this by hand :/
Comment 8 Keith Miller 2016-05-24 11:59:47 PDT
Committed r201339: <http://trac.webkit.org/changeset/201339>
Comment 9 Csaba Osztrogonác 2016-05-25 00:03:10 PDT
(In reply to comment #8)
> Committed r201339: <http://trac.webkit.org/changeset/201339>

These new tests fail on all performace bots, please fix or skip them. Otherwise why didn't you run them before committing?
Comment 10 Keith Miller 2016-05-25 00:27:53 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Committed r201339: <http://trac.webkit.org/changeset/201339>
> 
> These new tests fail on all performace bots, please fix or skip them.
> Otherwise why didn't you run them before committing?

I didn't know the run-perf-tests script existed. I'll disable the tests for now, although I'll have to figure out how to do that.
Comment 11 Csaba Osztrogonác 2016-05-25 01:59:01 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Committed r201339: <http://trac.webkit.org/changeset/201339>
> > 
> > These new tests fail on all performace bots, please fix or skip them.
> > Otherwise why didn't you run them before committing?

Log:
ERROR: layer at (0,0) size 800x600

Maybe it is trivial to fix them, the general rule that a performance
tests shouldn't dump any extra output. Otherwise it will fail.
 
> I didn't know the run-perf-tests script existed. I'll disable the tests for
> now, although I'll have to figure out how to do that.

Use PerformanceTests/Skipped file to skip them.
Comment 12 Keith Miller 2016-05-25 02:06:53 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > Committed r201339: <http://trac.webkit.org/changeset/201339>
> > > 
> > > These new tests fail on all performace bots, please fix or skip them.
> > > Otherwise why didn't you run them before committing?
> 
> Log:
> ERROR: layer at (0,0) size 800x600
> 
> Maybe it is trivial to fix them, the general rule that a performance
> tests shouldn't dump any extra output. Otherwise it will fail.

Interesting, I'll look into that. I'm not sure if the run-jsc-benchmarks test harness uses that information or not however.

>  
> > I didn't know the run-perf-tests script existed. I'll disable the tests for
> > now, although I'll have to figure out how to do that.
> 
> Use PerformanceTests/Skipped file to skip them.

Should be fixed in http://trac.webkit.org/changeset/201372, hopefully.
Comment 13 Yusuke Suzuki 2016-05-25 02:10:42 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Committed r201339: <http://trac.webkit.org/changeset/201339>
> 
> These new tests fail on all performace bots, please fix or skip them.
> Otherwise why didn't you run them before committing?

FYI, EFL / GTK ports can run this JSBench from run-jsc-benchmark script now :)
http://trac.webkit.org/changeset/201365