[webkit-reviews] review denied: [Bug 135423] Allow high fidelity type profiling to be enabled and disabled. : [Attachment 236487] patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Saam Barati <sbarati at apple.com>'s
request for review:
Bug 135423: Allow high fidelity type profiling to be enabled and disabled.
https://bugs.webkit.org/show_bug.cgi?id=135423

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236487&action=review


Looks solid, but I think I spotted a bug or two, and this could use some
refinement before landing.

> Source/JavaScriptCore/ChangeLog:9
> +	   - Merged op_put_to_scope_with_profile and
op_get_from_scope_with_profile into
> +	     op_profile_types_with_high_fidelity by adding extra arguments to
the opcode.

Does "_with_high_fidelity" add anything here? We try to keep opcodes short, so
they can dump like assembly code. How about "op_profile_type"? That has nice
symmetry with our other "op_profile" opcodes.

> Source/JavaScriptCore/ChangeLog:15
> +	   - Refactored how entries are written to HighFidelityLog to make it
> +	     easier to inline when generating machine code.

Seems like this should be named something like "TypeLog". Fidelity is an
adjective -- we want a noun to name a thing.

> Source/JavaScriptCore/ChangeLog:21
> +	   * bytecode/BytecodeList.json:
> +	   * bytecode/BytecodeUseDef.h:

It's good to fill out this line-by-line template with details of why you did
something in a certain way, which might not be obvious to a reader.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1719
> +	   if (m_vm->isProfilingTypesWithHighFidelity()) {
> +	       ConcurrentJITLocker locker(symbolTable->m_lock);
> +	       symbolTable->prepareForHighFidelityTypeProfiling(locker);
> +	   }

Same comment here about "WithHighFidelity".

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2021
> +		       // This must be called because there is a chance we may
grab a Symbol Table that hasn't been prepared for profiling from an outer
scope.

Better than what in a comment is why: "If our parent scope was created while
profiling was disabled, it will not have prepared for profiling yet."

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2035
> +		   symbolTable = m_symbolTable.get();
>		   ConcurrentJITLocker locker(symbolTable->m_lock);
> -		   globalVariableID = symbolTable->uniqueIDForRegister(locker,
virtualRegister.offset(), *vm());
> -		   globalTypeSet =
symbolTable->globalTypeSetForRegister(locker, virtualRegister.offset(), *vm());

> +		   globalVariableID = symbolTable->uniqueIDForRegister(locker,
profileRegister.offset(), *vm());
> +		   globalTypeSet =
symbolTable->globalTypeSetForRegister(locker, profileRegister.offset(), *vm());

>		   break;

Why don't we need prepareForHighFidelityTypeProfiling here?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:124
> -    if (m_codeBlock->symbolTable())
> +    if (m_codeBlock->symbolTable()) {
> +	   if (m_codeBlock->vm()->isProfilingTypesWithHighFidelity()) {
> +	       ConcurrentJITLocker locker(m_codeBlock->symbolTable()->m_lock);
> +	      
m_codeBlock->symbolTable()->prepareForHighFidelityTypeProfiling(locker);
> +	   }
>	  
m_codeBlock->setSymbolTable(m_codeBlock->symbolTable()->cloneCapturedNames(*m_c
odeBlock->vm()));
> +    }

Why do we do this both during bytecode generation and during CodeBlock linking?
It seems like we should only need one or the other.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:141
> +    RegisterID* ret = generator.moveToDestinationIfNeeded(dst,
generator.thisRegister());

We usually call this "result" rather than "ret".

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:173
> +    RegisterID* ret = generator.emitGetFromScope(finalDest, scope.get(),
m_ident, ThrowIfNotFound);

Ditto.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:302
> +	       [=] (VM& vm, JSGlobalObject*) 
> +	       {

Brace should be on line above.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1387
> +    // Store the JSValue on the log entry.
> +    emitGetVirtualRegister(valueToProfile, regT0);
> +    store64(regT0, Address(regT1,
HighFidelityLog::LogEntry::valueOffset()));

I believe that emitGetVirtualRegister attempts to reuse regT0, without
reloading from the stack, if the last bytecode put its result in regT0. So, you
might sometimes get garbage results by using regT0 and then calling
emitGetVirtualRegister for regT0. Usually, we work around this by doing the
emitGetVirtualRegister as the first thing in the opcode.

> Source/JavaScriptCore/jit/JITOperations.cpp:1925
> +void JIT_OPERATION operationClearHighFidelityTypeProfilerLog(ExecState*
exec)

"clear" made me think that this function would delete the log. But then this
function called a function named "process", which seems to consume the log,
rather than just throwing it away. A good general rule is that if function A
just calls through to function B, they should not have substantially different
names. If they do, you have given two names to one thing, which defeats the
purpose of naming.

I'd call this "operationProcessTypeLog", and the function it calls
"exec->vm().typeLog().process()".