118481 – 30% JSBench regression (caused by adding column numbers to stack traces)

Bug 118481 - 30% JSBench regression (caused by adding column numbers to stack traces)

Summary: 30% JSBench regression (caused by adding column numbers to stack traces)

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Lam

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2013-07-08 13:25 PDT by Mark Lam
Modified:	2013-07-23 18:36 PDT (History)
CC List:	14 users (show)

See Also:

Attachments
the patch (353.34 KB, patch) 2013-07-08 15:42 PDT, Mark Lam	eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
patch 2 (353.46 KB, patch) 2013-07-08 15:57 PDT, Mark Lam	eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
patch 3: Use MathExtras.h, not StdLibExtras.h (353.46 KB, patch) 2013-07-08 16:08 PDT, Mark Lam	mhahnenberg: review+ eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
patch 4: addressed feedback comments and attempt to appease EWS bots again. (358.74 KB, patch) 2013-07-09 00:33 PDT, Mark Lam	ggaren: review+	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

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

Description Mark Lam 2013-07-08 13:25:08 PDT

http://trac.webkit.org/changeset/148720 caused a 30% JSBench regression. 

ref: <rdar://problem/14241573>.

Comment 1 Mark Lam 2013-07-08 15:42:24 PDT

Created attachment 206275 [details]
the patch

Comment 2 EFL EWS Bot 2013-07-08 15:48:04 PDT

Comment on attachment 206275 [details]
the patch

Attachment 206275 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1052012

Comment 3 Early Warning System Bot 2013-07-08 15:49:11 PDT

Comment on attachment 206275 [details]
the patch

Attachment 206275 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/946789

Comment 4 Mark Lam 2013-07-08 15:49:39 PDT

Comment on attachment 206275 [details]
the patch

Will look into the UINT_MAX build failures on other platforms.

Comment 5 Mark Lam 2013-07-08 15:57:59 PDT

Created attachment 206278 [details]
patch 2

Comment 6 EFL EWS Bot 2013-07-08 16:02:30 PDT

Comment on attachment 206278 [details]
patch 2

Attachment 206278 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1028347

Comment 7 EFL EWS Bot 2013-07-08 16:04:58 PDT

Comment on attachment 206278 [details]
patch 2

Attachment 206278 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1002346

Comment 8 Mark Lam 2013-07-08 16:08:11 PDT

Created attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

Comment 9 EFL EWS Bot 2013-07-08 16:12:17 PDT

Comment on attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

Attachment 206280 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1029226

Comment 10 EFL EWS Bot 2013-07-08 16:13:30 PDT

Comment on attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

Attachment 206280 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1028349

Comment 11 Early Warning System Bot 2013-07-08 16:15:10 PDT

Comment on attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

Attachment 206280 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1048244

Comment 12 Early Warning System Bot 2013-07-08 16:16:42 PDT

Comment on attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

Attachment 206280 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1041109

Comment 13 kov's GTK+ EWS bot 2013-07-08 16:26:38 PDT

Comment on attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

Attachment 206280 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1028351

Comment 14 Mark Hahnenberg 2013-07-08 16:57:23 PDT

Comment on attachment 206280 [details]
patch 3: Use MathExtras.h, not StdLibExtras.h

View in context: https://bugs.webkit.org/attachment.cgi?id=206280&action=review

r=me with some modifications and green EWS bots.

> Source/JavaScriptCore/ChangeLog:27
> +        1. We do not want to perturb the lexer and parser as minimally as possible.

Odd phrasing.

> Source/JavaScriptCore/ChangeLog:30
> +        2. We regard the divot as the source character position we're are interested

We are

> Source/JavaScriptCore/ChangeLog:43
> +           UnlinkeCodeBlocks into CodeBlocks.

UnlinkedCodeBlocks

> Source/JavaScriptCore/ChangeLog:57
> +          startOffset exclusively throughout the system to for consistency.

for

> Source/JavaScriptCore/ChangeLog:67
> +        - Also fixed some bugs in what divot positions to be used. For example,

Odd phrasing.

> Source/JavaScriptCore/ChangeLog:68
> +          both Eval and Function expressions will previously use column numbers

Future vs. past tense.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1677
> +        startColumn += (!unlinkedExecutable->firstLineOffset()) ? ownerExecutable->startColumn() : 1;

Invert logic to avoid extra negation.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1691
> +        startColumn += (!unlinkedExecutable->firstLineOffset()) ? ownerExecutable->startColumn() : 1;

Ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2479
> +    // We can't assert this here because this is called from the CodeBlock
> +    // constructor before instructions() is initialized.
> +    // RELEASE_ASSERT(bytecodeOffset < instructions().size());

No need to keep this around. If somebody adds it later, they'll die in a fire anyways.

> Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:45
> +        Mode0LineShift = 8,
> +        Mode0LineMask = (1 << 22) - 1,
> +        Mode0ColumnMask = (1 << 8) - 1,
> +        Mode1LineShift = 22,
> +        Mode1LineMask = (1 << 8) - 1,
> +        Mode1ColumnMask = (1 << 22) - 1

Give meaningful names to Modes, e.g. FatColumn, FatLine, FatEverything, etc.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:126
> +    unsigned startColumn = m_functionStartColumn + 1; // Convert to base 1.

1-indexed? Or maybe more english.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:278
> +        low = 1; // Use nearest neighbor i.e. 1st entry.

Comment is confusing. Remove?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:286
> +    case 0:

Named enum please.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:347
> +    case 0:
> +        info.encodeMode0(line, column);
> +        break;
> +    case 1:
> +        info.encodeMode1(line, column);
> +        break;
> +    case 2: {
> +        createRareDataIfNecessary();
> +        unsigned fatIndex = m_rareData->m_expressionInfoFatPositions.size();
> +        ExpressionRangeInfo::FatPosition fatPos = { line, column };
> +        m_rareData->m_expressionInfoFatPositions.append(fatPos);
> +        info.position = fatIndex;

Ditto.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:349
> +    };

Extra semicolon.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1742
> +            generator.emitExpressionInfo(divot(), divotStartOffset(), divotEndOffset(), divotLine(), divotLineStart());

You mentioned in person that these are the fields of ThrowableExpressionData, so you could probably just pass that object by reference and it would eliminate a lot of the noise here. Not necessary for this patch though.

> Source/JavaScriptCore/parser/ASTBuilder.h:44
> +        BinaryOpInfo(int s, int d, int e, unsigned dLine, unsigned dLineStart, bool r)

Let's rename these while we're here since they're terribly named.

> Source/JavaScriptCore/parser/Lexer.cpp:786
> +        int identifierLength = currentCodePtr() - identifierStart;

currentSourcePtr?

Comment 15 Geoffrey Garen 2013-07-08 17:06:40 PDT

This patch would benefit a lot from per-function commentary. A giant list of function names with no comments is not a great ChangeLog.

Comment 16 Mark Lam 2013-07-09 00:33:04 PDT

Created attachment 206297 [details]
patch 4: addressed feedback comments and attempt to appease EWS bots again.

Comment 17 WebKit Commit Bot 2013-07-09 00:34:39 PDT

Attachment 206297 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/execCommand/outdent-blockquote-test1-expected.txt', u'LayoutTests/editing/execCommand/outdent-blockquote-test2-expected.txt', u'LayoutTests/editing/execCommand/outdent-blockquote-test3-expected.txt', u'LayoutTests/editing/execCommand/outdent-blockquote-test4-expected.txt', u'LayoutTests/editing/pasteboard/copy-paste-float-expected.txt', u'LayoutTests/editing/pasteboard/paste-blockquote-before-blockquote-expected.txt', u'LayoutTests/editing/pasteboard/paste-double-nested-blockquote-before-blockquote-expected.txt', u'LayoutTests/fast/dom/Window/window-resize-contents-expected.txt', u'LayoutTests/fast/events/remove-target-with-shadow-in-drag-expected.txt', u'LayoutTests/fast/js/line-column-numbers-expected.txt', u'LayoutTests/fast/js/line-column-numbers.html', u'LayoutTests/fast/js/script-tests/line-column-numbers.js', u'LayoutTests/fast/js/stack-trace-expected.txt', u'LayoutTests/inspector/console/console-url-line-column-expected.txt', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.h', u'Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h', u'Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp', u'Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/Lexer.cpp', u'Source/JavaScriptCore/parser/Lexer.h', u'Source/JavaScriptCore/parser/NodeConstructors.h', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/Parser.h', u'Source/JavaScriptCore/parser/ParserTokens.h', u'Source/JavaScriptCore/parser/SourceCode.h', u'Source/JavaScriptCore/parser/SourceProvider.cpp', u'Source/JavaScriptCore/parser/SourceProvider.h', u'Source/JavaScriptCore/parser/SourceProviderCacheItem.h', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/JavaScriptCore/runtime/CodeCache.cpp', u'Source/JavaScriptCore/runtime/Executable.cpp', u'Source/JavaScriptCore/runtime/Executable.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/ScriptCallStackFactory.cpp', u'Source/WebCore/bindings/js/ScriptSourceCode.h']" exit_code: 1
Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:37:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:38:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 18 Mark Lam 2013-07-09 00:56:52 PDT

Here are some benchmark results:
I did 3 runs comparing the following 2 builds:
1. baseline: trunk r151920 with StackFrame::column() stubbed to always return 0.  This negates the regression to yield perf that we should expect to match.
2. trunk r151920 plus this patch.

                                                       Conf#1                    Conf#2                                      
