Bug 127566 - fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer
Summary: fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-24 10:39 PST by Joseph Pecoraro
Modified: 2014-01-24 13:31 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Naive Approach - Does not work running multiple tests (2.57 KB, patch)
2014-01-24 11:35 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix - Let the VM manage recompilation (5.59 KB, patch)
2014-01-24 12:00 PST, Joseph Pecoraro
oliver: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix - Let VM manage recompilation (5.35 KB, patch)
2014-01-24 12:16 PST, Joseph Pecoraro
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.