Bug 21735 – Emit profiling instrumentation only if the Web Inspector's profiling feature is enabled

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Geoffrey Garen]

Patch coming.

[Geoffrey Garen]

Created attachment 24493 [details]
patch

[Timothy Hatcher]

Comment on attachment 24493 [details]
patch

             // Profiler check
             emitGetCTIParam(CTI_ARGS_profilerReference, X86::eax);

You should remove that comment too.

Maybe recompileAllJSFunctionsSoon() should be on some other class or standalone function? Or moved to JSCore? It seems weird for it to be on the JavaScriptDebugServer now that it is used be the profiler.

[Geoffrey Garen]

(In reply to comment #2)
> (From update of attachment 24493 [details] [edit])
>              // Profiler check
>              emitGetCTIParam(CTI_ARGS_profilerReference, X86::eax);
> 
> You should remove that comment too.

Fixed!

> Maybe recompileAllJSFunctionsSoon() should be on some other class or standalone
> function? Or moved to JSCore? It seems weird for it to be on the
> JavaScriptDebugServer now that it is used be the profiler.

recompileAllJSFunctionsSoon() can't move to JSCore unless we move the Timer class down to WTF, too.

It can't be standalone unless we rework the Timer class to support standalone functions in addition to member functions.

Maybe these changes would be worth it, though.

Ideally, I think we'd use a shared object like GCController. JavaScriptDebugServer seemed like that object to me. It's not the profiler, but it makes sense to me that it knows how to recompile things, and that the profiler / Settings object can use it for that purpose.

But maybe I should just split off a new object -- CompilerController?

[Geoffrey Garen]

Created attachment 24496 [details]
patch - removed the comment tim mentioned

Updated patch to remove the comment Tim mentioned.

[Darin Adler]

Comment on attachment 24496 [details]
patch - removed the comment tim mentioned

r=me

[Timothy Hatcher]

Geoff landed this in r37730.

http://trac.webkit.org/changeset/37730

[Cameron Zwarich (cpst)]

*** Bug 21296 has been marked as a duplicate of this bug. ***