JSBench:
   amazon                                          8.3333+-0.3128            8.3333+-0.3128        
   facebook                                       38.9167+-0.5720     ?     39.1667+-0.5304        ?
   google                                         79.4167+-0.7399     ?     80.0000+-0.9385        ?
   twitter                                        10.2500+-0.2874            9.9167+-0.3272          might be 1.0336x faster
   yahoo                                           3.5833+-0.3272            3.5833+-0.3272        

   <arithmetic> *                                 28.1000+-0.2331     ?     28.2000+-0.2099        ? might be 1.0036x slower
   <geometric>                                    15.6423+-0.3311           15.5773+-0.2499          might be 1.0042x faster
   <harmonic>                                      9.2930+-0.4600            9.2314+-0.4182          might be 1.0067x faster

The JSBench regression is fixed.

The only regression I saw were the following (which only manifested on 1 run and disappeared in subsequent runs):

Octane and V8v7:
   decrypt                                        8.51886+-0.01023    !     8.55913+-0.01885       ! definitely 1.0047x slower
   earley                                         0.90463+-0.00511    !     0.91907+-0.00788       ! definitely 1.0160x slower

There were a few "definitely faster"s here and there in each of the runs, but they were inconsistent across runs.  However, on all 3 runs, the following improvement was consistently:

JSRegress:
   method-on-number                               24.6593+-0.8551     ^     22.4434+-0.5250        ^ definitely 1.0987x faster

Overall, the performance is unchanged from pre-regression numbers.

Here's the complete benchmark results output from the 3rd run:

$ ../Internal/Tools/Scripts/run-jsc-benchmarks /Volumes/Data/ws6/OpenSource/WebKitBuild/Release/DumpRenderTree /Volumes/Data/ws7/OpenSource/WebKitBuild/Release/DumpRenderTree
1516/1516                                                          
Generating benchmark report at /Volumes/Data/ws7/OpenSource/SunSpiderV8SpiderOctaneKrakenJSBenchJSRegressDSP_chronon_20130708_2315_report.txt
And raw data at /Volumes/Data/ws7/OpenSource/SunSpiderV8SpiderOctaneKrakenJSBenchJSRegressDSP_chronon_20130708_2315.json

Benchmark report for SunSpider, V8Spider, Octane, Kraken, JSBench, JSRegress, and DSP on chronon (MacPro5,1).

VMs tested:
"Conf#1" at /Volumes/Data/ws6/OpenSource/WebKitBuild/Release/DumpRenderTree (r151920)
"Conf#2" at /Volumes/Data/ws7/OpenSource/WebKitBuild/Release/DumpRenderTree (r151920)

Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get
microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                                                       Conf#1                    Conf#2                                      
SunSpider:
   3d-cube                                        10.5983+-0.2765           10.3337+-0.3564          might be 1.0256x faster
   3d-morph                                        8.5913+-0.1632     ?      8.5977+-0.0965        ?
   3d-raytrace                                    11.0687+-0.2897     ?     11.1038+-0.2604        ?
   access-binary-trees                             2.7348+-0.3472            2.7180+-0.3528        
   access-fannkuch                                 7.5137+-0.0934     ?      7.5345+-0.0891        ?
   access-nbody                                    4.5810+-0.0410     ?      4.5914+-0.0725        ?
   access-nsieve                                   4.8817+-0.0789     ?      4.9136+-0.0420        ?
   bitops-3bit-bits-in-byte                        1.7378+-0.0133     ?      1.7487+-0.0202        ?
   bitops-bits-in-byte                             6.5368+-0.0640     ?      6.5753+-0.1400        ?
   bitops-bitwise-and                              2.5359+-0.0462     ?      2.5554+-0.0480        ?
   bitops-nsieve-bits                              4.4729+-0.0203     ?      4.5187+-0.0494        ? might be 1.0102x slower
   controlflow-recursive                           2.8603+-0.0273     ?      2.8812+-0.0421        ?
   crypto-aes                                      8.1578+-0.3654     ?      8.4396+-0.3089        ? might be 1.0345x slower
   crypto-md5                                      4.2111+-0.0609     ?      4.2870+-0.0656        ? might be 1.0180x slower
   crypto-sha1                                     3.3234+-0.0351     ?      3.3478+-0.0330        ?
   date-format-tofte                              13.5844+-0.9691     ?     13.6889+-1.0077        ?
   date-format-xparb                              10.3460+-0.7148     ?     10.4049+-0.6199        ?
   math-cordic                                     3.7852+-0.0207     ?      3.8242+-0.0559        ? might be 1.0103x slower
   math-partial-sums                              11.7870+-0.1174     ?     11.8209+-0.1201        ?
   math-spectral-norm                              3.0558+-0.0202     ?      3.1043+-0.0329        ? might be 1.0159x slower
   regexp-dna                                     12.2062+-0.5079     ?     12.5089+-0.5233        ? might be 1.0248x slower
   string-base64                                   5.7428+-0.5650            5.6901+-0.5791        
   string-fasta                                   11.8516+-0.1933           11.7244+-0.1932          might be 1.0108x faster
   string-tagcloud                                14.0472+-0.2779     ?     14.1150+-0.2608        ?
   string-unpack-code                             30.2820+-0.3081           29.9551+-0.3587          might be 1.0109x faster
   string-validate-input                           9.2587+-0.3341     ?      9.2740+-0.2422        ?

   <arithmetic> *                                  8.0674+-0.1442     ?      8.0868+-0.1445        ? might be 1.0024x slower
   <geometric>                                     6.4611+-0.1080     ?      6.4916+-0.1088        ? might be 1.0047x slower
   <harmonic>                                      5.1998+-0.0846     ?      5.2295+-0.0863        ? might be 1.0057x slower

                                                       Conf#1                    Conf#2                                      
