Bug 118481

Created attachment 206275 [details] the patch

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 on attachment 206275 [details] the patch Attachment 206275 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/946789

Comment on attachment 206275 [details] the patch Will look into the UINT_MAX build failures on other platforms.

Created attachment 206278 [details] patch 2

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 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

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

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 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 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 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 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 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?

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

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

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.

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 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.

(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>.

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

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

(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>.