[Webkit-unassigned] [Bug 71977] DFG should not reparse code that was just parsed

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Filip Pizlo <fpizlo at apple.com>  2011-11-10 14:03:20 PST ---
> 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.

I dropped the Later.

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

I dropped the comment.  It's actually less sneaky and even more correct: I'm using the PtrHash, which picks an appropriate int hash, so even just the pointer hash has all bits populated with some manner of rubbish.  Then I xor in the mode enum, which flips the low bit.

It's only "likely" to be good enough because if someone changed the hash table implementation to use high bits instead of low bits in the mask, then this would become slightly sub-optimal.  But even then it would only be *slightly* sub-optimal because it would mean collisions when you want to inline an executable for both a call and a construction.  And even then that collision probably won't cost you anything since the dominant cost of inlining is, well, inlining.  So we're starting to enter into the unlikely * unlikely * unlikely domain of probabilities.

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

I did, though probably one of my next patches will make it public again.  The WeakReferenceHarvester list should be processed multiple times, and the easiest way to do that is for the GC to consume the global one into a private one, which can be added to without locking.

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

I picked UnconditionalFinalizer, and made the callback "finalizeUnconditionally", since I did not want CodeBlock to have a method called "finalize".  It would be hard to tell where that method came from.

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