[webkit-reviews] review denied: [Bug 133395] Web Inspector: debugger should be able to show variable types : [Attachment 233658] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Saam Barati <sbarati at apple.com>'s
request for review:
Bug 133395: Web Inspector: debugger should be able to show variable types
https://bugs.webkit.org/show_bug.cgi?id=133395

Attachment 233658: patch
https://bugs.webkit.org/attachment.cgi?id=233658&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233658&action=review


I think I found a bug (std::unique_ptr in TypeLocation for the global set
thing) and some obvious performance goofs (the log being an array of
Log3Triple*'s instead of Log3Triples, and the use of append() instead of
StringBuilder).

> Source/JavaScriptCore/ChangeLog:8
> +	   Increase the amount of type information the VM gathers when directed

> +	   to do so. This initial commit is working towards the goal of
> +	   capturing, and then showing (via the Web Inspector) type information
for all
> +	   assignment and load operations. This patch doesn't have the feature
fully 
> +	   implemented, but it ensures the VM has no performance regressions
> +	   unless the feature is specifically turned on.

Up here you should just put the bug title.

> Source/JavaScriptCore/ChangeLog:13
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * JavaScriptCore.xcodeproj/project.pbxproj:

In between the "Reviewed by..." and the list of changed files, you should put
the text that you have up above.

> Source/JavaScriptCore/bytecode/TypeLocation.h:52
> +    std::unique_ptr<TypeSet> m_globalTypeSet;

Should this *really* be a unique_ptr?  Is it ever possible for the thing that
globalTypeSet points to to outlive the TypeLocation?  Is it ever possible for
other things to point at the TypeSet pointed to by m_globalTypeSet?  If you
said "yes" to either question, then this shouldn't be a unique_ptr.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:194
> +#define LLINT_PUT_TO_SCOPE_COMMON() \
> +    LLINT_BEGIN(); \
> +    CodeBlock* codeBlock = exec->codeBlock(); \
> +    const Identifier& ident = codeBlock->identifier(pc[2].u.operand); \
> +    JSObject* scope = jsCast<JSObject*>(LLINT_OP(1).jsValue()); \
> +    JSValue value = LLINT_OP_C(3).jsValue(); \
> +    ResolveModeAndType modeAndType = ResolveModeAndType(pc[4].u.operand); \
> + \
> +    if (modeAndType.mode() == ThrowIfNotFound && !scope->hasProperty(exec,
ident)) \
> +	   LLINT_THROW(createUndefinedVariableError(exec, ident)); \
> + \
> +    PutPropertySlot slot(scope, codeBlock->isStrictMode()); \
> +    scope->methodTable()->put(scope, exec, ident, value, slot); \
> + \
> +    /* Covers implicit globals. Since they don't exist until they first
execute, we didn't know how to cache them at compile time.*/  \
> +    if (modeAndType.type() == GlobalProperty || modeAndType.type() ==
GlobalPropertyWithVarInjectionChecks) { \
> +	   if (slot.isCacheablePut() && slot.base() == scope &&
scope->structure()->propertyAccessesAreCacheable()) { \
> +	       ConcurrentJITLocker locker(codeBlock->m_lock); \
> +	       pc[5].u.structure.set(exec->vm(), codeBlock->ownerExecutable(),
scope->structure()); \
> +	       pc[6].u.operand = slot.cachedOffset(); \
> +	   } \
> +    } \
> +

Does this really have to be a macro?  Could this be a helper function?

> Source/JavaScriptCore/runtime/HighFidelityLog.cpp:46
> +    m_logStartPtr = (Log3Tuple**) malloc(sizeof(Log3Tuple*) *
m_highFidelityLogSize);
> +    m_nextBuffer = (Log3Tuple**) malloc(sizeof(Log3Tuple*) *
m_highFidelityLogSize);

Don't use malloc().  At a minimum, use fastMalloc().  Even better yet, use C++
allocation, like new Log3Tuple[m_highFidelityLogSize].

I also don't agree with your decision to use an array of pointers to a bunch of
Log3Tuple instances.  That's just an extra unnecessary indirection.  You could
instead just have an array of Log3Tuples (i.e. a Log3Tuple* instead of
Log3Tuple**).  This will save memory (fewer pointers and less allocator
per-object overhead) and execution time (no need for an extra load to get to
the actual Log3Tuple).

Finally, wouldn't it be better to say LogTriple or LogEntry instead of
Log3Tuple?

> Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.cpp:53
> +    auto iter2 = m_globalIDMap.find(iter->value);
> +    auto end2 = m_globalIDMap.end();
> +    if (iter2 == end2)

You should come up with better names than iter2 and end2.

> Source/JavaScriptCore/runtime/HighFidelityTypeProfiler.h:43
> +    WTF::String getTypesForVariableInRange(unsigned startLine, unsigned
startColumn, unsigned endLine, unsigned endColumn, const WTF::String&
variableName, intptr_t sourceID);
> +    WTF::String getGlobalTypesForVariableInRange(unsigned startLine,
unsigned startColumn, unsigned endLine, unsigned endColumn, const WTF::String&
variableName, intptr_t sourceID);
> +    WTF::String getLocalTypesForVariableInRange(unsigned startLine, unsigned
startColumn, unsigned endLine, unsigned endColumn, const WTF::String&
variableName, intptr_t sourceID);
> +    void insertNewLocation(TypeLocation*);

No need to say WTF::String.  You can just say String.

> Source/JavaScriptCore/runtime/TypeSet.cpp:179
> +    m_propertyHash = std::make_unique<String>();
> +    m_propertyHash->append(":");
> +    for (auto iter = m_fields.begin(), end = m_fields.end(); iter != end;
++iter) {
> +	   String property = String(iter->key);
> +	   property.replace(":", "\\:"); // Ensure that hash({"foo:", "bar"})
!= hash({"foo", ":bar"}) because we're using colons as a separator and colons
are legal characters in field names in JS.
> +	   m_propertyHash->append(property);
> +    }

I suspect that you should use StringBuilder here.

> Source/JavaScriptCore/runtime/TypeSet.cpp:209
> +    String lub("");
> +
> +    if (!shapes->size())
> +	   return lub;
> +	
> +    RefPtr<StructureShape> origin = shapes->at(0);
> +    lub.append("{");
> +    for (auto iter = origin->m_fields.begin(), end = origin->m_fields.end();
iter != end; ++iter) {
> +	   bool shouldAdd = true;
> +	   for (size_t i = 1, size = shapes->size(); i < size; i++) {
> +	       // If all other Shapes have the same field as origin, add it to
the least upper bound.
> +	       if (!shapes->at(i)->m_fields.contains(iter->key)) {
> +		   shouldAdd = false;
> +		   break;
> +	       }
> +	   }
> +	   if (shouldAdd)
> +	       lub.append(String(iter->key.get()) + String(", "));
> +    }
> +
> +    if (lub.length() >= 3)
> +	   lub = lub.left(lub.length() - 2); // Remove the trailing ', '
> +
> +    lub.append("}");

StringBuilder again, please.