[webkit-reviews] review denied: [Bug 26448] Need to optimize MessagePort GC for same-thread case : [Attachment 31501] Proposed patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Andrew Wilson
<atwilson at google.com>'s request for review:
Bug 26448: Need to optimize MessagePort GC for same-thread case
https://bugs.webkit.org/show_bug.cgi?id=26448

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

------- Additional Comments from David Levin <levin at chromium.org>
A few comments to address and then this should be fine (so r- for now).


> diff --git a/LayoutTests/fast/events/message-channel-gc-4.html-disabled
b/LayoutTests/fast/events/message-channel-gc-4.html-disabled

> +    for (var i = 0; i < 10000; i++) { // > force garbage collection (FF
requires about 9K allocations before a collect)

The ">" in the comment doesn't make sense to me.


> +function test1()
> +{
> +    var channel = new MessageChannel;
> +    var channel2 = new MessageChannel;
> +    channel.port1.postMessage("msg1");
> +    channel2.port1.postMessage("", channel.port1);
> +    channel2.port2.postMessage("", channel.port2);
> +    channel2.port2.onmessage = channel2.port1.onmessage = function(event) {
> +	  event.messagePort.onmessage = function(event) {

It seems like it would be better if this didn't use "event" again since this is

already in the outer scope.


> +	       } else {
> +		   log("FAIL: Received unknown message: " + event.data);
> +	       }
> +	   event.messagePort = 0;
Indentation is off or it is in the wrong scope.

> +	   }
> +    }

> diff --git a/WebCore/dom/MessagePort.h b/WebCore/dom/MessagePort.h

> +	   // Returns null if there is no entangled port, or if the port is
entangled remotely.
This function is given a description in 3 places.  I think this one is the
least descriptive because
I don't know what "entangled remotely" means here.

> +	   MessagePort* locallyEntangledPort();
> +	   bool isEntangled() { return m_entangledChannel; }


> diff --git a/WebCore/dom/MessagePortChannel.h
b/WebCore/dom/MessagePortChannel.h


>	   // Extracts a message from the message queue for this port.
>	   bool tryGetMessageFromRemote(OwnPtr<EventData>&);
>  
> +	   // Returns a reference to the entangled port, if it is run by the
same thread.
> +	   // Returns null if remotely entangled, or if disentangled due to the
remote port being cloned and in-transit to a new owner.

How about:
// Returns a reference to the entangled port, if it is run by on same thread as
the given ScriptExecutionContext.
// Returns null otherwise.
?


> diff --git a/WebCore/dom/default/PlatformMessagePortChannel.cpp
b/WebCore/dom/default/PlatformMessagePortChannel.cpp

> +MessagePort* PlatformMessagePortChannel::locallyEntangledPort(const
ScriptExecutionContext& context)
> +{
> +    MutexLocker lock(m_mutex);
> +    // See if both contexts are run by the same thread (are the same
context, or are both documents).
> +    if (m_remotePort) {
> +	   ScriptExecutionContext* remoteContext =
m_remotePort->scriptExecutionContext();
> +	   if (remoteContext == &context || (remoteContext &&
remoteContext->isDocument() && context.isDocument()))

2 Issues:
1. (Minor) It feels odd to do pointer comparison on an item passed in using a
ref.  Maybe we should get rid of the ref here and just assert?	What do you
think?
2. (Major) When you call "remoteContext->isDocument()" if remoteContext is a
context in a different thread, how do you know that it hasn't been deleted yet?


You explained this one to me.  It is because it has the lock... It may be nice
to add some small comment here to note that because it looked wrong when
reading this code.
 


> diff --git a/WebCore/dom/default/PlatformMessagePortChannel.h
b/WebCore/dom/default/PlatformMessagePortChannel.h

> +	   // Returns a reference to the entangled port, if it is run by the
same thread.
> +	   // Returns null if remotely entangled, or if disentangled due to the
remote port being cloned and in-transit to a new owner.
I added a comment about this comment below.  It is a shame to have the same
comment twice and seems destine to get out of sync.

Can we get rid of this one (You could refer to the other header file if
desired, something like // See MessagePortChannel.h for the description.)

Same thing for the duplicate comment in hasPendingActivity().

What do you think?


> +	   // Used only to do more optimal GC, so platforms where this is
difficult to determine can always just return null.
Are there platforms where it will return NULL even if these items on one the
same thread?  If so, then the comment above is wrong.