Bug 141257 – Web Inspector: ES6: Better support for Symbol types in Type Profiler

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
We should better support for Symbol types in Type Profiler. Symbols are pseudo-primitives, like unique strings. I would expect them to be handled similar to Strings in the Type profiling.

* TEST
<script>
function foo(a) {
  console.log(a);
}
</script>

js> foo(Symbol()); // I expect "Symbol" to show up next to foo's `a` param.
js> foo(1); // I expect "(many)" to show up next to foo's `a` param. Popover should have "Integer" and "Symbol".

* NOTES
- Symbol has some special properties. But it is more like a primitive than an object.

[Radar WebKit Bug Importer]

<rdar://problem/19719214>

[Saam Barati]

Created attachment 248484 [details]
WIP

it begins

[Joseph Pecoraro]

Comment on attachment 248484 [details]
WIP

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

Yeah, this looks good!

> Source/JavaScriptCore/runtime/TypeSet.cpp:77
> +    if (structure && newShape && (type & (TypeString | TypeSymbol) == TypeNothing)) {

Why are String|Symbol special but boolean and number aren't?

> Source/JavaScriptCore/runtime/TypeSet.cpp:139
> +    if (m_seenTypes & TypeSymbol)
> +         seen.append("Symbol ");

These can all be appendLiteral.

[Saam Barati]

Comment on attachment 248484 [details]
WIP

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

Just checked out my local build. Looks like this patch works. Gonna run the tests I have now to make sure they work and also write a few more.

>> Source/JavaScriptCore/runtime/TypeSet.cpp:77
>> +    if (structure && newShape && (type & (TypeString | TypeSymbol) == TypeNothing)) {
> 
> Why are String|Symbol special but boolean and number aren't?

Because Symbol and String are the two primitives that are also JSCells.
It may be worth writing a function that checks if a RuntimeType bit mask is a primitive, something like:
bool isPrimitive(uint16_t types) { return types & ~(TypeFunction | TypeObject) }
and the above line can just be
... && !isPrimitive(type)

[Saam Barati]

Created attachment 248549 [details]
patch

[Joseph Pecoraro]

Comment on attachment 248549 [details]
patch

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

Nice! r=me, some nits you can choose to fix if you want to.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1183
> -            uint8_t seenTypes = typeSet->seenTypes();
> +            uint16_t seenTypes = typeSet->seenTypes();

Instead of "uint16_t" can this just be "RuntimeType"?

> Source/JavaScriptCore/runtime/TypeSet.cpp:2
> - * Copyright (C) 2008, 2014 Apple Inc. All Rights Reserved.
> + * Copyright (C) 2014, 2015 Apple Inc. All Rights Reserved.

lol

> Source/JavaScriptCore/runtime/TypeSet.cpp:123
> +        seen.append("Function ");

Since you're updating each of these lines anyways, these should be "appendLiteral" instead of just "append".

> Source/JavaScriptCore/runtime/TypeSet.cpp:164
> -bool TypeSet::doesTypeConformTo(uint8_t test) const
> +bool TypeSet::doesTypeConformTo(uint16_t test) const

Instead of "uint16_t" can this just be "RuntimeType"?

> Source/JavaScriptCore/runtime/TypeSet.cpp:327
> +            json.append(",");

Nit: All of these appends with 1 character can/should be append(',').

> Source/JavaScriptCore/runtime/TypeSet.cpp:329
> +        json.append("\"Symbol\"");

Nit: This can/should be appendLiteral.

> Source/JavaScriptCore/runtime/TypeSet.h:113
> +    bool doesTypeConformTo(uint16_t test) const;
> +    uint16_t seenTypes() const { return m_seenTypes; }

Same here about "uint16_t" <=> "RuntimeType"?

> Source/JavaScriptCore/runtime/TypeSet.h:117
> +    uint16_t m_seenTypes;

Same here about "uint16_t" <=> "RuntimeType"?

[Saam Barati]

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