Bug 127566

Summary: fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, ggaren, joepeck, mark.lam, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Naive Approach - Does not work running multiple tests
none
[PATCH] Proposed Fix - Let the VM manage recompilation
oliver: review-
[PATCH] Proposed Fix - Let VM manage recompilation oliver: review+

Description Joseph Pecoraro 2014-01-24 10:39:31 PST
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162690%20(2192)/results.html

Another internal method causing recompilation within a JS stack:

    window.internals.setJavaScriptProfilingEnabled(bool)

This Enables/Disables the Profiler which now recompiles synchronously within a JS stack. I can add a timer here in window.internals and do this after a loop. But its looking more and more like it might be better if the VM handled recompiling when appropriate instead of us trying to avoid calling it within a JS stack.
Comment 1 Geoffrey Garen 2014-01-24 11:19:51 PST
OK.
Comment 2 Joseph Pecoraro 2014-01-24 11:35:49 PST
Created attachment 222128 [details]
[PATCH] Naive Approach - Does not work running multiple tests

The naive approach of just making window.internals.setJavaScriptProfilerEnabled did not work.

    shell> Tools/Scripts/run-webkit-tests --debug fast/profiler/anonymous-event-handler.html
    PASS

    shell> Tools/Scripts/run-webkit-tests --debug fast/profiler/anonymous-event-handler.html --iterations=2
    FAILs

I could spend some more time looking into this, but we've already tried to work around it twice. I think it might be better for the JSC VM to handle recompiling when appropriate. No timers involved.
Comment 3 Joseph Pecoraro 2014-01-24 11:36:15 PST
> The naive approach of just making window.internals.setJavaScriptProfilerEnabled did not work.

window.internals.setJavaScriptProfilerEnabled *use a timer* did not work.
Comment 4 Joseph Pecoraro 2014-01-24 12:00:56 PST
Created attachment 222131 [details]
[PATCH] Proposed Fix - Let the VM manage recompilation

Suggestions on how I should measure the performance of this?
Comment 5 Oliver Hunt 2014-01-24 12:03:25 PST
Comment on attachment 222131 [details]
[PATCH] Proposed Fix - Let the VM manage recompilation

View in context: https://bugs.webkit.org/attachment.cgi?id=222131&action=review

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:66
> +    if (UNLIKELY(m_recompilationNeeded)) {

You can't do this unless .entryScope is null.  Otherwise you have to set the parent's entry scope recompile flag.
Comment 6 Joseph Pecoraro 2014-01-24 12:05:55 PST
Comment on attachment 222131 [details]
[PATCH] Proposed Fix - Let the VM manage recompilation

View in context: https://bugs.webkit.org/attachment.cgi?id=222131&action=review

>> Source/JavaScriptCore/runtime/VMEntryScope.cpp:66
>> +    if (UNLIKELY(m_recompilationNeeded)) {
> 
> You can't do this unless .entryScope is null.  Otherwise you have to set the parent's entry scope recompile flag.

The next line ASSERTs that.
Comment 7 Joseph Pecoraro 2014-01-24 12:16:29 PST
Created attachment 222134 [details]
[PATCH] Proposed Fix - Let VM manage recompilation

More straightforward approach, instead of just adjusting the topEntryScope.
Comment 8 Joseph Pecoraro 2014-01-24 13:31:40 PST
Landed <http://trac.webkit.org/changeset/162720>.