Harden how the compiler references GC objects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

Created attachment 299450 [details]
WIP

It begins

[Saam Barati]

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.

[Saam Barati]

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

[WebKit Commit Bot]

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.

[Filip Pizlo]

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?

[Filip Pizlo]

This looks pretty good to me.

[Radar WebKit Bug Importer]

<rdar://problem/30179506>

[Saam Barati]

Created attachment 299672 [details]
patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

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

[Saam Barati]

This is a huge slowdown. I need to figure out why.

[Saam Barati]

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

[Saam Barati]

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.

[Filip Pizlo]

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 299847 [details]
patch for landing

[Saam Barati]

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.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 299849 [details]
patch for landing

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 299851 [details]
patch for landing

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 299852 [details]
patch for landing

[Saam Barati]

Created attachment 299853 [details]
perf numbers

[WebKit Commit Bot]

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.

[JF Bastien]

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(); }

[Saam Barati]

Created attachment 299861 [details]
patch for landing

[Saam Barati]

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

[WebKit Commit Bot]

Comment on attachment 299861 [details]
patch for landing

Clearing flags on attachment: 299861

Committed r211237: <http://trac.webkit.org/changeset/211237>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.