Bug 63485 – Support throwing away non-running code even while other code is running

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

[Oliver Hunt]

Support throwing away non-running code even while other code is running

[Oliver Hunt]

Created attachment 98791 [details]
Patch

[Geoffrey Garen]

Comment on attachment 98791 [details]
Patch

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?

[Oliver Hunt]

Created attachment 98820 [details]
Patch

[Geoffrey Garen]

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

[Oliver Hunt]

Created attachment 98836 [details]
Patch

[Geoffrey Garen]

Comment on attachment 98836 [details]
Patch

r=me

[Oliver Hunt]

Committed r89885: <http://trac.webkit.org/changeset/89885>

[Ryosuke Niwa]

This patch broke Snow Leopard, Windows, and WinCairo builds:

http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/31350/steps/compile-webkit/logs/warnings%20%281%29
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/31350/steps/compile-webkit/logs/stdio

http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/37159/steps/compile-webkit/logs/stdio

http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/7884/steps/compile-webkit/logs/stdio

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

Build fix: http://trac.webkit.org/changeset/89887

It seems like Windows XP (Debug) needs some sort of a "kick":
http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/37160/steps/compile-webkit/logs/stdio

[Alexander Pavlov (apavlov)]

Windows (Debug) Builder still broken, as of r89922

[Ryosuke Niwa]

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