V8Spider:
   crypto                                         88.0549+-0.8940           87.3878+-0.6061        
   deltablue                                     122.2087+-0.5894     ?    122.7571+-0.5470        ?
   earley-boyer                                   81.6742+-0.6228     ?     82.2235+-0.5754        ?
   raytrace                                       60.0328+-0.2072     ?     60.3867+-0.3428        ?
   regexp                                        100.2016+-0.6094          100.1242+-0.8896        
   richards                                      115.2345+-0.7356     ?    115.2864+-0.7810        ?
   splay                                          61.3719+-3.7543           60.1047+-3.5444          might be 1.0211x faster

   <arithmetic>                                   89.8255+-0.7315           89.7529+-0.7470          might be 1.0008x faster
   <geometric> *                                  86.8451+-0.9072           86.7038+-0.9410          might be 1.0016x faster
   <harmonic>                                     83.8341+-1.1016           83.6252+-1.1631          might be 1.0025x faster

                                                       Conf#1                    Conf#2                                      
Octane and V8v7:
   encrypt                                        0.47472+-0.00095    ?     0.47476+-0.00096       ?
   decrypt                                        8.56644+-0.01826    ?     8.57887+-0.01339       ?
   deltablue                             x2       0.57652+-0.00475          0.57251+-0.00656       
   earley                                         0.91083+-0.00594    ?     0.91686+-0.00664       ?
   boyer                                         12.35298+-0.05573    ?    12.40401+-0.04465       ?
   raytrace                              x2       4.62326+-0.06480          4.57383+-0.05294         might be 1.0108x faster
   regexp                                x2      31.35191+-0.20854    ^    30.97844+-0.09720       ^ definitely 1.0121x faster
   richards                              x2       0.29810+-0.00075          0.29761+-0.00081       
   splay                                 x2       0.66287+-0.02633          0.65662+-0.02828       
   navier-stokes                         x2      10.45721+-0.01679         10.45667+-0.01530       
   closure                                        0.35727+-0.01362    ?     0.36312+-0.01440       ? might be 1.0164x slower
   jquery                                         4.37932+-0.52820    ?     4.44716+-0.54185       ? might be 1.0155x slower
   gbemu                                 x2     129.82489+-1.31453    ?   133.46258+-5.05251       ? might be 1.0280x slower
   mandreel                              x2     175.86859+-1.69818    ?   176.11658+-0.77637       ?
   pdfjs                                 x2     104.16363+-0.65458        103.26986+-0.43108       
   box2d                                 x2      33.24659+-0.17972    !    33.77435+-0.16183       ! definitely 1.0159x slower

V8v7:
   <arithmetic>                                   7.39030+-0.03068    ^     7.34036+-0.01696       ^ definitely 1.0068x faster
   <geometric> *                                  2.41733+-0.01401          2.40678+-0.01235         might be 1.0044x faster
   <harmonic>                                     0.92517+-0.00619          0.92179+-0.00606         might be 1.0037x faster

Octane including V8v7:
   <arithmetic>                                  38.81495+-0.14991    ?    39.05780+-0.40261       ? might be 1.0063x slower
   <geometric> *                                  7.08948+-0.03902    ?     7.09816+-0.05201       ? might be 1.0012x slower
   <harmonic>                                     1.27179+-0.00900          1.27094+-0.00959         might be 1.0007x faster

                                                       Conf#1                    Conf#2                                      
