Make it possible to start and stop the bytecode sampler.
Created attachment 224445 [details] Patch
Comment on attachment 224445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224445&action=review > Source/JavaScriptCore/ChangeLog:9 > + Refactor the bytecode sampler logic to allow us to start and stop > + profiling sessions dynamically. What is this for? > Source/JavaScriptCore/runtime/VM.cpp:419 > +void VM::initializeBytecodeProfiler() > +{ > + ASSERT(m_bytecodeProfilerEnabled); > + if (m_perBytecodeProfiler) > + return; > + > + m_perBytecodeProfiler = adoptPtr(new Profiler::Database(*this)); > +} > + > +void VM::startBytecodeProfiler() > +{ > + if (m_bytecodeProfilerEnabled) > + return; > + m_bytecodeProfilerEnabled = true; > + if (entryScope) > + entryScope->setRecompilationNeeded(true); > + else > + initializeBytecodeProfiler(); > + > +} Why are starting and initializing two different things? > Source/JavaScriptCore/runtime/VM.cpp:438 > +String VM::stopBytecodeProfiler() > +{ > + m_bytecodeProfilerEnabled = false; > + if (!m_perBytecodeProfiler) > + return String(); > + String result = m_perBytecodeProfiler->toJSON(); > + if (entryScope) > + entryScope->setRecompilationNeeded(true); > + else > + destroyBytecodeProfiler(); > + return result; > +} I really don't like this API. I think it's weird that stopping the profiler, and getting its JSON, are part of the same thing. Why can't you have two separate APIs? > Source/JavaScriptCore/runtime/VM.h:516 > + void prepareProfilerIfNecessary() > + { > + if (m_perBytecodeProfiler && !m_bytecodeProfilerEnabled) > + destroyBytecodeProfiler(); > + else if (m_bytecodeProfilerEnabled && !m_perBytecodeProfiler) > + initializeBytecodeProfiler(); > + } This is really weird. A method called "prapare" that destroys the profiler, or initializes it? I don't understand this method.
(In reply to comment #2) > (From update of attachment 224445 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224445&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + Refactor the bytecode sampler logic to allow us to start and stop > > + profiling sessions dynamically. > > What is this for? So we'll be able to profile pages in browser > > > Source/JavaScriptCore/runtime/VM.cpp:419 > > +void VM::initializeBytecodeProfiler() > > +{ > > + ASSERT(m_bytecodeProfilerEnabled); > > + if (m_perBytecodeProfiler) > > + return; > > + > > + m_perBytecodeProfiler = adoptPtr(new Profiler::Database(*this)); > > +} > > + > > +void VM::startBytecodeProfiler() > > +{ > > + if (m_bytecodeProfilerEnabled) > > + return; > > + m_bytecodeProfilerEnabled = true; > > + if (entryScope) > > + entryScope->setRecompilationNeeded(true); > > + else > > + initializeBytecodeProfiler(); > > + > > +} > > Why are starting and initializing two different things? So you can start the profiler from inside JS, i guess that's not strictly necessary but it was useful for testing. > > > Source/JavaScriptCore/runtime/VM.cpp:438 > > +String VM::stopBytecodeProfiler() > > +{ > > + m_bytecodeProfilerEnabled = false; > > + if (!m_perBytecodeProfiler) > > + return String(); > > + String result = m_perBytecodeProfiler->toJSON(); > > + if (entryScope) > > + entryScope->setRecompilationNeeded(true); > > + else > > + destroyBytecodeProfiler(); > > + return result; > > +} > > I really don't like this API. I think it's weird that stopping the profiler, and getting its JSON, are part of the same thing. Why can't you have two separate APIs? If we don't have the ability to start/stop from js this ins't necessary > > > Source/JavaScriptCore/runtime/VM.h:516 > > + void prepareProfilerIfNecessary() > > + { > > + if (m_perBytecodeProfiler && !m_bytecodeProfilerEnabled) > > + destroyBytecodeProfiler(); > > + else if (m_bytecodeProfilerEnabled && !m_perBytecodeProfiler) > > + initializeBytecodeProfiler(); > > + } > > This is really weird. A method called "prapare" that destroys the profiler, or initializes it? I don't understand this method.
Created attachment 224522 [details] Patch