Bug (inspector_types) – Make improvements to Type Profiling

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

This diff almost completes the patch I want to submit to make significant improvements to the infrastructure around profiling types in JSC.

- We now know which functions have executed.
- Ranges have been made to be zero indexed, because this is simply how it should be.
- There are fewer extraneous methods that weren't needed.
- The inspector no longer receives a String blob from JSC, it now gets back structured data.
- Constructor names are now gathered for Objects

(There is also corresponding code in this current diff that shows some WebInspector JavaScript files that support displaying types, but this is there simply to show the start of code on that side, it will not make it into this patch. It will be made into a different patch under the WebInspector)

[Saam Barati]

Created attachment 234812 [details]
patch

(Description of this patch above ^)

[Saam Barati]

Created attachment 234890 [details]
patch

I improved the API between the inspector and JSC. We no longer send one huge
string to the inspector. We now send structured data that represents the type
information that JSC has collected. I've also created a beginning implementation 
of a type lattice that allows us to resolve a display name for a type that
consists of a single word.

I created a data structure that knows which functions have executed. This
solves the bug where types inside an un-executed function will resolve
to the type of the enclosing expression of that function. This data
structure may also be useful later if the inspector chooses to create a UI
around showing which functions have executed.

[Saam Barati]

Created attachment 234891 [details]
patch

I improved the API between the inspector and JSC. We no longer send one huge
string to the inspector. We now send structured data that represents the type
information that JSC has collected. I've also created a beginning implementation 
of a type lattice that allows us to resolve a display name for a type that
consists of a single word.

I created a data structure that knows which functions have executed. This
solves the bug where types inside an un-executed function will resolve
to the type of the enclosing expression of that function. This data
structure may also be useful later if the inspector chooses to create a UI
around showing which functions have executed.

[Saam Barati]

Created attachment 235172 [details]
patch

I improved the API between the inspector and JSC. We no longer send one huge
string to the inspector. We now send structured data that represents the type
information that JSC has collected. I've also created a beginning implementation 
of a type lattice that allows us to resolve a display name for a type that
consists of a single word.

I created a data structure that knows which functions have executed. This
solves the bug where types inside an un-executed function will resolve
to the type of the enclosing expression of that function. This data
structure may also be useful later if the inspector chooses to create a UI
around showing which functions have executed.

Better type information is gathered for objects. StructureShape now
represents an object's prototype chain.  StructureShape also collects
the constructor name for an object.

Expression ranges are now zero indexed.

Removed some extraneous methods.

[Saam Barati]

Created attachment 235326 [details]
patch 

I improved the API between the inspector and JSC. We no longer send one huge
string to the inspector. We now send structured data that represents the type
information that JSC has collected. I've also created a beginning implementation 
of a type lattice that allows us to resolve a display name for a type that
consists of a single word.

I created a data structure that knows which functions have executed. This
solves the bug where types inside an un-executed function will resolve
to the type of the enclosing expression of that function. This data
structure may also be useful later if the inspector chooses to create a UI
around showing which functions have executed.

Better type information is gathered for objects. StructureShape now
represents an object's prototype chain.  StructureShape also collects
the constructor name for an object.

Expression ranges are now zero indexed.

Removed some extraneous methods.

[Filip Pizlo]

Comment on attachment 235326 [details]
patch 

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

It's good modulo two small changes.  Can you file that bug, fix that indentation, and upload a new patch so that I can land it?

> Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.cpp:40
> +    TypeProfilerSearchDescriptor descriptor = location->m_globalVariableID == HighFidelityReturnStatement ? TypeProfilerSearchDescriptorFunctionReturn
> +                                              : location->m_globalVariableID == HighFidelityThisStatement ? TypeProfilerSearchDescriptorThisStatement
> +                                              : TypeProfilerSearchDescriptorNormal;

I think that you should fix the style here.  In WebKit, any line that is a continuation of the statement on the previous line should be indented just an extra four spaces.

> Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.cpp:131
> +    // FIXME: BestMatch should never be null past this point. This doesn't hold currently because we ignore some Eval/With/VarInjection variable assignments.

Can has bugzilla bug for this FIXME?  I think that every FIXME should have a bug.

[Saam Barati]

(In reply to comment #6)
> (From update of attachment 235326 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235326&action=review
> 
> It's good modulo two small changes.  Can you file that bug, fix that indentation, and upload a new patch so that I can land it?
> 
> > Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.cpp:40
> > +    TypeProfilerSearchDescriptor descriptor = location->m_globalVariableID == HighFidelityReturnStatement ? TypeProfilerSearchDescriptorFunctionReturn
> > +                                              : location->m_globalVariableID == HighFidelityThisStatement ? TypeProfilerSearchDescriptorThisStatement
> > +                                              : TypeProfilerSearchDescriptorNormal;
> 
> I think that you should fix the style here.  In WebKit, any line that is a continuation of the statement on the previous line should be indented just an extra four spaces.
> 
> > Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.cpp:131
> > +    // FIXME: BestMatch should never be null past this point. This doesn't hold currently because we ignore some Eval/With/VarInjection variable assignments.
> 
> Can has bugzilla bug for this FIXME?  I think that every FIXME should have a bug.
Agreed. I'll make one and resubmit.
I'll also fix the spacing issues to be WebKit style compliant.

[Saam Barati]

Created attachment 235332 [details]
patch

Made the above style compliant changes and created a new bug.

[Filip Pizlo]

Comment on attachment 235332 [details]
patch

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

I'll land this with the style changes.

> Source/JavaScriptCore/runtime/Executable.cpp:515
> +        if (vm.isProfilingTypesWithHighFidelity())
> +            vm.highFidelityTypeProfiler()->functionHasExecutedCache()->insertUnexecutedRange(sourceID(), 
> +                unlinkedFunctionExecutable->highFidelityTypeProfilingStartOffset(), 
> +                unlinkedFunctionExecutable->highFidelityTypeProfilingEndOffset());

This should have { } around the body of the if, since the body is more than one line.

> Source/JavaScriptCore/runtime/Structure.cpp:1083
> +                // FIXME: What is a custom property slot?

A custom property slot is something that cannot be created from JavaScript.  There are two kinds of them:

1) Someone inherits from JSObject and overrides getOwnPropertySlot so that it intercepts attempts to access certain properties, and then does "custom" things instead.  This allows you to create properties with magical semantics.  Array.length is an example of a custom property.  Note that custom properties will have behavior that is observably different from anything that you could create in JavaScript.

2) Custom getter property.  This is really just a getter, except that instead of the getter being a JavaScript function, it's a C function.  Note that custom getters are not observably any different from normal getters.  They are sort of a redundant feature since we can also have JavaScript functions that are backed by native code, so you could create the equivalent of a custom getter by having a normal getter with a JSFunction that has a NativeExecutable.

I will remove this comment.

> Source/JavaScriptCore/runtime/TypeLocationCache.h:46
> +                   && m_sourceID == other.m_sourceID
> +                   && m_start == other.m_start
> +                   && m_end == other.m_end;

Bad indentation.  Only four spaces, so:

return m_globalVariableID == other.m_globalVariableID
    && m_sourceID == other.m_sourceID
...

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/171510

[Mark Lam]

Landed missing build file changes in r172185: <http://trac.webkit.org/r172185>.