Bug 26448 – Need to optimize MessagePort GC for same-thread case

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andrew Wilson]

The patch for bug #25043 reflects the latest spec - any entangled MessagePort should be treated as "reachable" regardless of whether its entangled port is reachable or not. This spec change was driven by the need to support ports that are entangled across multiple threads where it is difficult to determine reachability.

In the case where both ends of the MessagePort are run by the same thread, it is possible to determine reachability correctly. This would allow us to GC unreachable ports in case like this:

var channel = new MessageChannel();
channel = 0;

[Andrew Wilson]

Created attachment 31501 [details]
Proposed patch

I'd like to land this patch along with the patch for 25043, as it makes the merge into Chromium much simpler. So if we could land them together that would be much appreciated.

[David Levin]

Comment on attachment 31501 [details]
Proposed patch

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.

[Andrew Wilson]

Created attachment 31521 [details]
Revised patch per review comments

[David Levin]

Comment on attachment 31521 [details]
Revised patch per review comments

> diff --git a/LayoutTests/fast/events/message-channel-gc-4.html-disabled b/LayoutTests/fast/events/message-channel-gc-4.html-disabled
> +    channel2.port2.postMessage("", channel.port2);
> +    channel2.port2.onmessage = channel2.port1.onmessage = function(evt) {
> +       evt.messagePort.onmessage = function(event) {
This should be indented by one more space.  (The alignment of things just looked funny when I was reviewing it.)


I'll fix this when landing.

[David Levin]

Assigned to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/44916