[webkit-reviews] review denied: [Bug 133395] Web Inspector: debugger should be able to show variable types : [Attachment 233658] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 24 11:36:54 PDT 2014
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.
More information about the webkit-reviews
mailing list