Bug 128941 – Make it possible to start and stop the bytecode sampler.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Oliver Hunt]

Make it possible to start and stop the bytecode sampler.

[Oliver Hunt]

Created attachment 224445 [details]
Patch

[Filip Pizlo]

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.

[Oliver Hunt]

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

[Oliver Hunt]

Created attachment 224522 [details]
Patch