Bug 179376 – Add super sampler begin and end bytecodes.

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Add super sampler begin and end bytecodes.

[Keith Miller]

Created attachment 326219 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 31 files


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

[Saam Barati]

Comment on attachment 326219 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4375
> +            {
> +                emitIteratorNext(value.get(), nextMethod.get(), iterator.get(), node, isForAwait ? EmitAwait::Yes : EmitAwait::No);
> +            }

why?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4544
> +            SuperSamplerBytecodeScope scope(*this);

Did you mean to keep these in? Maybe you want them to take an extra parameter like the SuperSamplerScope object so we can pass in true/false to actually emit the bytecode?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4563
> +    SuperSamplerBytecodeScope scope(*this);

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3515
> +    m_jit.load32(bitwise_cast<void*>(&g_superSamplerCount), reg);
> +    m_jit.add32(TrustedImm32(1), reg);
> +    m_jit.store32(reg, bitwise_cast<void*>(&g_superSamplerCount));

Isn't there a masm function that does this for you if you give it an address and it just uses the masm temp register?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3525
> +    m_jit.load32(bitwise_cast<void*>(&g_superSamplerCount), reg);
> +    m_jit.sub32(TrustedImm32(1), reg);
> +    m_jit.store32(reg, bitwise_cast<void*>(&g_superSamplerCount));

ditto

> Source/JavaScriptCore/jit/JITOpcodes.cpp:976
> +    load32(bitwise_cast<void*>(&g_superSamplerCount), regT0);
> +    add32(TrustedImm32(1), regT0);
> +    store32(regT0, bitwise_cast<void*>(&g_superSamplerCount));

ditto about masm helper

> Source/JavaScriptCore/jit/JITOpcodes.cpp:983
> +    load32(bitwise_cast<void*>(&g_superSamplerCount), regT0);
> +    sub32(TrustedImm32(1), regT0);
> +    store32(regT0, bitwise_cast<void*>(&g_superSamplerCount));

ditto

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1717
> +LLINT_SLOW_PATH_DECL(slow_path_super_sampler_begin)

We can't do this inline in LLInt code?

[Keith Miller]

Comment on attachment 326219 [details]
Patch

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

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1717
>> +LLINT_SLOW_PATH_DECL(slow_path_super_sampler_begin)
> 
> We can't do this inline in LLInt code?

The LLInt parser doesn't know how to handle global variables, or at least I wasn't easily able to get it to work. Also, you can't pass a address to our current constexpr code since there's no way to reinterpret_cast in a constexpr, AFAIK.

[Mark Lam]

Comment on attachment 326219 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4544
>> +            SuperSamplerBytecodeScope scope(*this);
> 
> Did you mean to keep these in? Maybe you want them to take an extra parameter like the SuperSamplerScope object so we can pass in true/false to actually emit the bytecode?

I don't think you should keep this in.  It makes no sense to sample this specific case and not something else, and it doesn't make sense to sprinkle this scope indiscriminately everywhere either.  So, I vote for taking this out.  We should only add these scopes in our local workspace when doing profiling work.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3515
>> +    m_jit.store32(reg, bitwise_cast<void*>(&g_superSamplerCount));
> 
> Isn't there a masm function that does this for you if you give it an address and it just uses the masm temp register?

You can use the operations that take an AbsoluteAddress as a dest.

[Keith Miller]

Comment on attachment 326219 [details]
Patch

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4544
>>> +            SuperSamplerBytecodeScope scope(*this);
>> 
>> Did you mean to keep these in? Maybe you want them to take an extra parameter like the SuperSamplerScope object so we can pass in true/false to actually emit the bytecode?
> 
> I don't think you should keep this in.  It makes no sense to sample this specific case and not something else, and it doesn't make sense to sprinkle this scope indiscriminately everywhere either.  So, I vote for taking this out.  We should only add these scopes in our local workspace when doing profiling work.

I was already planning on removing this code before putting the patch up for review so there's no worries there. I just uploaded this so Saam could take a look at my current patch.

[Keith Miller]

Created attachment 326347 [details]
Patch

[Keith Miller]

Created attachment 326349 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 326349 [details]
Patch for landing

Clearing flags on attachment: 326349

Committed r224594: <https://trac.webkit.org/changeset/224594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562293>