Support using TrackedCallbacks as hints to determine the handling thread of resource reply messages

ppapi/proxy/resource_reply_thread_registrar.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PPAPI_PROXY_RESOURCE_REPLY_THREAD_REGISTRAR_H_
+#define PPAPI_PROXY_RESOURCE_REPLY_THREAD_REGISTRAR_H_
+
+#include <map>
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "ppapi/c/pp_resource.h"
+#include "ppapi/proxy/ppapi_proxy_export.h"
+
+
+namespace base {
+class MessageLoopProxy;
+}
+
+namespace ppapi {
+
+class TrackedCallback;
+
+namespace proxy {
+
+// ResourceReplyThreadRegistrar records handling thread for

[dmichael (off chromium), 2013/12/10 23:38:01]

nit: records +the+ handling

[yzshen1, 2013/12/11 00:21:49]

Done.

+// PpapiPluginMsg_ResourceReply messages.
+// This class is thread safe.
+class PPAPI_PROXY_EXPORT ResourceReplyThreadRegistrar
+    : public base::RefCountedThreadSafe<ResourceReplyThreadRegistrar> {
+ public:
+  explicit ResourceReplyThreadRegistrar(
+      scoped_refptr<base::MessageLoopProxy> default_thread);
+
+  void Register(PP_Resource resource,
+                int32_t sequence_number,
+                scoped_refptr<TrackedCallback> reply_thread_hint);
+  void Unregister(PP_Resource resource);
+
+  scoped_refptr<base::MessageLoopProxy> GetTargetThreadAndUnregister(

[dmichael (off chromium), 2013/12/10 23:38:01]

As I understand it, Unregister and this method should be called *without* the
ProxyLock, while Register has to have it. Not clear about the constructor and
destructor; presumably they don't care (and it's probably not locked when they
run).

[yzshen1, 2013/12/11 00:21:49]

All methods except Register() don't care about whether ProxyLock is held or not,
I think.

I added a comment to Register that it must be called while the proxy lock is
held. (And I also have a debug check in the impl.)

+      PP_Resource resource,
+      int32_t sequence_number);
+
+ private:
+  friend class base::RefCountedThreadSafe<ResourceReplyThreadRegistrar>;
+
+  typedef std::map<int32_t, scoped_refptr<base::MessageLoopProxy> >
+      SequenceNumberMap;
+  typedef std::map<PP_Resource, SequenceNumberMap> ResourceMap;
+
+  ~ResourceReplyThreadRegistrar();
+
+  base::Lock lock_;

[dmichael (off chromium), 2013/12/10 23:38:01]

I find this a little bit scary, like we might mess up and either deadlock or
have a race condition. (Though I think your current code is correct). It might
be nice if we only used the ProxyLock, though I'm guessing you are worried about
too much contention.

Have you benchmarked to see how much difference there is? In terms of raw work,
there's not much difference. If you rely only on the proxy lock, then Register
will only have to lock one mutex instead of two. Unregister would lock the proxy
lock instead of this one. But the PRoxyLock will obviously be more contended...


At a minimum, this should be documented better, w.r.t. the locking expectations
of each method and noting which lock(s) protect the members.

[yzshen1, 2013/12/11 00:21:49]

Yeah, GetTargetThreadAndUnregister() will be called on the IO thread for every
incoming reply, it seems better if we can use a separate lock.

I think it should be safe, because this lock only protects the internal state of
the class, and while holding the lock, we never try to acquire the pepper proxy
lock.
I am writing an test, and will test and see how much is the difference.
 
I added a comment for Register() and a comment for |lock_|. Please take a look.

+  ResourceMap map_;
+  scoped_refptr<base::MessageLoopProxy> default_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(ResourceReplyThreadRegistrar);
+};
+
+}  // namespace proxy
+}  // namespace ppapi
+
+#endif   // PPAPI_PROXY_RESOURCE_REPLY_THREAD_REGISTRAR_H_