Now that we safepoint between each phase, we probably have bugs where we have references that never become known to the compiler. Let's harden how we do things to be consistent everywhere and hard to get wrong.
Created attachment 299450 [details] WIP It begins
Created attachment 299488 [details] WIP A bit more. I want to find a way where using TrustedImmPtr(T*) where T inherits from HeapCell is a compile error. I've finished this for FTLOutput with FTLOutput::constIntPtr.
Created attachment 299538 [details] WIP This now passes release JSC tests for me locally without running the structure registration phase. I want to read over my code and run debug tests before putting up for review. (Also, I think I probably caught a few legitimate, but probably almost impossible to reproduce bugs when doing this.)
Attachment 299538 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGPlan.cpp:314: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:28: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:47: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:48: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:221: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:221: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:89: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:90: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 9 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 299538 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=299538&action=review > Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:559 > + if (value.m_structure.isFinite()) > + m_graph.addStructureSet(value.m_structure.set()); Seems weird. Why don't we just have the guarantee that AbstractInterpreter registers structures before building sets out of them? I think that contract trivially works: the only structures that AbstractInterpreter can see are RegisteredStructures. > Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:589 > + > + if (value.m_structure.isFinite()) > + m_graph.addStructureSet(value.m_structure.set()); Ditto. > Source/JavaScriptCore/ftl/FTLOutput.h:126 > + > + LValue constGCPointer(DFG::Graph& graph, JSCell* cell) > + { > + ASSERT(graph.m_plan.weakReferences.contains(cell)); > + > + if (sizeof(void*) == 8) > + return constInt64(bitwise_cast<intptr_t>(cell)); > + return constInt32(bitwise_cast<intptr_t>(cell)); > + } > + > + LValue constGCPointer(DFG::FrozenValue* value) > + { > + RELEASE_ASSERT(value->value().isCell()); > + > + if (sizeof(void*) == 8) > + return constInt64(bitwise_cast<intptr_t>(value->cell())); > + return constInt32(bitwise_cast<intptr_t>(value->cell())); > + } > + Why can't this be called weakPointer?
This looks pretty good to me.
<rdar://problem/30179506>
Created attachment 299672 [details] patch
Attachment 299672 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGRegisteredStructure.cpp:39: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h:135: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h:234: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:28: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:47: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:221: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:221: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 9 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 299672 [details] patch Attachment 299672 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2944589 New failing tests: imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html
Created attachment 299679 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 299672 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=299672&action=review This looks pretty good to me. > Source/JavaScriptCore/dfg/DFGRegisteredStructure.cpp:37 > +void RegisteredStructureSet::filter(const DFG::StructureAbstractValue& other) Can you put RegisteredStructureSet into its own file? > Source/JavaScriptCore/dfg/DFGRegisteredStructure.h:84 > +class RegisteredStructureSet : public TinyPtrSet<RegisteredStructure> { Separate file > Source/WTF/wtf/TinyPtrSet.h:479 > - return static_cast<T>(pointer()); > + return bitwise_cast<T>(pointer()); Wow
This is a huge slowdown. I need to figure out why.
(In reply to comment #13) > This is a huge slowdown. I need to figure out why. Not sure why yet. Using concurrentJIT=0 and the super sampler under Graph::registerStructure, the super sampler says we're spending no time there. Maybe I mistranslated code somewhere.
reportTotalCompileTimes=true says we're spending way more time compiling. I'm not yet sure if this is because I mistranslated code and we do more compilations, or the compiler is just 6x more expensive with my patch.
(In reply to comment #15) > reportTotalCompileTimes=true says we're spending way more time compiling. > I'm not yet sure if this is because I mistranslated code and we do more > compilations, or the compiler is just 6x more expensive with my patch. Could be OSR exiting like crazy. Maybe some references that were previously treated as strong are now treated as weak, or you are failing to transform Transition into a CodeBlock Transition constraint. Easy way to test that theory is to count the number of jettisons during GC.
Ok, I've found the issue. An FTL CodeBlock had a weakPointer to itself, causing itself to get collected. I've fixed this bug. I'm now considering if we could even assert we never weakly point to a CodeBlock* just for sanity. Maybe I'll do this in a separate patch.
Created attachment 299847 [details] patch for landing
Comment on attachment 299847 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=299847&action=review > Source/JavaScriptCore/dfg/DFGGraph.cpp:1482 > + return; oops.
Attachment 299847 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:47: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 2 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 299849 [details] patch for landing
Attachment 299849 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:47: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 2 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 299851 [details] patch for landing
Attachment 299851 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:47: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 2 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 299852 [details] patch for landing
Created attachment 299853 [details] perf numbers
Attachment 299852 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:47: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 2 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 299852 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=299852&action=review I looked over the new code. Neat! > Source/JavaScriptCore/ChangeLog:12 > + the compiler must be track of all the heap pointers that are part "must be track of" > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:132 > + } You could make this a link error by declaring but not defining it. It's kinda ugly! You can make it more obvious with: void ErrorTrustedImmPtrCannotBeConstructedWithInt(); // Declared, not defined. explicit TrustedImmPtr(int) { ErrorTrustedImmPtrCannotBeConstructedWithInt(); }
Created attachment 299861 [details] patch for landing
(In reply to comment #28) > Comment on attachment 299852 [details] > patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299852&action=review > > I looked over the new code. Neat! > > > Source/JavaScriptCore/ChangeLog:12 > > + the compiler must be track of all the heap pointers that are part > > "must be track of" Fixed. > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:132 > > + } > > You could make this a link error by declaring but not defining it. It's > kinda ugly! You can make it more obvious with: > > void ErrorTrustedImmPtrCannotBeConstructedWithInt(); // Declared, not > defined. > explicit TrustedImmPtr(int) { > ErrorTrustedImmPtrCannotBeConstructedWithInt(); } I did just to try to keep refactoring levels down since MacroAssembler::TrustedImmPtr does this. It would be nice if we could eventually remove it. But I think some callers really want to say I have an pointer sized integer with value zero, and I suspect MacroAssembler::TrustedImmPtr does this to not confuse the compiler between void* and size_t when the immediate is "0".
Comment on attachment 299861 [details] patch for landing Clearing flags on attachment: 299861 Committed r211237: <http://trac.webkit.org/changeset/211237>
All reviewed patches have been landed. Closing bug.