Add super sampler begin and end bytecodes.
Created attachment 326219 [details] Patch
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.
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?
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.
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.
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.
Created attachment 326347 [details] Patch
Created attachment 326349 [details] Patch for landing
Comment on attachment 326349 [details] Patch for landing Clearing flags on attachment: 326349 Committed r224594: <https://trac.webkit.org/changeset/224594>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35562293>