Kraken:
   ai-astar                                       505.305+-1.170            504.473+-1.149         
   audio-beat-detection                           241.182+-1.376            240.513+-2.112         
   audio-dft                                      304.652+-1.569            302.167+-1.876         
   audio-fft                                      141.326+-0.300            141.183+-0.467         
   audio-oscillator                               240.799+-0.545      ?     241.360+-0.642         ?
   imaging-darkroom                               281.981+-0.979            281.254+-0.511         
   imaging-desaturate                             154.926+-0.413            154.264+-0.345         
   imaging-gaussian-blur                          487.954+-2.861            484.302+-1.024         
   json-parse-financial                            78.978+-0.187      ^      77.613+-0.408         ^ definitely 1.0176x faster
   json-stringify-tinderbox                       107.870+-0.388      ^     103.490+-2.339         ^ definitely 1.0423x faster
   stanford-crypto-aes                            114.620+-0.699      ?     114.812+-0.612         ?
   stanford-crypto-ccm                            112.224+-0.532            111.302+-0.447         
   stanford-crypto-pbkdf2                         259.409+-1.129      ^     256.615+-1.096         ^ definitely 1.0109x faster
   stanford-crypto-sha256-iterative               120.759+-0.666            119.934+-0.542         

   <arithmetic> *                                 225.142+-0.402      ^     223.806+-0.307         ^ definitely 1.0060x faster
   <geometric>                                    191.988+-0.268      ^     190.494+-0.290         ^ definitely 1.0078x faster
   <harmonic>                                     165.890+-0.223      ^     164.185+-0.329         ^ definitely 1.0104x faster

                                                       Conf#1                    Conf#2                                      
JSBench:
   amazon                                          8.3333+-0.3128            8.3333+-0.3128        
   facebook                                       38.9167+-0.5720     ?     39.1667+-0.5304        ?
   google                                         79.4167+-0.7399     ?     80.0000+-0.9385        ?
   twitter                                        10.2500+-0.2874            9.9167+-0.3272          might be 1.0336x faster
   yahoo                                           3.5833+-0.3272            3.5833+-0.3272        

   <arithmetic> *                                 28.1000+-0.2331     ?     28.2000+-0.2099        ? might be 1.0036x slower
   <geometric>                                    15.6423+-0.3311           15.5773+-0.2499          might be 1.0042x faster
   <harmonic>                                      9.2930+-0.4600            9.2314+-0.4182          might be 1.0067x faster

                                                       Conf#1                    Conf#2                                      
