[webkit-reviews] review granted: [Bug 67072] JSC::Executable is inconsistent about using weak handle finalizers and destructors for releasing memory : [Attachment 105424] the patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 67072: JSC::Executable is inconsistent about using weak handle finalizers
and destructors for releasing memory
https://bugs.webkit.org/show_bug.cgi?id=67072

Attachment 105424: the patch
https://bugs.webkit.org/attachment.cgi?id=105424&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105424&action=review


> Source/JavaScriptCore/runtime/Executable.cpp:251
> +    if (m_evalCodeBlock)
> +	   m_evalCodeBlock->clearEvalCache();
> +    m_evalCodeBlock.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.cpp:343
> +    if (m_programCodeBlock)
> +	   m_programCodeBlock->clearEvalCache();
> +    m_programCodeBlock.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.cpp:496
>      if (m_codeBlockForCall)
>	   m_codeBlockForCall->clearEvalCache();
>      m_codeBlockForCall.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.cpp:499
>      if (m_codeBlockForConstruct)
>	   m_codeBlockForConstruct->clearEvalCache();
>      m_codeBlockForConstruct.clear();

Could put the clear inside the if.

> Source/JavaScriptCore/runtime/Executable.h:-69
> -#if ENABLE(JIT)
>	       Weak<ExecutableBase> finalizer(globalData, this,
executableFinalizer());
>	       finalizer.leakHandle();
> -#endif

Would be nice to have a comment in the change log explaining this change.

> Source/JavaScriptCore/runtime/Executable.h:337
> +	   virtual void clearCode();

I like to follow a pattern where virtual functions overrides are private or
protected, even when the base class function is public. This sometimes lets us
discover cases where we would be doing a virtual function dispatch and could
instead do a non-virtual, and has little cost.

Accordingly, I suggest making this protected and private in derived classes,
and leave it public only in ExecutableBase.