[webkit-reviews] review granted: [Bug 71977] DFG should not reparse code that was just parsed : [Attachment 114468] the patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 71977: DFG should not reparse code that was just parsed
https://bugs.webkit.org/show_bug.cgi?id=71977

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=114468&action=review


r=me, with some comments below.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1655
> +    if (hasInstructions() && m_shouldDiscardBytecodeLater)

The "Later" in "m_shouldDiscardBytecodeLater" confused me because it sounded
like setting the flag was a way to delay or prevent discarding bytecode ("Not
now, later"), but actually it's the way to cause bytecode to be discarded. I
think "m_shouldDiscardBytecode" would be better.

> Source/JavaScriptCore/dfg/DFGByteCodeCache.h:68
> +	   // This is likely to be good enough.

Sneaky, but true. I would remove this comment, or add a "why" explanation.

Thinking this through, I wonder if | might be clearer than ^ here. This hash
function really just relies on the low bits of m_executable being zero, and the
low bits of m_kind being meaningful.

> Source/JavaScriptCore/heap/ListableHandler.h:60
> +	   void addNotThreadSafe(T* handler)

Please make this private.

> Source/JavaScriptCore/heap/UltraWeakFinalizer.h:37
> +class UltraWeakFinalizer : public ListableHandler<UltraWeakFinalizer> {

The "Ultra" in "UltraWeak" didn't do enough to tell me what this class is. To
my mind, two things are unique about this kind of "finalizer": (1) It fires no
matter what, even if the owning object is live; (2) If GC happens soon enough,
it might prefer not to fire, thus avoiding cache churn. Based on those facts,
I'd suggest:

- CacheOwner (with a "clearCache" callback)
OR
- UnconditionalFinalizer (with a "finalize" callback)