[Webkit-unassigned] [Bug 133395] Web Inspector: debugger should be able to show variable types

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=133395


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #233658|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #20 from Filip Pizlo <fpizlo at apple.com>  2014-06-24 11:37:15 PST ---
(From update of attachment 233658)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.