[webkit-reviews] review denied: [Bug 63485] Support throwing away non-running code even while other code is running : [Attachment 98791] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 63485: Support throwing away non-running code even while other code is
running
https://bugs.webkit.org/show_bug.cgi?id=63485

Attachment 98791: Patch
https://bugs.webkit.org/attachment.cgi?id=98791&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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?