Bug 136161 – Create tests for type profiling

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

Right now, there is no test coverage for type profiling. Fix this.

[Saam Barati]

Created attachment 237095 [details]
work in progress

What do you guys think of the API for handling testing types?

[Saam Barati]

Created attachment 237096 [details]
work in progress

(Forgot actual test js files).

[Geoffrey Garen]

Comment on attachment 237096 [details]
work in progress

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

General approach looks good. I have some comments on the specifics.

> Source/JavaScriptCore/jsc.cpp:633
> +        addFunction(vm, "typeJSONStringForExpressionAtOffset", functionTypeJSONStringForExpressionAtOffset, 3);

Based on this name, I expected that the caller would need to pass in an offset, which is not true.

I think a better name would be "typeForExpression" or "findTypeForExpression" (connoting text search). Another option would be one function named "offsetOfExpression", which took a function and an expression and returned the text offset of the expression in the function, and another function named "typeAtOffset", which took a text offset and returned a type descriptor.

Putting the return type in the function name makes it a bit wordy. It's hard to get all types into a name, so I usually leave them out. For a broader exploration of how putting types in names can make life unhappy, you can read about so-called Hungarian Notation at Microsoft -- or you can read the example code @ <http://msdn.microsoft.com/en-us/library/aa260976(v=vs.60).aspx>, and then gouge your eyes out.

> Source/JavaScriptCore/jsc.cpp:1089
> +    JSValue descriptorValue = exec->argument(2); 
> +    ASSERT(descriptorValue.isMachineInt());
> +    TypeProfilerSearchDescriptor descriptor = static_cast<TypeProfilerSearchDescriptor>(descriptorValue.toUInt32(exec));
> +    ASSERT(descriptor == TypeProfilerSearchDescriptorNormal || descriptor == TypeProfilerSearchDescriptorFunctionReturn);

Can we make this descriptor a string or a global constant? A secret number is a difficult API to understand when reading these tests, or writing new tests.

It helps that you wrote out these constants in all of your tests, but I think it would be even better to make that step unnecessary.

> Source/JavaScriptCore/tests/profiler/type-profiler-basic.js:38
> +// Types matching those in runtime/TypeSet
> +var T = {
> +    Boolean:"Boolean",
> +    Integer: "Integer",
> +    Null: "Null",
> +    Number: "Number",
> +    Many: "(many)",
> +    String: "String",
> +    Undefined: "Undefined",
> +    UndefinedOrNull: "(?)"
> +};
> +
> +// Optional types matching those in runtime/TypeSet
> +var TOptional = {
> +    Boolean:"Boolean?",
> +    Integer: "Integer?",
> +    Number: "Number?",
> +    String: "String?"
> +};

Similar comment about these: It would be nice if they were exported by the engine into the testing environment, instead of being copied into each test. If you ever change these strings inside the engine, it will be a pain to change all the tests.

You can either export these as constants, or you can export a function like "displayTypeNameFor()", which takes a list of JSValues, and returns the display type they would produce.

[Saam Barati]

Comment on attachment 237096 [details]
work in progress

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

>> Source/JavaScriptCore/jsc.cpp:633
>> +        addFunction(vm, "typeJSONStringForExpressionAtOffset", functionTypeJSONStringForExpressionAtOffset, 3);
> 
> Based on this name, I expected that the caller would need to pass in an offset, which is not true.
> 
> I think a better name would be "typeForExpression" or "findTypeForExpression" (connoting text search). Another option would be one function named "offsetOfExpression", which took a function and an expression and returned the text offset of the expression in the function, and another function named "typeAtOffset", which took a text offset and returned a type descriptor.
> 
> Putting the return type in the function name makes it a bit wordy. It's hard to get all types into a name, so I usually leave them out. For a broader exploration of how putting types in names can make life unhappy, you can read about so-called Hungarian Notation at Microsoft -- or you can read the example code @ <http://msdn.microsoft.com/en-us/library/aa260976(v=vs.60).aspx>, and then gouge your eyes out.

It is wordy indeed. I forgot to change this function's name after I removed text offsets from the API.
I like the name "findTypeForExpression"

>> Source/JavaScriptCore/jsc.cpp:1089
>> +    ASSERT(descriptor == TypeProfilerSearchDescriptorNormal || descriptor == TypeProfilerSearchDescriptorFunctionReturn);
> 
> Can we make this descriptor a string or a global constant? A secret number is a difficult API to understand when reading these tests, or writing new tests.
> 
> It helps that you wrote out these constants in all of your tests, but I think it would be even better to make that step unnecessary.

I agree, this is the crappiest part about this API and the API from the WebInspector.

I think the solution for these tests is to expose two APIs: 

findTypeForExpression( <function>, <string>)
and
returnTypeForFunction(<function>) or returnTypeFor(<function>)

[Saam Barati]

Created attachment 237121 [details]
work in progress

What do you guys think about this patch?

Also, do you think the inline JSON building should be changed? Or maybe abstracted into some kind of JSON builder class?

What do you guys think about how I edited run-javascriptcore-tests and run-jsc-stress-tests to include the profiler tests?

[Saam Barati]

Created attachment 237537 [details]
patch

[Filip Pizlo]

Comment on attachment 237537 [details]
patch

Any reason why the new functions return a string that needs to be JSON parsed instead of just returning the object (that would come out of JSON parsing)?  I sort of understand having the internal APIs work with strings but it seems that the glue in JSC.cpp could just do the parsing for you.

[Saam Barati]

(In reply to comment #7)
> (From update of attachment 237537 [details])
> Any reason why the new functions return a string that needs to be JSON parsed instead of just returning the object (that would come out of JSON parsing)?  I sort of understand having the internal APIs work with strings but it seems that the glue in JSC.cpp could just do the parsing for you.

There isn't a good reason, it's just the way I started out. 
I should make JSC.cpp in charge of doing the parsing.

[Saam Barati]

Created attachment 237569 [details]
patch

Now doing JSON parsing inside jsc.cpp

[Geoffrey Garen]

Comment on attachment 237569 [details]
patch

r=me

[Saam Barati]

landed in: http://trac.webkit.org/changeset/173225