Description
Caitlin Potter (:caitp)
2016-08-30 14:56:53 PDT
TODO: - Put async function parsing behind compile-time flag (the runtime flag approach in 156147 was probably not worth it) - Fix bug 161408 so that test FIXME can be uncommented - Fix module parsing errors re: AWAIT keyword (Scope objects no longer know if they're parsing a module or not, have not yet introduced a fix) Created attachment 287437 [details]
Patch
Attachment 287437 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/parser/Keywords.table:10: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 287442 [details]
Patch
Get rid of the xcodeproj changes, since the new files aren't included in this patch.
Created attachment 287444 [details]
Async Function Parsing v1
Fix modules test failures
So, experimenting with this a bit, I'm finding that those "expensive branches" identified by Yusuke Suzuki can be (mostly) mitigated in the benchmarks by moving their contents to never-inlined functions. I guess this makes the on-stack allocation for the more commonly executed code smaller, and means the only extra work the "commonly executed" code has to do is the interned string comparison (testing if a particular string is "async" or not), which should be pretty cheap. The biggest one of these in the jQuery benchmark is, according to Instruments.app, the one in parseStatement(). Uninlining these rare branches might be a good way to go, but I'd appreciate a second set of eyes measuring that solution using different tools and settings as well. Some benchmark runs (just with jquery) with 4 VM runs and 10 inner runs: ``` Collected 40 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. 1) Baseline Patched jquery 6.76557+-0.13436 ? 6.89083+-0.08124 ? might be 1.0185x slower <geometric> 6.76557+-0.13436 ? 6.89083+-0.08124 ? might be 1.0185x slower 2) Baseline Patched jquery 7.06592+-0.09167 7.03025+-0.07808 <geometric> 7.06592+-0.09167 7.03025+-0.07808 might be 1.0051x faster ``` So, I dunno, that seems like an improvement over the existing regression to me. (In reply to comment #6) > So, experimenting with this a bit, I'm finding that those "expensive > branches" identified by Yusuke Suzuki can be (mostly) mitigated in the > benchmarks by moving their contents to never-inlined functions. I guess this > makes the on-stack allocation for the more commonly executed code smaller, > and means the only extra work the "commonly executed" code has to do is the > interned string comparison (testing if a particular string is "async" or > not), which should be pretty cheap. > > The biggest one of these in the jQuery benchmark is, according to > Instruments.app, the one in parseStatement(). Uninlining these rare branches > might be a good way to go, but I'd appreciate a second set of eyes measuring > that solution using different tools and settings as well. > > Some benchmark runs (just with jquery) with 4 VM runs and 10 inner runs: > > ``` > Collected 40 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. > > 1) > Baseline Patched > > > jquery 6.76557+-0.13436 ? 6.89083+-0.08124 ? > might be 1.0185x slower > > <geometric> 6.76557+-0.13436 ? 6.89083+-0.08124 ? > might be 1.0185x slower > > 2) > Baseline Patched > > > jquery 7.06592+-0.09167 7.03025+-0.07808 > > <geometric> 7.06592+-0.09167 7.03025+-0.07808 > might be 1.0051x faster > ``` > > So, I dunno, that seems like an improvement over the existing regression to > me. Great! Did you already upload the updated patch? no, but the change is pretty minor, so im not sure if it has the benefit i imagine it does. I'll put it up in a sec Created attachment 288192 [details]
Async Function Parsing v2
Patch to try to reduce codeload regression a bit
Ping: have you tried this and seen similar results re: the perf regression on jQuery tending to be significantly lower than it was originally? I'd like to establish that the perf is acceptable before adding the runtime portions (In reply to comment #10) > Ping: have you tried this and seen similar results re: the perf regression > on jQuery tending to be significantly lower than it was originally? I'd like > to establish that the perf is acceptable before adding the runtime portions Last night, I tried the patch in my Linux box (64bit), and I've got 1~1.5% regression in octane jquery and closure. Today, I'll re-check it in my OS X laptop. That still sounds pretty bad, if not worse than it was originally measured to be. I think I will keep looking for improvements. Comment on attachment 288192 [details] Async Function Parsing v2 View in context: https://bugs.webkit.org/attachment.cgi?id=288192&action=review > JSTests/stress/async-await-syntax.js:1 > +// Copyright (C) 2016 the V8 project authors. All rights reserved. Careful, lack of license is not acceptable for WebKit. I hope it's unintentional? Can you get it fixed to BSD, like other Chromium project stuff? Created attachment 288384 [details]
Patch
I took the benchmark on my OS X laptop last night. shell> Tools/Scripts/run-jsc-benchmarks baseline:WebKitBuild/async-master/Release/jsc patched:WebKitBuild/async/Release/jsc --outer=30 --octane --benchmark='closure|jquery' Generating benchmark report at /Users/yusukesuzuki/dev/WebKit/baseline_patched_Octane_dandelion_20160909_1144_report.txt And raw data at /Users/yusukesuzuki/dev/WebKit/baseline_patched_Octane_dandelion_20160909_1144.json Benchmark report for Octane on dandelion (MacBookPro10,1). VMs tested: "baseline" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/async-master/Release/jsc "patched" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/async/Release/jsc Collected 30 samples per benchmark/VM, with 30 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. baseline patched closure 0.59315+-0.00124 0.59265+-0.00138 jquery 7.91025+-0.02773 ? 7.91560+-0.03665 ? <geometric> 2.16606+-0.00448 2.16585+-0.00537 might be 1.0001x faster It seems that the performance is neutral. Do you have any ideas what might be the result of the poor performance on the linux runs? er, the cause of the poor linux perf. (In reply to comment #17) > er, the cause of the poor linux perf. OK, I've retaken the performance on my Linux box with rebooting. Generating benchmark report at /home/yusukesuzuki/dev/WebKit/baseline_patch_Octane_hanayamata_20160910_0605_report.txt And raw data at /home/yusukesuzuki/dev/WebKit/baseline_patch_Octane_hanayamata_20160910_0605.json Benchmark report for Octane on hanayamata. VMs tested: "baseline" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async-master/Release/bin/jsc "patch" at /home/yusukesuzuki/dev/WebKit/WebKitBuild/async/Release/bin/jsc Collected 10 samples per benchmark/VM, with 10 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. baseline patch closure 0.62470+-0.01647 0.62386+-0.00330 jquery 8.14041+-0.09801 ? 8.17138+-0.04920 ? <geometric> 2.25454+-0.02681 ? 2.25780+-0.00850 ? might be 1.0014x slower Seems good. Comment on attachment 288384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288384&action=review Could you hide async / await parsing feature behind compile time / run time flag? > Source/JavaScriptCore/ChangeLog:7 > + Could you note ChangeLog? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2980 > + ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode); Let's use SourceParseModeSet here. > Source/JavaScriptCore/parser/Parser.cpp:264 > + if (parseMode == SourceParseMode::GeneratorBodyMode || isAsyncFunctionBodyParseMode(parseMode)) Let's use SourceParseModeSet even if there is only one candidate. See the reason why https://bugs.webkit.org/show_bug.cgi?id=156147#c132. > Source/JavaScriptCore/parser/Parser.cpp:269 > + if (oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(parseMode) && !hasError()) { Could you change this oneOfSourceParseModes to one done in https://trac.webkit.org/changeset/201523 (SourceParseModeSet) ? > Source/JavaScriptCore/parser/Parser.cpp:315 > + if (oneOfSourceParseModes<SourceParseMode::GeneratorWrapperFunctionMode>(parseMode) || isAsyncFunctionWrapperParseMode(parseMode)) { Ditto > Source/JavaScriptCore/parser/Parser.cpp:559 > + info.parameterCount = 4; // generator, state, value, resume mode We no longer need this code since createGeneratorParameters automatically set info.parameterCount. And keep in mind that the updated generator takes 5th argument, @generatorFrame. > Source/JavaScriptCore/parser/Parser.cpp:629 > + // FIXME: These branch causes 1% regression in octane code-load jquery. Since we already solved this, let's drop it. > Source/JavaScriptCore/parser/Parser.cpp:1748 > + // FIXME: These branch causes 1% regression in octane code-load jquery. Ditto. > Source/JavaScriptCore/parser/Parser.cpp:1946 > + RELEASE_ASSERT(!(oneOfSourceParseModes<SourceParseMode::ProgramMode, SourceParseMode::ModuleAnalyzeMode, SourceParseMode::ModuleEvaluateMode>(mode))); Ditto > Source/JavaScriptCore/parser/Parser.cpp:1950 > + if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) { Ditto. > Source/JavaScriptCore/parser/Parser.cpp:2086 > + if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) Ditto. > Source/JavaScriptCore/parser/Parser.cpp:2129 > + if (UNLIKELY((oneOfSourceParseModes<SourceParseMode::ArrowFunctionMode, SourceParseMode::AsyncArrowFunctionMode>(mode)))) { Ditto. > Source/JavaScriptCore/parser/Parser.cpp:2179 > + if (!(functionDefinitionType == FunctionDefinitionType::Expression && oneOfSourceParseModes<SourceParseMode::NormalFunctionMode, SourceParseMode::AsyncFunctionMode>(mode))) Ditto. > Source/JavaScriptCore/parser/Parser.cpp:2294 > + RELEASE_ASSERT((oneOfSourceParseModes<SourceParseMode::NormalFunctionMode, SourceParseMode::MethodMode, SourceParseMode::ArrowFunctionMode, SourceParseMode::GeneratorBodyMode, SourceParseMode::GeneratorWrapperFunctionMode>(mode)) || isAsyncFunctionWrapperParseMode(mode)); Ditto. > Source/JavaScriptCore/parser/Parser.cpp:2722 > + // FIXME: These branch causes 1% regression in octane code-load jquery. Since it is already solved, we can drop this FIXME. > Source/JavaScriptCore/parser/Parser.cpp:3353 > + // FIXME: These branch causes 1% regression in octane code-load jquery. Ditto. > Source/JavaScriptCore/parser/Parser.cpp:3512 > + return context.createYield(location, argument, delegate, divotStart, argumentStart, lastTokenEndPosition()); How about using AwaitNode here and emit the similar code in code generation phase? > Source/JavaScriptCore/parser/Parser.cpp:4117 > + // FIXME: These branch causes 1% regression in octane code-load jquery. Ditto. > Source/JavaScriptCore/parser/ParserModes.h:76 > +static ALWAYS_INLINE bool oneOfSourceParseModes(SourceParseMode mode) Change to SourceParseModeSet version. > Source/JavaScriptCore/parser/ParserModes.h:83 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:100 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:110 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:117 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:125 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:132 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:142 > + return oneOfSourceParseModes< Ditto. > Source/JavaScriptCore/parser/ParserModes.h:149 > + return oneOfSourceParseModes<SourceParseMode::ProgramMode>(parseMode); Ditto. (In reply to comment #19) > Comment on attachment 288384 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288384&action=review > > Could you hide async / await parsing feature behind compile time / run time > flag? I'll get around to addressing the review comments on Monday. But, before I leave this for the weekend, question re: your overall comments. 1) If using the compile-time flag option, is there any way I can keep tests added in this patch running and passing? I haven't really found a way to do this. 2) If a runtime flag approach is used, I don't think the runtime flag approach from the other bug is a good way to go, since it means adding flags all over the place when it's not realistically needed. A single global boolean in runtime/Options.cpp would probably make more sense, and would avoid changes all over the place for storing parser flags in various places where the parser might need to be entered again. Does that seem reasonable, or is the RuntimeFlags class the absolute preferred way to go? Created attachment 288477 [details]
Async Function Parsing v3
Comment on attachment 288477 [details]
Async Function Parsing v3
Most of the review addressed, just waiting on an answer to the questions before adding the flag
(In reply to comment #22) > Comment on attachment 288477 [details] > Async Function Parsing v3 > > Most of the review addressed, just waiting on an answer to the questions > before adding the flag Runtime flag based on Options.{h, cpp} is ok to me. Created attachment 288528 [details]
Async Function Parsing v4
Comment on attachment 288528 [details]
Async Function Parsing v4
Added new version with Options::useAsyncFunctions() to hide the feature
with the flag, I am again seeing pretty poor results on the benchmarks. (In reply to comment #26) > with the flag, I am again seeing pretty poor results on the benchmarks. I've tried giving the Parser a local copy of the Options value, and rearranged branches which rely on it a bit, seems to help a bit locally, but maybe it's not really needed. (In reply to comment #27) > (In reply to comment #26) > > with the flag, I am again seeing pretty poor results on the benchmarks. > > I've tried giving the Parser a local copy of the Options value, and > rearranged branches which rely on it a bit, seems to help a bit locally, but > maybe it's not really needed. Oh... So compile time flag is OK. Thank you for trying! I'll try to do that next week... I guess all I can do for the test is add a //@ skip line? (In reply to comment #29) > I'll try to do that next week... I guess all I can do for the test is add a > //@ skip line? Yup! Created attachment 289286 [details]
Patch
I've changed this to use a feature flag, and am again seeing neutral, or effectively neutral parser performance. This seems like a good thing. Created attachment 289289 [details]
Async Function Parsing v5
Comment on attachment 289289 [details] Async Function Parsing v5 View in context: https://bugs.webkit.org/attachment.cgi?id=289289&action=review r=me with comments. > Source/JavaScriptCore/parser/Parser.cpp:3174 > + ExpressionErrorClassifier classifier(this); Is this necessary? parseAssignmentExpression(context) has ExpressionErrorClassifier inside it. > Source/JavaScriptCore/parser/Parser.cpp:3522 > + // AwaitExpression desugared to YieldExpression This comment is not necessary. > Source/JavaScriptCore/parser/Parser.h:320 > + } What is the purpose of this `isModule()`? Created attachment 289446 [details]
Async Function Parsing v6
Comment on attachment 289289 [details] Async Function Parsing v5 View in context: https://bugs.webkit.org/attachment.cgi?id=289289&action=review I've uploaded the patch instead of manually committing, I'd like someone to sign off on the changes to build-jsc, just in case it's not quite right (and I haven't tested the cmake build for it at all) >> Source/JavaScriptCore/parser/Parser.cpp:3174 >> + ExpressionErrorClassifier classifier(this); > > Is this necessary? parseAssignmentExpression(context) has ExpressionErrorClassifier inside it. Acknowledged >> Source/JavaScriptCore/parser/Parser.cpp:3522 >> + // AwaitExpression desugared to YieldExpression > > This comment is not necessary. Acknowledged >> Source/JavaScriptCore/parser/Parser.h:320 >> + } > > What is the purpose of this `isModule()`? m_moduleScopeData used to be a member of Scope, so this made sense back then. I've gotten rid of this method, and just check if the one in the parser is null or not instead. This seems to fix a bug where modules allow "await" as an identifier in nested functions (which was incorrect per https://tc39.github.io/ecma262/#prod-FutureReservedWord) Created attachment 289447 [details]
Async Function Parsing v6 + Copy-edit the build-jsc help message changes
Comment on attachment 289447 [details] Async Function Parsing v6 + Copy-edit the build-jsc help message changes Attachment 289447 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2118470 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-duration.html Created attachment 289449 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 289289 [details] Async Function Parsing v5 View in context: https://bugs.webkit.org/attachment.cgi?id=289289&action=review >>> Source/JavaScriptCore/parser/Parser.h:320 >>> + } >> >> What is the purpose of this `isModule()`? > > m_moduleScopeData used to be a member of Scope, so this made sense back then. I've gotten rid of this method, and just check if the one in the parser is null or not instead. > > This seems to fix a bug where modules allow "await" as an identifier in nested functions (which was incorrect per https://tc39.github.io/ecma262/#prod-FutureReservedWord) Nice. Make sense. Comment on attachment 289447 [details] Async Function Parsing v6 + Copy-edit the build-jsc help message changes View in context: https://bugs.webkit.org/attachment.cgi?id=289447&action=review r=me, with nits. > Source/JavaScriptCore/ChangeLog:87 > + * runtime/CommonIdentifiers.h: Could you update this ChangeLog by using Tools/Scripts/webkit-patch upload --update-changelogs? > Source/JavaScriptCore/parser/Parser.h:1560 > + // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module. Could you open a bug for this and note the URL here? And I think this can be easily fixed since we already have this information: JSParserCommentMode {Classic, Module}! This flag is named like this since currently it is only used for controlling comment mode. When you use this information here, you need to rename this mode to something different one (maybe, JSParserScriptMode?). > Source/JavaScriptCore/parser/Parser.h:1568 > + // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module. Ditto. > Tools/Scripts/build-jsc:211 > + if ($featureEnabled != $_->{default}) { For string, we need to use ne instead of !=. This causes warnings. Comment on attachment 289447 [details] Async Function Parsing v6 + Copy-edit the build-jsc help message changes View in context: https://bugs.webkit.org/attachment.cgi?id=289447&action=review >> Tools/Scripts/build-jsc:211 >> + if ($featureEnabled != $_->{default}) { > > For string, we need to use ne instead of !=. This causes warnings. Not just warnings, incorrect behavior! Using != causes it to do a numeric comparison, not a string one. For example, ("ON" != "OFF") is false because both strings have a numeric value of 0. Created attachment 289488 [details]
Patch
Created attachment 289489 [details]
Async Function Parsing v7
Comment on attachment 289489 [details] Async Function Parsing v7 View in context: https://bugs.webkit.org/attachment.cgi?id=289489&action=review > Tools/Scripts/build-jsc:206 > + if ($featureEnabled != $_->{default}) { ne Comment on attachment 289447 [details] Async Function Parsing v6 + Copy-edit the build-jsc help message changes View in context: https://bugs.webkit.org/attachment.cgi?id=289447&action=review >> Source/JavaScriptCore/ChangeLog:87 >> + * runtime/CommonIdentifiers.h: > > Could you update this ChangeLog by using Tools/Scripts/webkit-patch upload --update-changelogs? Done >> Source/JavaScriptCore/parser/Parser.h:1560 >> + // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module. > > Could you open a bug for this and note the URL here? > And I think this can be easily fixed since we already have this information: JSParserCommentMode {Classic, Module}! > This flag is named like this since currently it is only used for controlling comment mode. When you use this information here, you need to rename this mode to something different one (maybe, JSParserScriptMode?). For the uses in this patch, I think this actually doesn't matter here, because nested functions will never be re-parsed if `isDisallowedIdentifierAwait()` triggers an error (which it always should here). Maybe the FIXMEs are better to just be removed. >>> Tools/Scripts/build-jsc:211 >>> + if ($featureEnabled != $_->{default}) { >> >> For string, we need to use ne instead of !=. This causes warnings. > > Not just warnings, incorrect behavior! Using != causes it to do a numeric comparison, not a string one. For example, ("ON" != "OFF") is false because both strings have a numeric value of 0. Done --- Right, this was sloppily written entirely and has been fixed up. Note, the behaviour is different from build-webkit, but I don't think it should really matter. Thanks for the look and testing on cmake. Comment on attachment 289447 [details] Async Function Parsing v6 + Copy-edit the build-jsc help message changes View in context: https://bugs.webkit.org/attachment.cgi?id=289447&action=review >>>> Tools/Scripts/build-jsc:211 >>>> + if ($featureEnabled != $_->{default}) { >>> >>> For string, we need to use ne instead of !=. This causes warnings. >> >> Not just warnings, incorrect behavior! Using != causes it to do a numeric comparison, not a string one. For example, ("ON" != "OFF") is false because both strings have a numeric value of 0. > > Done --- Right, this was sloppily written entirely and has been fixed up. Note, the behaviour is different from build-webkit, but I don't think it should really matter. Thanks for the look and testing on cmake. Oops, thanks! Comment on attachment 289447 [details] Async Function Parsing v6 + Copy-edit the build-jsc help message changes View in context: https://bugs.webkit.org/attachment.cgi?id=289447&action=review >>> Source/JavaScriptCore/parser/Parser.h:1560 >>> + // FIXME: `m_moduleScopeData.get() != nullptr` is not a valid method of determining if parser is parsing a Module. >> >> Could you open a bug for this and note the URL here? >> And I think this can be easily fixed since we already have this information: JSParserCommentMode {Classic, Module}! >> This flag is named like this since currently it is only used for controlling comment mode. When you use this information here, you need to rename this mode to something different one (maybe, JSParserScriptMode?). > > For the uses in this patch, I think this actually doesn't matter here, because nested functions will never be re-parsed if `isDisallowedIdentifierAwait()` triggers an error (which it always should here). > > Maybe the FIXMEs are better to just be removed. Yeah, in this patch, it works and doesn't matter. But I think relying on this fact looks a bit dangerous since it is not this function that throws an actual syntax error. The name of this function is generic. So we may use this function for the other purpose. Consider the following case, if (!isDisallowedIdentifierAwait(xxx)) fail(); // pass In the above case, the first syntax checking succeeds, but the second actual parsing will fail. I think using JSParserScriptMode is less tricky. Created attachment 289499 [details]
Async Function Parsing v8
Comment on attachment 289499 [details]
Async Function Parsing v8
Updated with JSParserCommentMode renamed to JSParserScriptMode, m_commentMode renamed to m_scriptMode, and commentMode methods/variables renamed to scriptMode.
Attachment 289499 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 48 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 289501 [details]
Async Function Parsing v8 + fix ChangeLog
Created attachment 289502 [details]
Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods
Comment on attachment 289502 [details] Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods Attachment 289502 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2122226 New failing tests: fast/images/object-data-url-case-insensitivity.html Created attachment 289508 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 289502 [details] Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods View in context: https://bugs.webkit.org/attachment.cgi?id=289502&action=review > Source/JavaScriptCore/ChangeLog:87 > + * runtime/CommonIdentifiers.h: ChangeLog seems duplicated. Please drop the above one :) Comment on attachment 289502 [details] Async Function Parsing v8 + fix ChangeLog + finish renaming variables/methods Attachment 289502 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2122909 New failing tests: fast/images/move-image-to-new-document.html Created attachment 289519 [details]
Archive of layout-test-results from ews101 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 289680 [details]
Async Function Parsing v8 + rebased and given another EWS try
As requested, a followup run benchmark: ``` Baseline WithPatch closure 0.51188+-0.02331 0.50381+-0.01643 might be 1.0160x faster jquery 6.63200+-0.04502 ? 6.64141+-0.04629 ? <geometric> 1.82306+-0.02974 1.81769+-0.02420 might be 1.0030x faster ``` The WithPatch VM is built with --asyncfunction-syntax, indicating that even with these changes, it's still essentially a non-issue for the parser. New patch with fixed Source/JavaScriptCore/ChangeLog uploaded momentarily Created attachment 289706 [details]
Async Function Parsing v8
Created attachment 289707 [details]
Async Function Parsing v8
Created attachment 289708 [details]
Async Function Parsing v8
Created attachment 289710 [details]
Async Function Parsing v8
Comment on attachment 289710 [details]
Async Function Parsing v8
r=me
Comment on attachment 289710 [details] Async Function Parsing v8 Clearing flags on attachment: 289710 Committed r206333: <http://trac.webkit.org/changeset/206333> All reviewed patches have been landed. Closing bug. |