[Webkit-unassigned] [Bug 67072] JSC::Executable is inconsistent about using weak handle finalizers and destructors for releasing memory

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67072


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105424|review?                     |review+
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2011-08-27 11:54:53 PST ---
(From update of attachment 105424)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.