JSRegress:
   adapt-to-double-divide                         21.5129+-0.1556     ?     21.5728+-0.1917        ?
   aliased-arguments-getbyval                      0.9027+-0.0104     ?      0.9145+-0.0121        ? might be 1.0131x slower
   allocate-big-object                             4.0667+-1.2710            4.0464+-1.3365        
   arity-mismatch-inlining                         0.7795+-0.0217            0.7531+-0.0130          might be 1.0350x faster
   array-access-polymorphic-structure              8.9606+-2.0809     ?      9.0100+-2.0366        ?
   array-nonarray-polymorhpic-access              57.6663+-0.7133           57.5824+-0.6685        
   array-with-double-add                           5.5026+-0.0687     ?      5.5191+-0.0657        ?
   array-with-double-increment                     3.7906+-0.0805            3.7277+-0.0220          might be 1.0169x faster
   array-with-double-mul-add                       7.5251+-0.0563            7.4802+-0.0685        
   array-with-double-sum                           7.4026+-0.0555     ?      7.5346+-0.1570        ? might be 1.0178x slower
   array-with-int32-add-sub                        9.9740+-0.0844     ?      9.9908+-0.1042        ?
   array-with-int32-or-double-sum                  7.5312+-0.0838            7.5188+-0.0641        
   big-int-mul                                     4.6741+-0.0584            4.6232+-0.0365          might be 1.0110x faster
   boolean-test                                    4.2394+-0.1591            4.2349+-0.1597        
   branch-fold                                     4.4865+-0.0616            4.4247+-0.0245          might be 1.0139x faster
   cast-int-to-double                             13.2526+-0.0792     ?     13.3408+-0.1576        ?
   cell-argument                                  13.6537+-0.0381     ?     13.7168+-0.0943        ?
   cfg-simplify                                    3.6552+-0.0283     ?      3.7068+-0.0712        ? might be 1.0141x slower
   cmpeq-obj-to-obj-other                          9.9848+-0.1399     ?     10.0627+-0.2921        ?
   constant-test                                   8.0306+-0.0849     ?      8.0438+-0.0889        ?
   direct-arguments-getbyval                       0.8450+-0.0177     ?      0.8887+-0.0876        ? might be 1.0517x slower
   double-pollution-getbyval                      10.1506+-0.0667     ?     10.1974+-0.0781        ?
   double-pollution-putbyoffset                    5.4836+-0.5872            5.4723+-0.5893        
   empty-string-plus-int                          13.5046+-0.2536           13.4198+-0.2392        
   external-arguments-getbyval                     2.4188+-0.1268     ?      2.5163+-0.1773        ? might be 1.0403x slower
   external-arguments-putbyval                     3.8025+-0.2302            3.7220+-0.2460          might be 1.0216x faster
   Float32Array-matrix-mult                       14.8329+-0.7054     ?     15.1350+-0.7712        ? might be 1.0204x slower
   fold-double-to-int                             21.0975+-0.3986           20.9080+-0.1899        
   function-dot-apply                              3.0342+-0.0504     ?      3.0397+-0.0392        ?
   function-test                                   4.4344+-0.0783     ?      4.4723+-0.0925        ?
   get-by-id-chain-from-try-block                  6.4962+-0.1549     ?      6.5563+-0.1312        ?
   HashMap-put-get-iterate-keys                   60.1414+-0.6092           59.3811+-0.5760          might be 1.0128x faster
   HashMap-put-get-iterate                        67.3874+-0.9790     ?     67.4469+-0.6877        ?
   HashMap-string-put-get-iterate                 68.8419+-0.6297     ?     69.8662+-0.7830        ? might be 1.0149x slower
   imul-double-only                               16.7749+-0.0966     ?     17.0798+-0.6781        ? might be 1.0182x slower
   imul-int-only                                  14.0954+-0.0996     ?     14.1534+-0.1357        ?
   imul-mixed                                     21.4152+-0.6407           21.0793+-0.0924          might be 1.0159x faster
   indexed-properties-in-objects                   4.1960+-0.0120     ?      4.2549+-0.0558        ? might be 1.0140x slower
   inline-arguments-access                         1.2183+-0.0169     ?      1.2454+-0.0334        ? might be 1.0222x slower
   inline-arguments-local-escape                  26.2746+-1.7036           25.5814+-0.3002          might be 1.0271x faster
   inline-get-scoped-var                           6.1853+-0.1011            6.1721+-0.0802        
   inlined-put-by-id-transition                   16.6993+-0.1870           16.4672+-0.2381          might be 1.0141x faster
   int-or-other-abs-then-get-by-val                8.9272+-0.0750     ?      9.0033+-0.1297        ?
   int-or-other-abs-zero-then-get-by-val          34.6323+-0.1743           34.4718+-0.0547        
   int-or-other-add-then-get-by-val                9.7517+-0.0915     ?      9.7766+-0.1097        ?
   int-or-other-add                               10.0913+-0.0928           10.0905+-0.1045        
   int-or-other-div-then-get-by-val                7.5461+-0.0716     ?      7.5670+-0.0507        ?
   int-or-other-max-then-get-by-val                9.2507+-0.1950     ?      9.2619+-0.2078        ?
   int-or-other-min-then-get-by-val                7.6303+-0.0549     ?      7.6739+-0.0988        ?
   int-or-other-mod-then-get-by-val                7.6554+-0.0828            7.6385+-0.0699        
   int-or-other-mul-then-get-by-val                6.7943+-0.0595     ?      6.8194+-0.0602        ?
   int-or-other-neg-then-get-by-val                7.6714+-0.0650            7.6171+-0.0648        
   int-or-other-neg-zero-then-get-by-val          35.0551+-0.1331           34.8493+-0.4179        
   int-or-other-sub-then-get-by-val                9.7732+-0.0777            9.7458+-0.0535        
   int-or-other-sub                                7.8232+-0.0834            7.7656+-0.0366        
   int-overflow-local                             12.4105+-0.1334           12.3328+-0.0806        
   Int16Array-bubble-sort                         76.7656+-3.4355           74.8517+-2.7941          might be 1.0256x faster
   Int16Array-load-int-mul                         1.7787+-0.0246     ?      1.8059+-0.0367        ? might be 1.0153x slower
   Int8Array-load                                  5.1773+-0.0489     ?      5.1924+-0.0977        ?
   integer-divide                                 14.7300+-0.0951           14.5900+-0.0615        
   integer-modulo                                  2.0654+-0.0361     ?      2.0907+-0.0406        ? might be 1.0123x slower
   make-indexed-storage                            4.7438+-0.7220            4.3928+-0.6187          might be 1.0799x faster
   method-on-number                               24.6593+-0.8551     ^     22.4434+-0.5250        ^ definitely 1.0987x faster
   negative-zero-divide                            0.3918+-0.0118     ?      0.4040+-0.0125        ? might be 1.0312x slower
   negative-zero-modulo                            0.3822+-0.0125     ?      0.3940+-0.0145        ? might be 1.0309x slower
   negative-zero-negate                            0.3596+-0.0094     ?      0.3716+-0.0133        ? might be 1.0332x slower
   nested-function-parsing-random                377.2842+-11.5469    ?    389.3693+-12.9024       ? might be 1.0320x slower
   nested-function-parsing                        55.2121+-3.4459           55.1201+-3.6977        
   new-array-buffer-dead                           3.5456+-0.1041            3.5341+-0.1175        
   new-array-buffer-push                          15.3677+-2.5655           15.1236+-2.4074          might be 1.0161x faster
   new-array-dead                                 27.0225+-0.1051     ?     27.0282+-0.1494        ?
   new-array-push                                 11.9112+-1.9472           11.7574+-2.0049          might be 1.0131x faster
   number-test                                     4.1602+-0.1815     ?      4.2380+-0.1720        ? might be 1.0187x slower
   object-closure-call                             8.1622+-0.1544     ?      8.1712+-0.1922        ?
   object-test                                     4.5076+-0.1694            4.4257+-0.1434          might be 1.0185x faster
   poly-stricteq                                  87.5031+-0.4203     ?     87.8466+-0.3063        ?
   polymorphic-structure                          19.1513+-0.1477     ?     19.2449+-0.1660        ?
   polyvariant-monomorphic-get-by-id              11.9152+-0.0626           11.8943+-0.0631        
   rare-osr-exit-on-local                         19.1659+-0.1017           19.0607+-0.0569        
   register-pressure-from-osr                     30.1645+-0.0857           30.1319+-0.1136        
   simple-activation-demo                         33.1924+-0.2614           33.1419+-0.2367        
   slow-array-profile-convergence                  4.4903+-0.2336            4.4718+-0.2692        
   slow-convergence                                3.6492+-0.0453            3.6447+-0.0414        
   sparse-conditional                              1.2713+-0.0258            1.2409+-0.0311          might be 1.0245x faster
   splice-to-remove                               46.3106+-0.1722     ^     45.6369+-0.2878        ^ definitely 1.0148x faster
   string-concat-object                            4.8326+-1.4336            4.7915+-1.4410        
   string-concat-pair-object                       4.7104+-1.4125     ?      4.7740+-1.5286        ? might be 1.0135x slower
   string-concat-pair-simple                      19.5167+-0.3872           19.4382+-0.5411        
   string-concat-simple                           19.6723+-0.4552     ?     19.7557+-0.4802        ?
   string-cons-repeat                             14.3220+-0.9313     ?     14.4121+-0.9065        ?
   string-cons-tower                              37.0753+-22.2249    ?     38.7243+-23.9378       ? might be 1.0445x slower
   string-equality                                50.0715+-0.3861           49.9474+-0.4084        
   string-hash                                     2.5011+-0.0182            2.4964+-0.0268        
   string-repeat-arith                            44.7431+-0.2930     ?     45.0013+-0.2812        ?
   string-sub                                     85.1287+-0.7429     ?     86.0621+-0.5636        ? might be 1.0110x slower
   string-test                                     3.9720+-0.0737     ?      4.0430+-0.0938        ? might be 1.0179x slower
   structure-hoist-over-transitions                3.8754+-0.6272     ?      3.9137+-0.6123        ?
   tear-off-arguments-simple                       1.7065+-0.0215     ?      1.7340+-0.0185        ? might be 1.0161x slower
   tear-off-arguments                              3.1406+-0.0105     ?      3.1678+-0.0271        ?
   temporal-structure                             19.8228+-0.0307     ?     19.9198+-0.0912        ?
   to-int32-boolean                               29.3214+-0.1246     ?     29.4131+-0.2247        ?
   undefined-test                                  4.3068+-0.1455     ?      4.3154+-0.1495        ?

   <arithmetic>                                   19.6637+-0.4600     ?     19.7549+-0.4746        ? might be 1.0046x slower
   <geometric> *                                   8.8666+-0.1783     ?      8.8681+-0.1746        ? might be 1.0002x slower
   <harmonic>                                      3.8757+-0.0400     ?      3.9143+-0.0404        ? might be 1.0100x slower

                                                       Conf#1                    Conf#2                                      
