[Webkit-unassigned] [Bug 26448] Need to optimize MessagePort GC for same-thread case

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 18 15:38:23 PDT 2009


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


levin at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31501|review?                     |review-
               Flag|                            |




------- Comment #2 from levin at chromium.org  2009-06-18 15:38 PDT -------
(From update of attachment 31501)
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.


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



More information about the webkit-unassigned mailing list