Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498743002:
Mojo: introducing a thread safe interface pointer. (Closed)
|
Created:
4 years, 1 month ago by Jay Civelli Modified:
4 years, 1 month ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreating a thread safe interface pointer that is tied to an actual
InterfacePtr and forwards message from interface calls to the
InterfacePtr thread and forwards back response messages to the
calling thread.
BUG=664628
TEST=InterfacePtrTest.TestThreadSafeInterfacePointer
Committed: https://crrev.com/64c7ea9ccda1354968015f4979135760627435e1
Cr-Commit-Position: refs/heads/master@{#432387}
Patch Set 1 #Patch Set 2 : Clean-up #
Total comments: 8
Patch Set 3 : Addressed comments. #Patch Set 4 : Sync + clean-up #Patch Set 5 : Fix Android compile #
Total comments: 16
Patch Set 6 : Addressed @yzshen1 comments. #
Total comments: 2
Patch Set 7 : Mojo: introducing a thread safe interface pointer. #
Messages
Total messages: 49 (21 generated)
Description was changed from ========== Mojo: introducing a thread safe interface pointer. Creating a thread safe interface pointer that is tied to an actual InterfacePtr and forwards message from interface calls to the InterfacePtr thread and forwards back response messages to the calling thread. BUG=664628 TEST=InterfacePtrTest.TestThreadSafeInterfacePointer ========== to ========== Creating a thread safe interface pointer that is tied to an actual InterfacePtr and forwards message from interface calls to the InterfacePtr thread and forwards back response messages to the calling thread. BUG=664628 TEST=InterfacePtrTest.TestThreadSafeInterfacePointer ==========
jcivelli@chromium.org changed reviewers: + rockot@chromium.org, yzshen@chromium.org
I don't have time to look at this today (OOO) but I already know the design since we discussed it yesterday. I will defer to yzshen@ on the review. :)
https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right): https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/interface_ptr_state.h:336: scoped_refptr<ThreadSafeInterfacePtr<Interface>> GetThreadSafePtr() { One thing that seems a little weird about this is that the ThreadSafeInterfacePtr is tied to the InterfacePtrState object itself, which is not moved around like its contents. As a result, when an InterfacePtr x is moved to y, although it behaves exactly the same, its connection with the ThreadSafeInterfacePtr has gone. https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:30: // any thread. Please consider adding comments about whether the InterfacePtr should be kept alive while using this ThreadSafeInterfacePtr. https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:41: Interface* get_interface() { return &proxy_; } Does it make sense to name it "get()" to be more consistent with InterfacePtr? It also seems convenient to add things like "operator->()", "operator bool()", etc. Generally speaking, it is nice to have (mostly) the same public API as InterfacePtr. WDYT?
The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
darin@chromium.org changed reviewers: + darin@chromium.org
https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { Does this class need to be ref-counted? Can it be move-only instead?
On 2016/11/12 at 04:09:23, darin wrote: > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > Does this class need to be ref-counted? Can it be move-only instead? Seems reasonable to make it move-only. I think it only ended up as ref-counted by default because it was inspired by ThreadSafeSender.
On 2016/11/12 09:01:23, Ken Rockot wrote: > On 2016/11/12 at 04:09:23, darin wrote: > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > > Does this class need to be ref-counted? Can it be move-only instead? > > Seems reasonable to make it move-only. I think it only ended up as ref-counted > by default because it was inspired by ThreadSafeSender. Also to guarantee it won't break if the backing InterfacePtr is reassigned (as @yzshen1 pointed out) I am thinking of making it take the InterfacePtr in when constructed (so it owns it). WDYT?
On Nov 12, 2016 8:56 AM, <jcivelli@chromium.org> wrote: > > On 2016/11/12 09:01:23, Ken Rockot wrote: > > On 2016/11/12 at 04:09:23, darin wrote: > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > > > Does this class need to be ref-counted? Can it be move-only instead? > > > > Seems reasonable to make it move-only. I think it only ended up as ref-counted > > by default because it was inspired by ThreadSafeSender. > > Also to guarantee it won't break if the backing InterfacePtr is reassigned (as > @yzshen1 pointed out) I am thinking of making it take the InterfacePtr in when > constructed (so it owns it). > WDYT? That would be reasonable, but remember the InterfacePtr still needs to be bound to a specific thread where it's watched and ultimately destroyed. > > https://codereview.chromium.org/2498743002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/12 16:56:22, Jay Civelli wrote: > On 2016/11/12 09:01:23, Ken Rockot wrote: > > On 2016/11/12 at 04:09:23, darin wrote: > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > > > Does this class need to be ref-counted? Can it be move-only instead? > > > > Seems reasonable to make it move-only. I think it only ended up as ref-counted > > by default because it was inspired by ThreadSafeSender. > > Also to guarantee it won't break if the backing InterfacePtr is reassigned (as > @yzshen1 pointed out) I am thinking of making it take the InterfacePtr in when > constructed (so it owns it). > WDYT? If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems there is no point to introduce ThreadSafeInterfacePtr at all -- instead we just bind the InterfacePtr on the right thread directly. Btw, the name ThreadSafeInterfacePtr is a little misleading -- it cannot be used from multiple threads simultaneously. Maybe it should be named something like InterfacePtrProxy. WDYT? Thanks!
On 2016/11/13 at 02:29:18, yzshen wrote: > On 2016/11/12 16:56:22, Jay Civelli wrote: > > On 2016/11/12 09:01:23, Ken Rockot wrote: > > > On 2016/11/12 at 04:09:23, darin wrote: > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): > > > > > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > > > > Does this class need to be ref-counted? Can it be move-only instead? > > > > > > Seems reasonable to make it move-only. I think it only ended up as ref-counted > > > by default because it was inspired by ThreadSafeSender. > > > > Also to guarantee it won't break if the backing InterfacePtr is reassigned (as > > @yzshen1 pointed out) I am thinking of making it take the InterfacePtr in when > > constructed (so it owns it). > > WDYT? > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems there is no point to introduce ThreadSafeInterfacePtr at all -- instead we just bind the InterfacePtr on the right thread directly. > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it cannot be used from multiple threads simultaneously. Maybe it should be named something like InterfacePtrProxy. > > WDYT? Thanks! If it *weren't* move-only and stayed ref-counted, then it makes sense. It's thread-safe as long as the sender guarantees that it only calls into the InterfacePtr from a specific thread, including the destructor. So I think we'd want to choose between either a ref-counted thread-safe ptr which owns the InterfacePtr, or a move-only proxy type which doesn't own the InterfacePtr. The use case here is most certainly to have a single pipe endpoint which can have messages sent on it from multiple threads.
On 2016/11/13 02:38:59, Ken Rockot wrote: > On 2016/11/13 at 02:29:18, yzshen wrote: > > On 2016/11/12 16:56:22, Jay Civelli wrote: > > > On 2016/11/12 09:01:23, Ken Rockot wrote: > > > > On 2016/11/12 at 04:09:23, darin wrote: > > > > > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... > > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public > > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > > > > > Does this class need to be ref-counted? Can it be move-only instead? > > > > > > > > Seems reasonable to make it move-only. I think it only ended up as > ref-counted > > > > by default because it was inspired by ThreadSafeSender. > > > > > > Also to guarantee it won't break if the backing InterfacePtr is reassigned > (as > > > @yzshen1 pointed out) I am thinking of making it take the InterfacePtr in > when > > > constructed (so it owns it). > > > WDYT? > > > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems there is no > point to introduce ThreadSafeInterfacePtr at all -- instead we just bind the > InterfacePtr on the right thread directly. > > > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it cannot be > used from multiple threads simultaneously. Maybe it should be named something > like InterfacePtrProxy. > > > > WDYT? Thanks! > > If it *weren't* move-only and stayed ref-counted, then it makes sense. It's > thread-safe as long as the sender guarantees that it only calls into the > InterfacePtr from a specific thread, including the destructor. > > So I think we'd want to choose between either a ref-counted thread-safe ptr > which owns the InterfacePtr, or a move-only proxy type which doesn't own the > InterfacePtr. > > The use case here is most certainly to have a single pipe endpoint which can > have messages sent on it from multiple threads. Agreed that either we should make it (1) ref-counted thread-safe owning InterfacePtr (2) move-only proxy not owning InterfacePtr. If we go with (1), however, it seems a little confusing that it is called "ThreadSafe" but won't work if multiple threads use it simultaneously.
I am not following why (1) would not be thread-safe. On Mon, Nov 14, 2016 at 9:36 AM, <yzshen@chromium.org> wrote: > On 2016/11/13 02:38:59, Ken Rockot wrote: > > On 2016/11/13 at 02:29:18, yzshen wrote: > > > On 2016/11/12 16:56:22, Jay Civelli wrote: > > > > On 2016/11/12 09:01:23, Ken Rockot wrote: > > > > > On 2016/11/12 at 04:09:23, darin wrote: > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/ > mojo/public/cpp/bindings/thread_safe_interface_ptr.h > > > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h > (right): > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2498743002/diff/20001/ > mojo/public/cpp/bindings/thread_safe_interface_ptr.h#newcode33 > > > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public > > > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { > > > > > > Does this class need to be ref-counted? Can it be move-only > instead? > > > > > > > > > > Seems reasonable to make it move-only. I think it only ended up as > > ref-counted > > > > > by default because it was inspired by ThreadSafeSender. > > > > > > > > Also to guarantee it won't break if the backing InterfacePtr is > reassigned > > (as > > > > @yzshen1 pointed out) I am thinking of making it take the > InterfacePtr in > > when > > > > constructed (so it owns it). > > > > WDYT? > > > > > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems > there is > no > > point to introduce ThreadSafeInterfacePtr at all -- instead we just bind > the > > InterfacePtr on the right thread directly. > > > > > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it > cannot be > > used from multiple threads simultaneously. Maybe it should be named > something > > like InterfacePtrProxy. > > > > > > WDYT? Thanks! > > > > If it *weren't* move-only and stayed ref-counted, then it makes sense. > It's > > thread-safe as long as the sender guarantees that it only calls into the > > InterfacePtr from a specific thread, including the destructor. > > > > So I think we'd want to choose between either a ref-counted thread-safe > ptr > > which owns the InterfacePtr, or a move-only proxy type which doesn't own > the > > InterfacePtr. > > > > The use case here is most certainly to have a single pipe endpoint which > can > > have messages sent on it from multiple threads. > > Agreed that either we should make it (1) ref-counted thread-safe owning > InterfacePtr (2) move-only proxy not owning InterfacePtr. > > If we go with (1), however, it seems a little confusing that it is called > "ThreadSafe" but won't work if multiple threads use it simultaneously. > > https://codereview.chromium.org/2498743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/14 18:11:51, Ken Rockot wrote: > I am not following why (1) would not be thread-safe. Because it holds a FooProxy instance, which has a serialization context that is not thread safe. We could either (1) add a lock to protect this FooProxy instance, or (2) change FooProxy from using a shared serialization context to creating one on the stack for each message serialization.
I think the concern is that the ProxyType is not reentrant. After a chat with @yzshen, the plan is to keep the ThreadSafeInterfacePointer ref counted, have it own the InterfacePtr (and push to the original thread in the destructor to delete the InterfacePtr), and use a lock around calls to the ProxyType. On Mon, Nov 14, 2016 at 10:11 AM, Ken Rockot <rockot@chromium.org> wrote: > I am not following why (1) would not be thread-safe. > > On Mon, Nov 14, 2016 at 9:36 AM, <yzshen@chromium.org> wrote: > >> On 2016/11/13 02:38:59, Ken Rockot wrote: >> > On 2016/11/13 at 02:29:18, yzshen wrote: >> > > On 2016/11/12 16:56:22, Jay Civelli wrote: >> > > > On 2016/11/12 09:01:23, Ken Rockot wrote: >> > > > > On 2016/11/12 at 04:09:23, darin wrote: >> > > > > > >> > > > > >> > > > >> > >> https://codereview.chromium.org/2498743002/diff/20001/mojo/ >> public/cpp/bindings/thread_safe_interface_ptr.h >> > > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h >> (right): >> > > > > > >> > > > > > >> > > > > >> > > > >> > >> https://codereview.chromium.org/2498743002/diff/20001/mojo/ >> public/cpp/bindings/thread_safe_interface_ptr.h#newcode33 >> > > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public >> > > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { >> > > > > > Does this class need to be ref-counted? Can it be move-only >> instead? >> > > > > >> > > > > Seems reasonable to make it move-only. I think it only ended up as >> > ref-counted >> > > > > by default because it was inspired by ThreadSafeSender. >> > > > >> > > > Also to guarantee it won't break if the backing InterfacePtr is >> reassigned >> > (as >> > > > @yzshen1 pointed out) I am thinking of making it take the >> InterfacePtr in >> > when >> > > > constructed (so it owns it). >> > > > WDYT? >> > > >> > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems >> there is >> no >> > point to introduce ThreadSafeInterfacePtr at all -- instead we just >> bind the >> > InterfacePtr on the right thread directly. >> > > >> > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it >> cannot be >> > used from multiple threads simultaneously. Maybe it should be named >> something >> > like InterfacePtrProxy. >> > > >> > > WDYT? Thanks! >> > >> > If it *weren't* move-only and stayed ref-counted, then it makes sense. >> It's >> > thread-safe as long as the sender guarantees that it only calls into the >> > InterfacePtr from a specific thread, including the destructor. >> > >> > So I think we'd want to choose between either a ref-counted thread-safe >> ptr >> > which owns the InterfacePtr, or a move-only proxy type which doesn't >> own the >> > InterfacePtr. >> > >> > The use case here is most certainly to have a single pipe endpoint >> which can >> > have messages sent on it from multiple threads. >> >> Agreed that either we should make it (1) ref-counted thread-safe owning >> InterfacePtr (2) move-only proxy not owning InterfacePtr. >> >> If we go with (1), however, it seems a little confusing that it is called >> "ThreadSafe" but won't work if multiple threads use it simultaneously. >> >> https://codereview.chromium.org/2498743002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh, sorry, I didn't actually look at the CL in deatil :). when we originally discussed this design, I thought the plan was to construct a new SerializationContext on the stack for each message. Is that a problem? On Mon, Nov 14, 2016 at 10:26 AM, Jay Civelli <jcivelli@chromium.org> wrote: > I think the concern is that the ProxyType is not reentrant. > After a chat with @yzshen, the plan is to keep the > ThreadSafeInterfacePointer ref counted, have it own the InterfacePtr (and > push to the original thread in the destructor to delete the InterfacePtr), > and use a lock around calls to the ProxyType. > > On Mon, Nov 14, 2016 at 10:11 AM, Ken Rockot <rockot@chromium.org> wrote: > >> I am not following why (1) would not be thread-safe. >> >> On Mon, Nov 14, 2016 at 9:36 AM, <yzshen@chromium.org> wrote: >> >>> On 2016/11/13 02:38:59, Ken Rockot wrote: >>> > On 2016/11/13 at 02:29:18, yzshen wrote: >>> > > On 2016/11/12 16:56:22, Jay Civelli wrote: >>> > > > On 2016/11/12 09:01:23, Ken Rockot wrote: >>> > > > > On 2016/11/12 at 04:09:23, darin wrote: >>> > > > > > >>> > > > > >>> > > > >>> > >>> https://codereview.chromium.org/2498743002/diff/20001/mojo/p >>> ublic/cpp/bindings/thread_safe_interface_ptr.h >>> > > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h >>> (right): >>> > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > >>> https://codereview.chromium.org/2498743002/diff/20001/mojo/p >>> ublic/cpp/bindings/thread_safe_interface_ptr.h#newcode33 >>> > > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: >>> public >>> > > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { >>> > > > > > Does this class need to be ref-counted? Can it be move-only >>> instead? >>> > > > > >>> > > > > Seems reasonable to make it move-only. I think it only ended up >>> as >>> > ref-counted >>> > > > > by default because it was inspired by ThreadSafeSender. >>> > > > >>> > > > Also to guarantee it won't break if the backing InterfacePtr is >>> reassigned >>> > (as >>> > > > @yzshen1 pointed out) I am thinking of making it take the >>> InterfacePtr in >>> > when >>> > > > constructed (so it owns it). >>> > > > WDYT? >>> > > >>> > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems >>> there is >>> no >>> > point to introduce ThreadSafeInterfacePtr at all -- instead we just >>> bind the >>> > InterfacePtr on the right thread directly. >>> > > >>> > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it >>> cannot be >>> > used from multiple threads simultaneously. Maybe it should be named >>> something >>> > like InterfacePtrProxy. >>> > > >>> > > WDYT? Thanks! >>> > >>> > If it *weren't* move-only and stayed ref-counted, then it makes sense. >>> It's >>> > thread-safe as long as the sender guarantees that it only calls into >>> the >>> > InterfacePtr from a specific thread, including the destructor. >>> > >>> > So I think we'd want to choose between either a ref-counted >>> thread-safe ptr >>> > which owns the InterfacePtr, or a move-only proxy type which doesn't >>> own the >>> > InterfacePtr. >>> > >>> > The use case here is most certainly to have a single pipe endpoint >>> which can >>> > have messages sent on it from multiple threads. >>> >>> Agreed that either we should make it (1) ref-counted thread-safe owning >>> InterfacePtr (2) move-only proxy not owning InterfacePtr. >>> >>> If we go with (1), however, it seems a little confusing that it is called >>> "ThreadSafe" but won't work if multiple threads use it simultaneously. >>> >>> https://codereview.chromium.org/2498743002/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
There is also the group_controller we'd have to take care of. We could go either way, I think one question is whether it might be cheaper to use a lock rather than creating a new context for every call. On Mon, Nov 14, 2016 at 10:30 AM, Ken Rockot <rockot@chromium.org> wrote: > Oh, sorry, I didn't actually look at the CL in deatil :). > > when we originally discussed this design, I thought the plan was to > construct a new SerializationContext on the stack for each message. Is that > a problem? > > On Mon, Nov 14, 2016 at 10:26 AM, Jay Civelli <jcivelli@chromium.org> > wrote: > >> I think the concern is that the ProxyType is not reentrant. >> After a chat with @yzshen, the plan is to keep the >> ThreadSafeInterfacePointer ref counted, have it own the InterfacePtr (and >> push to the original thread in the destructor to delete the InterfacePtr), >> and use a lock around calls to the ProxyType. >> >> On Mon, Nov 14, 2016 at 10:11 AM, Ken Rockot <rockot@chromium.org> wrote: >> >>> I am not following why (1) would not be thread-safe. >>> >>> On Mon, Nov 14, 2016 at 9:36 AM, <yzshen@chromium.org> wrote: >>> >>>> On 2016/11/13 02:38:59, Ken Rockot wrote: >>>> > On 2016/11/13 at 02:29:18, yzshen wrote: >>>> > > On 2016/11/12 16:56:22, Jay Civelli wrote: >>>> > > > On 2016/11/12 09:01:23, Ken Rockot wrote: >>>> > > > > On 2016/11/12 at 04:09:23, darin wrote: >>>> > > > > > >>>> > > > > >>>> > > > >>>> > >>>> https://codereview.chromium.org/2498743002/diff/20001/mojo/p >>>> ublic/cpp/bindings/thread_safe_interface_ptr.h >>>> > > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h >>>> (right): >>>> > > > > > >>>> > > > > > >>>> > > > > >>>> > > > >>>> > >>>> https://codereview.chromium.org/2498743002/diff/20001/mojo/p >>>> ublic/cpp/bindings/thread_safe_interface_ptr.h#newcode33 >>>> > > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: >>>> public >>>> > > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { >>>> > > > > > Does this class need to be ref-counted? Can it be move-only >>>> instead? >>>> > > > > >>>> > > > > Seems reasonable to make it move-only. I think it only ended up >>>> as >>>> > ref-counted >>>> > > > > by default because it was inspired by ThreadSafeSender. >>>> > > > >>>> > > > Also to guarantee it won't break if the backing InterfacePtr is >>>> reassigned >>>> > (as >>>> > > > @yzshen1 pointed out) I am thinking of making it take the >>>> InterfacePtr in >>>> > when >>>> > > > constructed (so it owns it). >>>> > > > WDYT? >>>> > > >>>> > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems >>>> there is >>>> no >>>> > point to introduce ThreadSafeInterfacePtr at all -- instead we just >>>> bind the >>>> > InterfacePtr on the right thread directly. >>>> > > >>>> > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it >>>> cannot be >>>> > used from multiple threads simultaneously. Maybe it should be named >>>> something >>>> > like InterfacePtrProxy. >>>> > > >>>> > > WDYT? Thanks! >>>> > >>>> > If it *weren't* move-only and stayed ref-counted, then it makes >>>> sense. It's >>>> > thread-safe as long as the sender guarantees that it only calls into >>>> the >>>> > InterfacePtr from a specific thread, including the destructor. >>>> > >>>> > So I think we'd want to choose between either a ref-counted >>>> thread-safe ptr >>>> > which owns the InterfacePtr, or a move-only proxy type which doesn't >>>> own the >>>> > InterfacePtr. >>>> > >>>> > The use case here is most certainly to have a single pipe endpoint >>>> which can >>>> > have messages sent on it from multiple threads. >>>> >>>> Agreed that either we should make it (1) ref-counted thread-safe owning >>>> InterfacePtr (2) move-only proxy not owning InterfacePtr. >>>> >>>> If we go with (1), however, it seems a little confusing that it is >>>> called >>>> "ThreadSafe" but won't work if multiple threads use it simultaneously. >>>> >>>> https://codereview.chromium.org/2498743002/ >>>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry the group controller is part of the serialization context, but I believe there were some subtleties with it though. On Mon, Nov 14, 2016 at 10:42 AM, Jay Civelli <jcivelli@chromium.org> wrote: > There is also the group_controller we'd have to take care of. > We could go either way, I think one question is whether it might be > cheaper to use a lock rather than creating a new context for every call. > > On Mon, Nov 14, 2016 at 10:30 AM, Ken Rockot <rockot@chromium.org> wrote: > >> Oh, sorry, I didn't actually look at the CL in deatil :). >> >> when we originally discussed this design, I thought the plan was to >> construct a new SerializationContext on the stack for each message. Is that >> a problem? >> >> On Mon, Nov 14, 2016 at 10:26 AM, Jay Civelli <jcivelli@chromium.org> >> wrote: >> >>> I think the concern is that the ProxyType is not reentrant. >>> After a chat with @yzshen, the plan is to keep the >>> ThreadSafeInterfacePointer ref counted, have it own the InterfacePtr (and >>> push to the original thread in the destructor to delete the InterfacePtr), >>> and use a lock around calls to the ProxyType. >>> >>> On Mon, Nov 14, 2016 at 10:11 AM, Ken Rockot <rockot@chromium.org> >>> wrote: >>> >>>> I am not following why (1) would not be thread-safe. >>>> >>>> On Mon, Nov 14, 2016 at 9:36 AM, <yzshen@chromium.org> wrote: >>>> >>>>> On 2016/11/13 02:38:59, Ken Rockot wrote: >>>>> > On 2016/11/13 at 02:29:18, yzshen wrote: >>>>> > > On 2016/11/12 16:56:22, Jay Civelli wrote: >>>>> > > > On 2016/11/12 09:01:23, Ken Rockot wrote: >>>>> > > > > On 2016/11/12 at 04:09:23, darin wrote: >>>>> > > > > > >>>>> > > > > >>>>> > > > >>>>> > >>>>> https://codereview.chromium.org/2498743002/diff/20001/mojo/p >>>>> ublic/cpp/bindings/thread_safe_interface_ptr.h >>>>> > > > > > File mojo/public/cpp/bindings/thread_safe_interface_ptr.h >>>>> (right): >>>>> > > > > > >>>>> > > > > > >>>>> > > > > >>>>> > > > >>>>> > >>>>> https://codereview.chromium.org/2498743002/diff/20001/mojo/p >>>>> ublic/cpp/bindings/thread_safe_interface_ptr.h#newcode33 >>>>> > > > > > mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: >>>>> public >>>>> > > > > base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> >>>>> { >>>>> > > > > > Does this class need to be ref-counted? Can it be move-only >>>>> instead? >>>>> > > > > >>>>> > > > > Seems reasonable to make it move-only. I think it only ended >>>>> up as >>>>> > ref-counted >>>>> > > > > by default because it was inspired by ThreadSafeSender. >>>>> > > > >>>>> > > > Also to guarantee it won't break if the backing InterfacePtr is >>>>> reassigned >>>>> > (as >>>>> > > > @yzshen1 pointed out) I am thinking of making it take the >>>>> InterfacePtr in >>>>> > when >>>>> > > > constructed (so it owns it). >>>>> > > > WDYT? >>>>> > > >>>>> > > If the ThreadSafeInterfacePtr owns the InterfacePtr, then it seems >>>>> there is >>>>> no >>>>> > point to introduce ThreadSafeInterfacePtr at all -- instead we just >>>>> bind the >>>>> > InterfacePtr on the right thread directly. >>>>> > > >>>>> > > Btw, the name ThreadSafeInterfacePtr is a little misleading -- it >>>>> cannot be >>>>> > used from multiple threads simultaneously. Maybe it should be named >>>>> something >>>>> > like InterfacePtrProxy. >>>>> > > >>>>> > > WDYT? Thanks! >>>>> > >>>>> > If it *weren't* move-only and stayed ref-counted, then it makes >>>>> sense. It's >>>>> > thread-safe as long as the sender guarantees that it only calls into >>>>> the >>>>> > InterfacePtr from a specific thread, including the destructor. >>>>> > >>>>> > So I think we'd want to choose between either a ref-counted >>>>> thread-safe ptr >>>>> > which owns the InterfacePtr, or a move-only proxy type which doesn't >>>>> own the >>>>> > InterfacePtr. >>>>> > >>>>> > The use case here is most certainly to have a single pipe endpoint >>>>> which can >>>>> > have messages sent on it from multiple threads. >>>>> >>>>> Agreed that either we should make it (1) ref-counted thread-safe owning >>>>> InterfacePtr (2) move-only proxy not owning InterfacePtr. >>>>> >>>>> If we go with (1), however, it seems a little confusing that it is >>>>> called >>>>> "ThreadSafe" but won't work if multiple threads use it simultaneously. >>>>> >>>>> https://codereview.chromium.org/2498743002/ >>>>> >>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right): https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/interface_ptr_state.h:336: scoped_refptr<ThreadSafeInterfacePtr<Interface>> GetThreadSafePtr() { On 2016/11/11 22:32:10, yzshen1 wrote: > One thing that seems a little weird about this is that the > ThreadSafeInterfacePtr is tied to the InterfacePtrState object itself, which is > not moved around like its contents. As a result, when an InterfacePtr x is moved > to y, although it behaves exactly the same, its connection with the > ThreadSafeInterfacePtr has gone. Per our discussion, the ThreadSafeInterfacePtr class is still thread safe ref counted, but it now owns the InterfacePtr. https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:30: // any thread. On 2016/11/11 22:32:10, yzshen1 wrote: > Please consider adding comments about whether the InterfacePtr should be kept > alive while using this ThreadSafeInterfacePtr. Updated that comment based on the modified design based on our discussions. https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:33: public base::RefCountedThreadSafe<ThreadSafeInterfacePtr<Interface>> { On 2016/11/12 04:09:23, darin (slow to review) wrote: > Does this class need to be ref-counted? Can it be move-only instead? After discussing with @yzshen1 we think it makes more sense to have it ref counted and that it owns the interface ptr. The reason is that a thread safe version makes it more prone to be used from different places (if that makes sense). https://codereview.chromium.org/2498743002/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:41: Interface* get_interface() { return &proxy_; } On 2016/11/11 22:32:10, yzshen1 wrote: > Does it make sense to name it "get()" to be more consistent with InterfacePtr? > It also seems convenient to add things like "operator->()", "operator bool()", > etc. > Generally speaking, it is nice to have (mostly) the same public API as > InterfacePtr. > > WDYT? Very good point. Done.
The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/interface_ptr_state.h:410: base::WeakPtrFactory<InterfacePtrState> weak_ptr_factory_; It is not needed in the majority case. Does it make sense to only construct it on demand? https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:52: static ThreadSafeInterfacePtr<Interface>* Create( It seems better to return a scoped_refptr. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:54: return new ThreadSafeInterfacePtr(std::move(interface_ptr), Please also deal with the case where interface_ptr is not bound. Maybe we return null in that case? https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:57: Do we need to also expose connection error handler? https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:163: serialization_context.group_controller = group_controller_; nit: please directly pass the group controller to the constructor of the serialization context. (here and elsewhere) https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:176: group_controller_.get(), &result I think .get() is not needed? https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_proxy_declaration.tmpl (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_proxy_declaration.tmpl:14: void set_group_controller(mojo::AssociatedGroupController* group_controller) { I think it is preferred to avoid passing raw pointer of ref-counted object as input? We could use a scoped_refptr as input and then do group_controller_ = std::move(group_controller) to avoid extra addref/release. WDYT?
https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/interface_ptr_state.h:410: base::WeakPtrFactory<InterfacePtrState> weak_ptr_factory_; On 2016/11/15 18:53:46, yzshen1 wrote: > It is not needed in the majority case. Does it make sense to only construct it > on demand? On Linux the size of a WeakPtrFactory is 16 bytes vs 8 for a unique_ptr. I am not sure it's worth adding more code that lazily instanciates for that, but maybe you have other concerns? https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:52: static ThreadSafeInterfacePtr<Interface>* Create( On 2016/11/15 18:53:46, yzshen1 wrote: > It seems better to return a scoped_refptr. Good idea, done. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:54: return new ThreadSafeInterfacePtr(std::move(interface_ptr), On 2016/11/15 18:53:46, yzshen1 wrote: > Please also deal with the case where interface_ptr is not bound. Maybe we return > null in that case? Done. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:57: On 2016/11/15 18:53:46, yzshen1 wrote: > Do we need to also expose connection error handler? That's a good question. Happy to add them to ThreadSafeInterfacePointer if you believe it makes sense. From my point of view, I wonder if the connection error handlers should be set on the original InterfacePtr before it gets passed to the ThreadSafeInterfacePointer. My thought is just for clarity reasons: with the InterfacePtr it's clear everything happens on the same thread. With ThreadSafeInterfacePointer, the handlers would still be invoked on the InterfacePtr's thread, but it might not be immediately clear to consumers. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:163: serialization_context.group_controller = group_controller_; On 2016/11/15 18:53:46, yzshen1 wrote: > nit: please directly pass the group controller to the constructor of the > serialization context. > > (here and elsewhere) Done. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:176: group_controller_.get(), &result On 2016/11/15 18:53:46, yzshen1 wrote: > I think .get() is not needed? Removed. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_proxy_declaration.tmpl (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_proxy_declaration.tmpl:14: void set_group_controller(mojo::AssociatedGroupController* group_controller) { On 2016/11/15 18:53:46, yzshen1 wrote: > I think it is preferred to avoid passing raw pointer of ref-counted object as > input? > > We could use a scoped_refptr as input and then do group_controller_ = > std::move(group_controller) to avoid extra addref/release. > > WDYT? Sounds reasonable, done.
LGTM with one nit Thanks! https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/interface_ptr_state.h (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/interface_ptr_state.h:410: base::WeakPtrFactory<InterfacePtrState> weak_ptr_factory_; On 2016/11/15 19:36:38, Jay Civelli wrote: > On 2016/11/15 18:53:46, yzshen1 wrote: > > It is not needed in the majority case. Does it make sense to only construct it > > on demand? > > On Linux the size of a WeakPtrFactory is 16 bytes vs 8 for a unique_ptr. > I am not sure it's worth adding more code that lazily instanciates for that, but > maybe you have other concerns? Just wanted to make sure that the size/init cost has been considered. I don't have a very strong opinion. https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/thread_safe_interface_ptr.h (right): https://codereview.chromium.org/2498743002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/thread_safe_interface_ptr.h:57: On 2016/11/15 19:36:38, Jay Civelli wrote: > On 2016/11/15 18:53:46, yzshen1 wrote: > > Do we need to also expose connection error handler? > > That's a good question. > Happy to add them to ThreadSafeInterfacePointer if you believe it makes sense. > > From my point of view, I wonder if the connection error handlers should be set > on the original InterfacePtr before it gets passed to the > ThreadSafeInterfacePointer. > My thought is just for clarity reasons: with the InterfacePtr it's clear > everything happens on the same thread. > With ThreadSafeInterfacePointer, the handlers would still be invoked on the > InterfacePtr's thread, but it might not be immediately clear to consumers. > > Yeah. I think that makes sense. https://codereview.chromium.org/2498743002/diff/100001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl (right): https://codereview.chromium.org/2498743002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:205: callback, group_controller_.get()); remove .get()?
The CQ bit was checked by jcivelli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2498743002/#ps120001 (title: "Mojo: introducing a thread safe interface pointer.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2498743002/diff/100001/mojo/public/tools/bind... File mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl (right): https://codereview.chromium.org/2498743002/diff/100001/mojo/public/tools/bind... mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:205: callback, group_controller_.get()); On 2016/11/15 20:26:15, yzshen1 wrote: > remove .get()? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jcivelli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jcivelli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Creating a thread safe interface pointer that is tied to an actual InterfacePtr and forwards message from interface calls to the InterfacePtr thread and forwards back response messages to the calling thread. BUG=664628 TEST=InterfacePtrTest.TestThreadSafeInterfacePointer ========== to ========== Creating a thread safe interface pointer that is tied to an actual InterfacePtr and forwards message from interface calls to the InterfacePtr thread and forwards back response messages to the calling thread. BUG=664628 TEST=InterfacePtrTest.TestThreadSafeInterfacePointer ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Creating a thread safe interface pointer that is tied to an actual InterfacePtr and forwards message from interface calls to the InterfacePtr thread and forwards back response messages to the calling thread. BUG=664628 TEST=InterfacePtrTest.TestThreadSafeInterfacePointer ========== to ========== Creating a thread safe interface pointer that is tied to an actual InterfacePtr and forwards message from interface calls to the InterfacePtr thread and forwards back response messages to the calling thread. BUG=664628 TEST=InterfacePtrTest.TestThreadSafeInterfacePointer Committed: https://crrev.com/64c7ea9ccda1354968015f4979135760627435e1 Cr-Commit-Position: refs/heads/master@{#432387} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/64c7ea9ccda1354968015f4979135760627435e1 Cr-Commit-Position: refs/heads/master@{#432387} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
