| Summary: | Web Inspector: ES6: Better support for Symbol types in Type Profiler | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | Web Inspector | Assignee: | Saam Barati <saam> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | graouts, joepeck, jonowells, mattbaker, nvasilyev, saam, timothy, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
Created attachment 248484 [details]
WIP
it begins
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. 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) Created attachment 248549 [details]
patch
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"? landed in: http://trac.webkit.org/changeset/182114 |
* 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.