DSP:
   filtrr-posterize-tint                          51.7666+-0.7969     ?     51.8432+-1.0021        ?
   filtrr-tint-contrast-sat-bright                72.9019+-1.2183     ?     73.0174+-1.0078        ?
   filtrr-tint-sat-adj-contr-mult                 89.6488+-3.8176           88.7318+-3.4479          might be 1.0103x faster
   filtrr-blur-overlay-sat-contr                 225.3870+-2.0221          224.7883+-1.9933        
   filtrr-sat-blur-mult-sharpen-contr            264.3871+-5.9968          263.8270+-5.9629        
   filtrr-sepia-bias                              37.7213+-1.9537           37.0576+-1.8604          might be 1.0179x faster
   route9-vp8                            x5     1190.6626+-17.6852    ?   1200.7561+-7.8342        ?
   starfield                             x5     1221.5781+-12.2917        1214.5346+-21.5427       
   bellard-jslinux                       x5     3019.5000+-12.3039    ?   3027.2500+-15.2251       ?
   zynaps-quake3                         x5     1259.3340+-25.6904    ?   1267.4471+-31.7251       ?
   zynaps-mandelbrot                     x5     1143.0610+-7.3298         1120.7955+-16.9067         might be 1.0199x faster
   ammojs-asm-js                         x5      243.9518+-11.3434    ?    249.9234+-11.0068       ? might be 1.0245x slower
   ammojs-regular-js                     x5      252.2456+-8.7234          250.7259+-8.3144        

   <arithmetic>                                 1033.9873+-5.9844     ?   1034.0592+-5.3582        ? might be 1.0001x slower
   <geometric> *                                 631.7172+-8.3780     ?    632.0220+-7.8136        ? might be 1.0005x slower
   <harmonic>                                    297.7304+-7.8529          297.2445+-7.5342          might be 1.0016x faster

                                                       Conf#1                    Conf#2                                      
