Bug 135423 – Allow high fidelity type profiling to be enabled and disabled.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

Currently, the VM has little flexibility in whether it enables/disables type profiling. 
It either enables or disables type profiling based on a runtime flag in JSC::Options. 
This patch will provide the necessary infrastructure that enables type profiling to be 
turned on and off dynamically. Once it can be enabled/disabled dynamically, the Inspector can 
present a UI for showing type profiling because it can call into JSC to enable/disable type profiling. 
This will allow us to integrate type profiling in the WebKit nightly builds sooner because  the 
various opcodes do not need to be fully implemented in all tiers of the JIT.

[Saam Barati]

Created attachment 235906 [details]
work in progress

- 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.
- Altered SymbolTable to use less memory by adding a rare data structure for type profiling.
- Created an interface to turn on and off type profiling.
- Refactored how I write to the log.
- deleted some unused code in HighFidelityLog.
- Implemented op_profile_types_with_high_fidelity in the baseline JIT by inlining the process of writing to the log and doing a small amount of type inference optimizations.

(This patch relies on https://bugs.webkit.org/show_bug.cgi?id=135358, therefore 135358 needs to land before this can land).

[Saam Barati]

Created attachment 235911 [details]
work in progress

same patch as above, but using JumpList to emit faster assembly.

[Saam Barati]

Created attachment 236403 [details]
patch

- 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.
- Altered SymbolTable to use less memory by adding a rare data structure for 
  type profiling.
- Created an interface to turn on and off type profiling from the Web
  Inspector.
- Refactored how entries are written to HighFidelityLog to make it
  easier to inline when generating machine code.
- Implemented op_profile_types_with_high_fidelity in the baseline JIT
  by inlining the process of writing to the log and doing a small amount
  of type inference optimizations.

[WebKit Commit Bot]

Attachment 236403 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:50:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:279:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:295:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JIT.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 236404 [details]
patch

Fixed webkit-style bugs from the previous patch

[Joseph Pecoraro]

Comment on attachment 236404 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236404&action=review

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:285
> +    // If JavaScript is running, it's not safe to recompile, since we'll end
> +    // up throwing away code that is live on the stack.
> +    if (vm.entryScope) {
> +        vm.entryScope->setEntryScopeDidPopListener(this,
> +            [] (VM& vm, JSGlobalObject*) 
> +            {
> +                recompileAllJSFunctionsForTypeProfiling(vm, true); 
> +            }
> +        );
> +    } else
> +        recompileAllJSFunctionsForTypeProfiling(vm, true);

This code is duplicated in enable/disable, it could move to a helper to avoid redundancy. Perhaps: setHighFidelityTypeProfilingEnabled(bool).

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:288
> +void InspectorRuntimeAgent::disableHighFidelityTypeProfiling(ErrorString*)

We should clear this state when the inspector disconnects so as to leave the VM in a pristine state. I would suggest:

  1. Give RuntimeAgent a member bool that says if this is enabled / disabled
  2. RuntimeAgent::willDestroyFrontendAndBackend should call in some was call disableHighFidelityTypeProfiling if if this was enabled and the destroy reason is NOT InspectorDisconnectReason::InspectedTargetDestroyed (as there is no need to recompile in this case)

> Source/JavaScriptCore/runtime/VM.cpp:865
> +void VM::enableHighFidelityTypeProfiling()
> +{
> +    if (isProfilingTypesWithHighFidelity())
> +        return;
> +
> +    m_highFidelityTypeProfiler = std::make_unique<HighFidelityTypeProfiler>();
> +    m_highFidelityLog = std::make_unique<HighFidelityLog>();
> +}

I think this needs to use a counter. Take the case of inspecting two different JSContext objects that share the same JSVirtualMachine. If you inspect both, enable type profiling on both, then disable on one, it will in an unbalanced way disable for the entire VM even though it should still be needed by a JSContext in the VM.

[Joseph Pecoraro]

(In reply to comment #6)
>   2. RuntimeAgent::willDestroyFrontendAndBackend should call in some was call disableHighFidelityTypeProfiling if if this was enabled and the destroy reason is NOT InspectorDisconnectReason::InspectedTargetDestroyed (as there is no need to recompile in this case)

Clarification:

  2. RuntimeAgent:: willDestroyFrontendAndBackend should call disableHighFidelityTypeProfiling if type profiling was enabled and the destroy reason is NOT InspectedTargetDestroyed.

[Saam Barati]

(In reply to comment #7)
> (In reply to comment #6)
> >   2. RuntimeAgent::willDestroyFrontendAndBackend should call in some was call disableHighFidelityTypeProfiling if if this was enabled and the destroy reason is NOT InspectorDisconnectReason::InspectedTargetDestroyed (as there is no need to recompile in this case)
> 
> Clarification:
> 
>   2. RuntimeAgent:: willDestroyFrontendAndBackend should call disableHighFidelityTypeProfiling if type profiling was enabled and the destroy reason is NOT InspectedTargetDestroyed.

Okay. This is what I've done. I've also had all the subclasses on InspectorRuntimeAgent call into InspecorRuntimeAgent::willDestroyFrontendAndBackend. Do you think that's a good approach?

Thanks for the other fedback too, I'll implement those changes.

[Saam Barati]

Created attachment 236421 [details]
patch

Took Joe's suggestions into account.

[WebKit Commit Bot]

Attachment 236421 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 236487 [details]
patch

Fixed style changes from previous patch.

[Geoffrey Garen]

Comment on attachment 236487 [details]
patch

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_codeBlock->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()".

[Brian Burg]

Comment on attachment 236487 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236487&action=review

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1995
> +            // The format of this instruciton is: op_profile_types_with_high_fidelity regToProfile, TypeLocation*, flag, identifier?, resolveType?

typo: instruction. But I think you can just mimic the other cases and drop the sentence fragment altogether.

> Source/WebCore/ChangeLog:8
> +        This patch has various subclasses of InspectorRuntimeAgent call their

I can see that. But why is this necessary (apropos the rest of the patch)? That is what you should write here.

> Source/WebCore/ChangeLog:11
> +        This just calls a super class's method, no need to test it.

You can omit this comment.

[Saam Barati]

Comment on attachment 236487 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236487&action=review

>> Source/JavaScriptCore/ChangeLog:21
>> +        * 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.

Do you mean to do this when submitting a patch for review?

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1995
>> +            // The format of this instruciton is: op_profile_types_with_high_fidelity regToProfile, TypeLocation*, flag, identifier?, resolveType?
> 
> typo: instruction. But I think you can just mimic the other cases and drop the sentence fragment altogether.

I think I did this because Fil advocated for comments being full sentences, but I'm not against mimicking the other instances of these comments.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2035
>>                  break;
> 
> Why don't we need prepareForHighFidelityTypeProfiling here?

Because we do  prepareForHighFidelityTypeProfiling for this CodeBlock's symbolTable at the beginning of the top of this function, that way we don't repeat calls for this CodeBlock's symbolTable.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:124
>> +    }
> 
> Why do we do this both during bytecode generation and during CodeBlock linking? It seems like we should only need one or the other.

Indeed. We can probably just do it during bytecode generation time.
I also think we can get away with cloning captured names in the symbol table just once as well.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:141
>> +    RegisterID* ret = generator.moveToDestinationIfNeeded(dst, generator.thisRegister());
> 
> We usually call this "result" rather than "ret".

Okay. Should I go through an retroactively fix the other places where I used ret?

>> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:302
>> +            {
> 
> Brace should be on line above.

check-webkit-style complains about lambda's having their brace on the line they're declared on. Should I do it anyway?

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:1387
>> +    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.

Okay, this is good to know, thanks.

>> 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()".

I agree, this is badly named.

[Saam Barati]

Comment on attachment 236487 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236487&action=review

>> Source/JavaScriptCore/ChangeLog:9
>> +          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.

Will land renaming all HighFidelity__ in this patch:
https://bugs.webkit.org/show_bug.cgi?id=135899

[Saam Barati]

Comment on attachment 236487 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236487&action=review

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:124
>>> +    }
>> 
>> Why do we do this both during bytecode generation and during CodeBlock linking? It seems like we should only need one or the other.
> 
> Indeed. We can probably just do it during bytecode generation time.
> I also think we can get away with cloning captured names in the symbol table just once as well.

I wrote this before we spoke in person.

[Saam Barati]

Created attachment 236577 [details]
patch

Took Geoff and Brian's recommendations into account.

[WebKit Commit Bot]

Attachment 236577 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:301:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 236577 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236577&action=review

Inspector parts look good to me now!

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1135
> +    instructions().append(0);

Nit: nullptr instead of 0?

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:260
> +    bool needsToRecompile;
> +    if (shouldEnableTypeProfiling)
> +        needsToRecompile = vm.enableHighFidelityTypeProfiling();
> +    else
> +        needsToRecompile = vm.disableHighFidelityTypeProfiling();

Nit: You can one line this with a ternary to avoid the awkward uninitialized bool.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:271
> +    if (reason == InspectorDisconnectReason::InspectorDestroyed && m_isTypeProfilingEnabled)

Nit: I think it might be clearer if this was: "reason != InspectorDisconnectReason::InspectedTargetDestroyed". Since that is at a high level the case we really care about here.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:278
> +    if (m_isTypeProfilingEnabled)
> +        return;

Nit: This check can go inside setHighFidelityTypeProfilingEnabledState.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:286
> +    if (!m_isTypeProfilingEnabled)
> +        return;

Nit: This check can go inside setHighFidelityTypeProfilingEnabledState.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.h:91
> +    void setHighFidelityTypeProfilingEnabledState(bool);

Style: Throw a newline between the private function and member variables.

> Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.cpp:55
> +    InspectorRuntimeAgent::willDestroyFrontendAndBackend(reason);

Style: Put a newline before this as these are really separate things.

> Source/JavaScriptCore/jit/JIT.cpp:506
> +        m_vm->highFidelityLog()->processHighFidelityLog("Preparing for JIT compilation.");

Nit: Since this is a string literal being converted into a WTF::String you could use ASCIILiteral:
<http://trac.webkit.org/wiki/EfficientStrings>

    processHighFidelityLog(ASCIILiteral("Preparing for JIT compilation."));

Likewise in other places.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1395
> +    // Incremement the current log entry.

Typo: "Incremement" => "Increment"

> Source/JavaScriptCore/runtime/VM.cpp:903
> +    for (Bag<TypeLocation>::iterator iter = m_typeLocationInfo->begin(); !!iter; ++iter) {

Isn't just "iter" enough instead of "!!iter"?

[Saam Barati]

Comment on attachment 236577 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=236577&action=review

Thanks for the feedback, other changes taken into account.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1135
>> +    instructions().append(0);
> 
> Nit: nullptr instead of 0?

I've chosen 0 to be consistent with the other code in this file.

>> Source/JavaScriptCore/runtime/VM.cpp:903
>> +    for (Bag<TypeLocation>::iterator iter = m_typeLocationInfo->begin(); !!iter; ++iter) {
> 
> Isn't just "iter" enough instead of "!!iter"?

I think !!iter is used when iterating over a Bag<T> because that class implements operator !

[Saam Barati]

Created attachment 236605 [details]
patch

Joe's comments taken into account.

[WebKit Commit Bot]

Attachment 236605 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:293:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 236605 [details]
patch

r=me, with renaming in a separate patch.

[WebKit Commit Bot]

Comment on attachment 236605 [details]
patch

Clearing flags on attachment: 236605

Committed r172614: <http://trac.webkit.org/changeset/172614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.