Bug 63485 - Support throwing away non-running code even while other code is running
Summary: Support throwing away non-running code even while other code is running
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
Depends on:
Reported: 2011-06-27 14:48 PDT by Oliver Hunt
Modified: 2011-06-28 06:05 PDT (History)
8 users (show)

See Also:

Patch (20.30 KB, patch)
2011-06-27 14:52 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (20.81 KB, patch)
2011-06-27 17:20 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (20.70 KB, patch)
2011-06-27 18:15 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-06-27 14:48:35 PDT
Support throwing away non-running code even while other code is running
Comment 1 Oliver Hunt 2011-06-27 14:52:58 PDT
Created attachment 98791 [details]
Comment 2 Geoffrey Garen 2011-06-27 16:00:27 PDT
Comment on attachment 98791 [details]

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

r- because there's some more work to do here.

> Source/JavaScriptCore/ChangeLog:8
> +        Add a function to CodeBlock to support unlinking direct linked callsites,

"call site" is two words.

> Source/JavaScriptCore/ChangeLog:12
> +        The unlinking completely reverts any optimised callsites, such that they

"optimized" is the spelling in US English.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1695
> +    // Ideally we'd unlink the method_check optimisation here as
> +    // well, but that would require additional work to deoptimise
> +    // general case property accesses.

I think you can get rid of this comment, since it doesn't contribute to understanding the code around it. If you think this is important work to do, filing a bug is the best way to remember to do it.

> Source/JavaScriptCore/bytecode/EvalCodeCache.h:75
> +        void invalidate()
> +        {
> +            m_cacheMap.clear();
> +        }

I think "clear" is a better name for this function, since that's what it does.

> Source/JavaScriptCore/jit/JITWriteBarrier.h:73
> +    void clearToMarkedStructure() { clear(reinterpret_cast<void*>(-1)); }
> +    void clearLocation() { m_location = CodeLocationDataLabelPtr(); }

Is this for another patch?

> Source/JavaScriptCore/jsc.cpp:158
> +    putDirectFunction(globalExec(), new (globalExec()) JSFunction(globalExec(), this, functionStructure(), 0, Identifier(globalExec(), "releaseExecutableMemory"), functionReleaseExecutableMemory));

#ifndef NDEBUG only, please.

> Source/JavaScriptCore/runtime/Executable.h:266
> +        void unlinkCalls();
>          OwnPtr<EvalCodeBlock> m_evalCodeBlock;

Newline between function and data declaration, please.

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:453
> +struct SafeRecompiler {

This can inherit from VoidFunctor.

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:455
> +    typedef bool ReturnType;
> +    static bool returnValue() { return true; }

Then you can remove these.

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:472
> +        SafeRecompiler recompiler;

"Safe" is something that all objects should be, so "SafeRecompiler" isn't a great class name. How about "ConservativeRecompiler" or "StackPreservingRecompiler".

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:480
> +        heap.getConservativeRoots(roots);
> +        HashSet<JSCell*>::iterator end = roots.end();
> +        for (HashSet<JSCell*>::iterator ptr = roots.begin(); ptr != end; ++ptr) {
> +            ScriptExecutable* executable = 0;
> +            JSCell* cell = *ptr;
> +            // Gathering roots se reset it to avoid badness
> +            heap.setMarked(cell);

Two comments:

(1) gatherConservativeRoots seems like the wrong API here. We only want to know about objects on the JS stack -- not all objects referenced by C.

(2) gatherConservativeRoots should do the job of re-setting the mark bit. We don't want to give the Heap an API that can leave it in an invalid state.

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:493
> +                recompiler.liveFunctions.add(static_cast<FunctionExecutable*>(executable));

"currentlyExecutingFunctions" is a better name than "liveFunctions" because lots of functions are live, but we'll throw away their code anyway.

> Source/JavaScriptCore/runtime/JSGlobalData.cpp:499
> +    heap.collectAllGarbage();

Kind of a shame that we'll iterate the heap twice, but I guess there's currently no other way to identify running eval and program code?
Comment 3 Oliver Hunt 2011-06-27 17:20:32 PDT
Created attachment 98820 [details]
Comment 4 Geoffrey Garen 2011-06-27 18:12:38 PDT
Oliver mentioned that the conservative stack trace saves us in cases where we would throw away a function during its own compilation.

He's going to post a new patch that saves us from that case inside discardCode().
Comment 5 Oliver Hunt 2011-06-27 18:15:30 PDT
Created attachment 98836 [details]
Comment 6 Geoffrey Garen 2011-06-27 18:17:31 PDT
Comment on attachment 98836 [details]

Comment 7 Oliver Hunt 2011-06-27 18:32:14 PDT
Committed r89885: <http://trac.webkit.org/changeset/89885>
Comment 9 Ryosuke Niwa 2011-06-27 19:24:29 PDT
It seems like the problem is that __ZN3JSC12JSGlobalData23releaseExecutableMemoryEv isn't defined for release builds but JavaScriptCore.exp tries to export it.

Can we add a preprocessor in JavaScriptCore.exp?
Comment 10 Ryosuke Niwa 2011-06-27 19:39:56 PDT
Build fix: http://trac.webkit.org/changeset/89887

It seems like Windows XP (Debug) needs some sort of a "kick":
Comment 11 Alexander Pavlov (apavlov) 2011-06-28 05:34:54 PDT
Windows (Debug) Builder still broken, as of r89922
Comment 12 Ryosuke Niwa 2011-06-28 06:05:29 PDT
(In reply to comment #11)
> Windows (Debug) Builder still broken, as of r89922

Linker somehow cannot see JSGlobalData::releaseExecutableMemory from jsc.obj.  Windows 7 (Release) is building fine though.