All benchmarks:
   <arithmetic>                                  224.1601+-1.1432     ?    224.1617+-1.0130        ? might be 1.0000x slower
   <geometric>                                         ERROR                     ERROR             
   <harmonic>                                      4.2005+-0.0320     ?      4.2220+-0.0291        ? might be 1.0051x slower

                                                       Conf#1                    Conf#2                                      
Geomean of preferred means:
   <scaled-result>                                40.4060+-0.2498     ?     40.4075+-0.2373        ? might be 1.0000x slower

Comment 19 Geoffrey Garen 2013-07-09 08:51:29 PDT

Comment on attachment 206297 [details]
patch 4: addressed feedback comments and attempt to appease EWS bots again.

View in context: https://bugs.webkit.org/attachment.cgi?id=206297&action=review

r=me, but I think I spotted two things you should fix before landing.

> Source/JavaScriptCore/ChangeLog:69
> +        - Also fixed some bugs in the choice of divot positions to use. For example,
> +          both Eval and Function expressions previously use column numbers from
> +          the start of the expression but use the line number at the end of the
> +          expression. This is now fixed to be consistently either using the start

You've still got some present vs future tense confusion here. I think you meant "used" instead of "use" in two places here?

"to be consistently either using" is a pretty awkward verb phrase, too.

> Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:39
> +    //   1. FatLine:          22-bit line,  8-bit column.
> +    //   2. FatColumn:         8-bit line, 22-bit column.
> +    //   3. FatLineAndColumn: 32-bit line, 32-bit column.

The reason the style bot complains about this is that it makes the code hard for other engineers to edit / refactor later. For example, if we ever rename FatLine, FatColumn, or FatLineAndColumn (as you've already done once in preparing this patch), there's a lot of extra typing to line all the comments up again.

You might think that we should just make an exception for the select pieces of code that you like to line up this way, since there aren't very many of them. Unfortunately, it's not practical in a large, distributed open source project to maintain special commenting rules tailored to individual developers.

Comment 20 Mark Lam 2013-07-09 09:19:00 PDT

(In reply to comment #19)
> > Source/JavaScriptCore/ChangeLog:69
> > +        - Also fixed some bugs in the choice of divot positions to use. For example,
> > +          both Eval and Function expressions previously use column numbers from
> > +          the start of the expression but use the line number at the end of the
> > +          expression. This is now fixed to be consistently either using the start
> 
> You've still got some present vs future tense confusion here. I think you meant "used" instead of "use" in two places here?
> 
> "to be consistently either using" is a pretty awkward verb phrase, too.

Thanks for catching these.  Fixed.

> > Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:39
> > +    //   1. FatLine:          22-bit line,  8-bit column.
> > +    //   2. FatColumn:         8-bit line, 22-bit column.
> > +    //   3. FatLineAndColumn: 32-bit line, 32-bit column.
> 
> The reason the style bot complains about this is that it makes the code hard for other engineers to edit / refactor later. For example, if we ever rename FatLine, FatColumn, or FatLineAndColumn (as you've already done once in preparing this patch), there's a lot of extra typing to line all the comments up again.
> 
> You might think that we should just make an exception for the select pieces of code that you like to line up this way, since there aren't very many of them. Unfortunately, it's not practical in a large, distributed open source project to maintain special commenting rules tailored to individual developers.

OK, makes sense.  I took out the extra spaces.

Landed in r152494: <http://trac.webkit.org/changeset/152494>.

Comment 21 Mark Lam 2013-07-09 10:39:09 PDT

Also landed r152497: <http://trac.webkit.org/changeset/152497> to make the Windows bot happy.

Comment 22 Simon Fraser (smfr) 2013-07-23 10:45:36 PDT

The Apple Windows 7 Debug bot is hitting lots of assertions: bug 119017

Comment 23 Mark Lam 2013-07-23 18:36:45 PDT

(In reply to comment #22)
> The Apple Windows 7 Debug bot is hitting lots of assertions: bug 119017

FYI, this issue is resolved in https://bugs.webkit.org/show_bug.cgi?id=118996 with r153071: <http://trac.webkit.org/changeset/153071>.