[webkit-reviews] review denied: [Bug 26027] Broadcast structure changes on prototype objects, to support better caching of property accesses. : [Attachment 30678] The patch!

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 26027: Broadcast structure changes on prototype objects, to support better
caching of property accesses.
https://bugs.webkit.org/show_bug.cgi?id=26027

Attachment 30678: The patch!
https://bugs.webkit.org/attachment.cgi?id=30678&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
> Index: bytecode/CodeBlock.cpp
> ===================================================================
> --- bytecode/CodeBlock.cpp	(revision 44076)
> +++ bytecode/CodeBlock.cpp	(working copy)
> @@ -1315,8 +1315,11 @@ CodeBlock::~CodeBlock()
>      }
>  
>      for (size_t size = m_methodCallLinkInfos.size(), i = 0; i < size; ++i) {

> -	   if (Structure* structure = m_methodCallLinkInfos[i].cachedStructure)

> +	   MethodCallLinkInfo& info = m_methodCallLinkInfos[i];
> +	   if (Structure* structure = info.cachedStructure)
>	       structure->deref();
> +	   if (info.proto)
> +	      
StructureChangeNotification::removeChangeNotification(info.proto, &info);
>      }

Probably best to deref after sending out the notification.

I think the phrase "remove change notification" is challenging to parse. I
believe this line of code says "this client is no longer interested in
notifications." I think AppKit's "observer" is a good word for "things that are
interested in notifications." 

So, how about renaming "removeChangeNotification" to "removeObserver" instead?

And maybe we should call the class "StructureChangeNotifier" or
"StructureChangeNotificationCenter" -- since it's not actually a notification
itself.

> +	   virtual void notify();

This function name could be a little more verbose about what it's a
notification for. How about "structureDidChange"?

> -    d()->methodCallDummy = constructEmptyObject(exec);

Yay!

> +	   static const int InheritorIDFlagMask = 1;
> +	   static const int InheritorIDFlagBroadcast = 0;

How about "InheritorIDFlagBroadcastStructureChanges", to indicate what we're
supposed to broadcast?

> +	   void broadcastStructureChanges()
> +	   {
> +	       m_inheritorID.setFlag(InheritorIDFlagBroadcast);
> +	   }

How about "startBroadcastingStructureChanges"? "broadcastStructureChanges"
sounds like a one-time thing.

>  inline void JSObject::setStructure(PassRefPtr<Structure> structure)
>  {
> +    if (m_inheritorID.getFlag(InheritorIDFlagBroadcast))
> +	   StructureChangeNotification::reportStructureChange(this);

I guess the idea here is to avoid a hash lookup to see if you need to broadcast
your structure change.

Just checking m_inheritorID for NULL would serve the same purpose for all
non-prototype objects.

For prototype objects, I don't know if I buy that this is a real optimization.
It will only help in the specific scenario where an object, after being used as
a prototype, but before ever being accessed by optimized code, is modified a
lot. Both of those conditions seem rare to me. (If the object were not used as
a prototype, m_inheritorID would be NULL, and the optimization would be
unnecessary. If the object were accessed by optimized code,
InheritorIDFlagBroadcast would be true, and the optimization would fail.)

The downside to this optimization is that relying on pointer alignment to set
flags is slightly black magic. I've heard that it has caused a number of
compatibility problems in the past. (We are one step ahead on the compatibility
front, since we allocate our own memory. But we hope to stop doing that some
day.) And it makes for weird code to set a flag on a pointer that is not
directly tied to the value pointed to.

Maybe it's worth reconsidering this part of your patch.

> +static SpinLock spinlock = SPINLOCK_INITIALIZER;
> +
> +StructureChangeNotification::~StructureChangeNotification() {}
> +
> +StructureChangeNotification::NotificationMap*
StructureChangeNotification::s_registeredNotifications = 0;
> +
> +void StructureChangeNotification::createNotificationMap()
> +{
> +    SpinLockHolder lock_holder(&spinlock);
> +    s_registeredNotifications = new NotificationMap();
> +}

Why is it safe to use a shared NotificationMap for all JS threads? Couldn't two
threads try to add and/or remove and/or get at the same time, causing
corruption? Can we make this object a member of JSGlobalData instead?

> +	   T* get() const
> +	   {
> +	       if (FlagMask)
> +		   return
reinterpret_cast<T*>(reinterpret_cast<intptr_t>(m_ptr) & ~FlagMask);
> +	       else
> +		   return m_ptr;
> +	   }

Despite my poo pooing above, I do think this is a neat trick, which could be
useful in a few places. But, since RefPtr is so fundamental to WebCore, maybe
this RefPtr change is worth a separate patch, which might draw comments form
Maciej and/or Darin, among others.

I'm going to say r- because of the threading thing, but please consider
splitting out the RefPtr change into a separate patch, along with the other
suggestions.