[webkit-reviews] review denied: [Bug 128941] Make it possible to start and stop the bytecode sampler. : [Attachment 224445] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 128941: Make it possible to start and stop the bytecode sampler.
https://bugs.webkit.org/show_bug.cgi?id=128941

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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.