Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409173006:
Making DeviceCapabilities threadsafe with locking. (Closed)
|
Created:
5 years, 2 months ago by esum Modified:
5 years, 1 month ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking DeviceCapabilities threadsafe with locking.
Also updating interface due to thread safety concerns. Unit tests
have been modified to ensure that Observers and Validators can call
DeviceCapabilities methods safely without hitting deadlocks or
unexpected behavior.
TEST=cast_base_unittests
BUG=
Committed: https://crrev.com/e834718b8525eee99660f2c063cb556c0bb5c4b5
Cr-Commit-Position: refs/heads/master@{#357042}
Patch Set 1 #Patch Set 2 : New locking strategy. Locking refptr swap. #Patch Set 3 : Clean up header and implementation files. #Patch Set 4 : Clean up unit test. #
Total comments: 18
Patch Set 5 : #
Total comments: 16
Patch Set 6 : #Patch Set 7 : Make GetCapabilities accessors use ref counting. #Patch Set 8 : Just changing unit test assertions to new format. No other changes. #
Messages
Total messages: 39 (4 generated)
esum@chromium.org changed reviewers: + byungchul@chromium.org, halliwell@chromium.org, maclellant@chromium.org
This CL goes with https://eureka-internal-review.git.corp.google.com/#/c/40226
gunsch@chromium.org changed reviewers: + gunsch@chromium.org
It's not very Chromium-idiomatic to use locking (ever!). It can cause subtle slowdowns that are hard to debug. Typically classes in Chromium are made thread-safe by enforcing that they're only accessed on one thread. Is this possible to accomplish instead of this CL?
On 2015/10/22 00:42:55, gunsch wrote: > It's not very Chromium-idiomatic to use locking (ever!). It can cause subtle > slowdowns that are hard to debug. Typically classes in Chromium are made > thread-safe by enforcing that they're only accessed on one thread. Is this > possible to accomplish instead of this CL? Yes, I think it's possible to enforce access from only one thread, and it was considered as an alternative to locking. However, I want to understand the reasoning. Here are my thoughts: 1) From what I understand (I could be wrong), mutex's cause slowdowns when there is contention due to context switching and low-level system calls. However, for DeviceCapabilities, I don't anticipate there being a lot of contention, and locking/unlocking an uncontested mutex is relatively cheap (which I think will be the case most of the time). Also, given that the class will mainly be used for reads and just occasional writes, we could use a multi-read/single-write lock if needed. 2) I'm not sure if it's what you had in mind, but the alternative I know to locking is using callbacks: if (task runner doesn't belong to current thread) post a task to task runner to run on correct thread return The main use of DeviceCapabilities is for reads (returning values of capabilities). So if we use this callback pattern, I anticipate there being asynchronous returns through the class, which I think complicates the interface for client code compared to locking. 3) This class was meant to go hand-in-hand with DeviceInfo, which uses locking itself. Why is it OK for that class (which serves a similar purpose and in fact owns DeviceCapabilities) to use locking and not this one? Ultimately, I think the interface simplicity of using locking outweighs the performance cost, but I respect/trust your experience :) So if you really think I shouldn't use locking, I'll find an alternative.
On 2015/10/22 04:46:38, ericsum wrote: > On 2015/10/22 00:42:55, gunsch wrote: > > It's not very Chromium-idiomatic to use locking (ever!). It can cause subtle > > slowdowns that are hard to debug. Typically classes in Chromium are made > > thread-safe by enforcing that they're only accessed on one thread. Is this > > possible to accomplish instead of this CL? > > Yes, I think it's possible to enforce access from only one thread, and it was > considered as an alternative to locking. > However, I want to understand the reasoning. Here are my thoughts: > > 1) From what I understand (I could be wrong), mutex's cause slowdowns when there > is contention due to > context switching and low-level system calls. However, for DeviceCapabilities, I > don't anticipate there being a lot of > contention, and locking/unlocking an uncontested mutex is relatively cheap > (which I think will be the case > most of the time). Also, given that the class will mainly be used for reads and > just occasional writes, > we could use a multi-read/single-write lock if needed. > > 2) I'm not sure if it's what you had in mind, but the alternative I know to > locking is using callbacks: > > if (task runner doesn't belong to current thread) > post a task to task runner to run on correct thread > return > > The main use of DeviceCapabilities is for reads (returning values of > capabilities). So if we use this callback pattern, > I anticipate there being asynchronous returns through the class, which I think > complicates the interface for client code > compared to locking. > > 3) This class was meant to go hand-in-hand with DeviceInfo, which uses locking > itself. Why is it OK for that class > (which serves a similar purpose and in fact owns DeviceCapabilities) to use > locking and not this one? > > Ultimately, I think the interface simplicity of using locking outweighs the > performance cost, but I respect/trust your experience :) > So if you really think I shouldn't use locking, I'll find an alternative. The short answer is I don't think DeviceInfo should be using locking either. But I think that one is more definitely accessed from multiple threads, so that would probably be hard to untangle. So that's a good point that if this is largely just backing DeviceInfo, then locking might be the simplest solution. I mostly just wanted to mechanically push back and see if there was a non-locking solution available / considered. The typical alternative isn't necessarily asynchrononus. Obviously you can use PostTaskAndReply but that (a) makes everything asynchronous and (b) could introduce its own problems depending on which thread you're posting to. So the answer is necessarily "it depends on the situation". The approach for data classes like this usually involves finding ways to only call into it from one thread per instance, though. So another solution might be if it were possible to easily build multiple DeviceCapabilities instances, all of which have the same data, but which are individually used on one thread only.
On 2015/10/22 14:09:00, gunsch wrote: > On 2015/10/22 04:46:38, ericsum wrote: > > On 2015/10/22 00:42:55, gunsch wrote: > > > It's not very Chromium-idiomatic to use locking (ever!). It can cause subtle > > > slowdowns that are hard to debug. Typically classes in Chromium are made > > > thread-safe by enforcing that they're only accessed on one thread. Is this > > > possible to accomplish instead of this CL? > > > > Yes, I think it's possible to enforce access from only one thread, and it was > > considered as an alternative to locking. > > However, I want to understand the reasoning. Here are my thoughts: > > > > 1) From what I understand (I could be wrong), mutex's cause slowdowns when > there > > is contention due to > > context switching and low-level system calls. However, for DeviceCapabilities, > I > > don't anticipate there being a lot of > > contention, and locking/unlocking an uncontested mutex is relatively cheap > > (which I think will be the case > > most of the time). Also, given that the class will mainly be used for reads > and > > just occasional writes, > > we could use a multi-read/single-write lock if needed. > > > > 2) I'm not sure if it's what you had in mind, but the alternative I know to > > locking is using callbacks: > > > > if (task runner doesn't belong to current thread) > > post a task to task runner to run on correct thread > > return > > > > The main use of DeviceCapabilities is for reads (returning values of > > capabilities). So if we use this callback pattern, > > I anticipate there being asynchronous returns through the class, which I think > > complicates the interface for client code > > compared to locking. > > > > 3) This class was meant to go hand-in-hand with DeviceInfo, which uses locking > > itself. Why is it OK for that class > > (which serves a similar purpose and in fact owns DeviceCapabilities) to use > > locking and not this one? > > > > Ultimately, I think the interface simplicity of using locking outweighs the > > performance cost, but I respect/trust your experience :) > > So if you really think I shouldn't use locking, I'll find an alternative. > > The short answer is I don't think DeviceInfo should be using locking either. But > I think that one is more definitely accessed from multiple threads, so that > would probably be hard to untangle. So that's a good point that if this is > largely just backing DeviceInfo, then locking might be the simplest solution. > > I mostly just wanted to mechanically push back and see if there was a > non-locking solution available / considered. > > The typical alternative isn't necessarily asynchrononus. Obviously you can use > PostTaskAndReply but that (a) makes everything asynchronous and (b) could > introduce its own problems depending on which thread you're posting to. So the > answer is necessarily "it depends on the situation". The approach for data > classes like this usually involves finding ways to only call into it from one > thread per instance, though. So another solution might be if it were possible to > easily build multiple DeviceCapabilities instances, all of which have the same > data, but which are individually used on one thread only. I'm assuming that "all of which have the same data" means that each DeviceCapabilities instance contains its own copy of same data content. The natural question then is when one instance gets written to, how does that write get propagated to all the different instances? If there is a single master DeviceCapabilities instance that gets reads/writes and all other instances (slaves) are read-only, then I think this is feasible. When the master gets written to, it would then notify all the slaves of the update. The update in each slave would be asynchronous and done on the owning thread (through the PostTask callback pattern). This would be fine because SetCapability() is already asynchronous, and it would avoid the need for locking since reads/writes occur on the same thread for each instance. We would need to be SURE though that capability writes will only occur on one thread (the Main UI thread). If writes can occur on all instances/threads, I think it would be doable but complicated to keep them all in sync because we could run into cases where we have concurrent writes to the same capability from multiple threads and we would have to resolve somehow. For reference, in the short term, DeviceCapabilities will have read/writes on Main UI thread and reads from SetupManager's API thread. This may expand in the future though. Byungchul/Tavis, what are your thoughts?
> I'm assuming that "all of which have the same data" means that each > DeviceCapabilities instance contains its own copy of same data content. The > natural question then is when one instance gets written to, how does that write > get propagated to all the different instances? If there is a single master > DeviceCapabilities instance that gets reads/writes and all other instances > When the master gets written to, it would then notify all the slaves of the > update. The update in each slave would be asynchronous and done on the owning > thread (through the PostTask callback pattern). This would be fine because > SetCapability() is already asynchronous, and it would avoid the need for locking > since reads/writes occur on the same thread for each instance. > > We would need to be SURE though that capability writes will only occur on one > thread (the Main UI thread). If writes can occur on all instances/threads, I > think it would be doable but complicated to keep them all in sync because we > could run into cases where we have concurrent writes to the same capability from > multiple threads and we would have to resolve somehow. > > For reference, in the short term, DeviceCapabilities will have read/writes on > Main UI thread and reads from SetupManager's API thread. This may expand in the > future though. > > Byungchul/Tavis, what are your thoughts? Interesting idea. We could wrap up all the getters that we want to be synchronous into a separate class called DeviceCapabilitiesGetter that listens for DeviceCapabilities to change on its main thread, and then it caches the capabilities values and guards it with locks. This would essentially work the same as the current implementation, but it splits up the validator and updating logic into DeviceCapabilities using async API, and exposes quick access to current capabilities with new getter class that uses sync API. This doesn't get rid of the locking, but it cleans up the class so we're not mixing locking and threading, and isolates the locking to a minimum.
One other idea I suggested to Eric over chat would be to have a given instance of DeviceCapabilities update itself by registering for config updates. e.g. whenever config download occurs, it gets a notification, then updates values on the thread that it was instantiated on. So then you'd have multiple instances of DeviceCapabilities as needed. However, that does make a dependency from DeviceCapabilities to some of the cast APIs that aren't upstream yet.
On 2015/10/22 18:00:16, gunsch wrote: > One other idea I suggested to Eric over chat would be to have a given instance > of DeviceCapabilities update itself by registering for config updates. e.g. > whenever config download occurs, it gets a notification, then updates values on > the thread that it was instantiated on. > > So then you'd have multiple instances of DeviceCapabilities as needed. > > However, that does make a dependency from DeviceCapabilities to some of the cast > APIs that aren't upstream yet. Ya, that would also limit who can actually change capabilities. The current design is that anybody (config downloader, command line flags, user specified actions) can try and set the capability values, and the Validators will make sure nothing stupid happens. We want to retain that ability and not limit only to certain observers that we listen for. Although this approach and the previously suggested approaches are very similar. They all involve a core class that runs on one thread that keeps the up-to-date capabiltiies values and works asynchronously with Validators. Then possibly many listener classes are created on separate threads that listen for updates from core class as the capabilities change. My opinion is the "listeners" can be a single class that uses locking to simplify the API, but I'm open to several listener instances on each thread.
Ready for review again. Implemented Byungchul's locked pointer swap idea.
Pinging for review when you guys have a chance. Thanks.
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:188: scoped_ptr<base::DictionaryValue> DeviceCapabilitiesImpl::GetCapabilities() wrap line after scoped<> https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:219: base::Unretained(this), path, It is not safe to use Unretained() here. This one can be destroyed before this task runs. https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } Lock twice doesn't look great. What about using a weak pointer for validator, and posting task to Validator::Validate() directly above? class ValidatorInfo : base::SupportsWeakPtr { ... void Validate(...) { DCHECK(task_runner->BelongsToCurrentThread()); validator->Validate(...); } ... }; void SetCapability() { ... task_runner->PostTask(FROM_HERE, base::Bind(&ValidatorInfo::Validate, validator_info.GetWeakPtr(), ...); ... } https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.h (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:54: const scoped_ptr<const T> data; This struct is not copyable. Please make this a class. And, a simple getter instead of direct member var access.
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } On 2015/10/27 20:38:22, byungchul wrote: > Lock twice doesn't look great. What about using a weak pointer for validator, > and posting task to Validator::Validate() directly above? > > class ValidatorInfo : base::SupportsWeakPtr { > ... > void Validate(...) { > DCHECK(task_runner->BelongsToCurrentThread()); > validator->Validate(...); > } > ... > }; > > void SetCapability() { > ... > task_runner->PostTask(FROM_HERE, base::Bind(&ValidatorInfo::Validate, > validator_info.GetWeakPtr(), ...); > ... > } > > Oh wow didn't think of that. I like this a lot more. Much cleaner. I think this also will resolve comment above about using Unretained().
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:216: validator_it->second.task_runner->PostTask( You can move this outside the lock once you have the Validator pointer. https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } On 2015/10/27 20:38:22, byungchul wrote: > Lock twice doesn't look great. What about using a weak pointer for validator, > and posting task to Validator::Validate() directly above? Would there be any concerns with accessing the weak_ptr on the wrong thread? SetCapability() will run on the capabilities thread, but the weak_ptr needs to be created and dereferenced on the validator thread, right? https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:323: capabilities_.swap(new_capabilities); Since you always update these two fields together, would it make sense to bundle them together as a single ref-counted object?
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } On 2015/10/27 21:00:32, maclellant wrote: > On 2015/10/27 20:38:22, byungchul wrote: > > Lock twice doesn't look great. What about using a weak pointer for validator, > > and posting task to Validator::Validate() directly above? > > Would there be any concerns with accessing the weak_ptr on the wrong thread? > SetCapability() will run on the capabilities thread, but the weak_ptr needs to > be created and dereferenced on the validator thread, right? > You can get weak_ptr on any thread, but can access or destroy weak_ptr only on a specific thread. So, GetWeakPtr() is fine, but not for *weak_ptr or weak_ptr.get().
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:216: validator_it->second.task_runner->PostTask( On 2015/10/27 21:00:32, maclellant wrote: > You can move this outside the lock once you have the Validator pointer. If you cache the ValidatorInfo reference and release the lock, couldn't another thread call Unregister() for that Validator and destroy it concurrently, resulting in a race? https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } On 2015/10/27 21:00:32, maclellant wrote: > On 2015/10/27 20:38:22, byungchul wrote: > > Lock twice doesn't look great. What about using a weak pointer for validator, > > and posting task to Validator::Validate() directly above? > > Would there be any concerns with accessing the weak_ptr on the wrong thread? > SetCapability() will run on the capabilities thread, but the weak_ptr needs to > be created and dereferenced on the validator thread, right? > Yes SetCapability() will run on the capabilities thread, but isn't it posting a task to the validator thread (where the weak_ptr will be dereferenced). I think this is fine because the validator thread is where the ValidatorInfo instance gets created (Register()) and destroyed (Unregister()). https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:323: capabilities_.swap(new_capabilities); On 2015/10/27 21:00:32, maclellant wrote: > Since you always update these two fields together, would it make sense to bundle > them together as a single ref-counted object? Sure, SGTM.
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:216: validator_it->second.task_runner->PostTask( On 2015/10/27 21:14:25, ericsum wrote: > On 2015/10/27 21:00:32, maclellant wrote: > > You can move this outside the lock once you have the Validator pointer. > > If you cache the ValidatorInfo reference and release the lock, couldn't another > thread call Unregister() for that Validator and destroy it concurrently, > resulting in a race? Well before you were posting unretained pointer and checking again, so there wouldn't be any race. But if you are switching to weak_ptr implementation then it would have to stay inside the lock. https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } > You can get weak_ptr on any thread, but can access or destroy weak_ptr only on a > specific thread. So, GetWeakPtr() is fine, but not for *weak_ptr or > weak_ptr.get(). +1 Got it, wasn't sure if weak_ptr was accessed on every access or only PostTask, looks like the latter. Also I was concerned about invalidation, but if ValidatorInfo is only ever destroyed in Unregister running on validator thread that is OK.
https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:188: scoped_ptr<base::DictionaryValue> DeviceCapabilitiesImpl::GetCapabilities() On 2015/10/27 20:38:22, byungchul wrote: > wrap line after scoped<> Done. https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:253: } On 2015/10/27 21:20:07, maclellant wrote: > > You can get weak_ptr on any thread, but can access or destroy weak_ptr only on > a > > specific thread. So, GetWeakPtr() is fine, but not for *weak_ptr or > > weak_ptr.get(). > > +1 > > Got it, wasn't sure if weak_ptr was accessed on every access or only PostTask, > looks like the latter. Also I was concerned about invalidation, but if > ValidatorInfo is only ever destroyed in Unregister running on validator thread > that is OK. Done. https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:323: capabilities_.swap(new_capabilities); On 2015/10/27 21:14:25, ericsum wrote: > On 2015/10/27 21:00:32, maclellant wrote: > > Since you always update these two fields together, would it make sense to > bundle > > them together as a single ref-counted object? > > Sure, SGTM. Done. https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.h (right): https://codereview.chromium.org/1409173006/diff/60001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:54: const scoped_ptr<const T> data; On 2015/10/27 20:38:22, byungchul wrote: > This struct is not copyable. Please make this a class. And, a simple getter > instead of direct member var access. Done.
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; Can we avoid deep copy? Why not use ref-count here as well? https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:152: delete validator_it->second; Use base::ScopedPtrHashMap https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:57: const std::string* json_string() const { return json_string_.get(); } Can they be null? Otherwise, const& https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:77: base::SingleThreadTaskRunner* task_runner() const { Please comment why it is not a scoped_refptr https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:114: // 2) capabilities_lock_ Any chances to lock both at a time?
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; On 2015/10/28 01:09:43, byungchul wrote: > Can we avoid deep copy? Why not use ref-count here as well? Hmm. We can, but I can't think of a way to do that without somehow exposing the internal wrapper class that makes base::DictionaryValue RefCountedThreadSafe. I guess we could expose that wrapper publicly, but I personally don't think it's very clean. https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.cc (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.cc:152: delete validator_it->second; On 2015/10/28 01:09:43, byungchul wrote: > Use base::ScopedPtrHashMap Thanks didn't know about this. Just what I need. Done. https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:57: const std::string* json_string() const { return json_string_.get(); } On 2015/10/28 01:09:43, byungchul wrote: > Can they be null? Otherwise, const& Done. https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:77: base::SingleThreadTaskRunner* task_runner() const { On 2015/10/28 01:09:43, byungchul wrote: > Please comment why it is not a scoped_refptr Done. Was just trying to avoid a call to AddRef(), and const scoped_refptr<base::SingleThreadTaskRunner>& looked too verbose. https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:114: // 2) capabilities_lock_ On 2015/10/28 01:09:43, byungchul wrote: > Any chances to lock both at a time? Not currently. Just put this here as precaution/reminder if it came up. Removing it.
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; On 2015/10/28 04:16:27, ericsum wrote: > On 2015/10/28 01:09:43, byungchul wrote: > > Can we avoid deep copy? Why not use ref-count here as well? > > Hmm. We can, but I can't think of a way to do that without somehow exposing the > internal wrapper class that makes base::DictionaryValue RefCountedThreadSafe. I > guess we could expose that wrapper publicly, but I personally don't think it's > very clean. Can you check how capabilities are used? If deep copy happens once per hours, I am fine with deep copy with comments that why deep copy is not that a problem. Otherwise, you may have to define a kind of CapabilitityGetter or Accessor which holds scoped_refprt capabilites and defines these GetFunctions. https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities_impl.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities_impl.h:77: base::SingleThreadTaskRunner* task_runner() const { On 2015/10/28 04:16:27, ericsum wrote: > On 2015/10/28 01:09:43, byungchul wrote: > > Please comment why it is not a scoped_refptr > > Done. Was just trying to avoid a call to AddRef(), and const > scoped_refptr<base::SingleThreadTaskRunner>& looked too verbose. It depends. Sometimes returning ref-counted object without scoped_refptr has an edge case when it is destroy after function returns since it doesn't increase ref-count. But, in this case, it would be fine because ValidatorInfo holds scoped_refptr and task_runner is accessed only within lock and ValidatorInfo cannot be destroyed when task_runner is accessed.
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; On 2015/10/28 16:04:18, byungchul wrote: > On 2015/10/28 04:16:27, ericsum wrote: > > On 2015/10/28 01:09:43, byungchul wrote: > > > Can we avoid deep copy? Why not use ref-count here as well? > > > > Hmm. We can, but I can't think of a way to do that without somehow exposing > the > > internal wrapper class that makes base::DictionaryValue RefCountedThreadSafe. > I > > guess we could expose that wrapper publicly, but I personally don't think it's > > very clean. > > Can you check how capabilities are used? If deep copy happens once per hours, I > am fine with deep copy with comments that why deep copy is not that a problem. > Otherwise, you may have to define a kind of CapabilitityGetter or Accessor which > holds scoped_refprt capabilites and defines these GetFunctions. GetCapabilities() gets called in EndPointWithApplication::SendReadyToReceiver(), which I think gets called once right after application gets launched? So I think it's not necessarily once per hours. But it's also not very frequent. GetCapabilitiesString() gets called only when capabilities are written through CastReceiverImpl::OnCapabilitiesChanged(). GetCapability() currently is not getting called. What do you think?
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; On 2015/10/28 17:58:50, ericsum wrote: > On 2015/10/28 16:04:18, byungchul wrote: > > On 2015/10/28 04:16:27, ericsum wrote: > > > On 2015/10/28 01:09:43, byungchul wrote: > > > > Can we avoid deep copy? Why not use ref-count here as well? > > > > > > Hmm. We can, but I can't think of a way to do that without somehow exposing > > the > > > internal wrapper class that makes base::DictionaryValue > RefCountedThreadSafe. > > I > > > guess we could expose that wrapper publicly, but I personally don't think > it's > > > very clean. > > > > Can you check how capabilities are used? If deep copy happens once per hours, > I > > am fine with deep copy with comments that why deep copy is not that a problem. > > Otherwise, you may have to define a kind of CapabilitityGetter or Accessor > which > > holds scoped_refprt capabilites and defines these GetFunctions. > > GetCapabilities() gets called in EndPointWithApplication::SendReadyToReceiver(), > which I think gets called once right after application gets launched? So I think > it's not necessarily once per hours. But it's also not very frequent. > > GetCapabilitiesString() gets called only when capabilities are written through > CastReceiverImpl::OnCapabilitiesChanged(). > > GetCapability() currently is not getting called. > > What do you think? It would be nice to mention that these api are not expected to be called frequently, otherwise need better approach.
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; On 2015/10/28 18:37:16, byungchul wrote: > On 2015/10/28 17:58:50, ericsum wrote: > > On 2015/10/28 16:04:18, byungchul wrote: > > > On 2015/10/28 04:16:27, ericsum wrote: > > > > On 2015/10/28 01:09:43, byungchul wrote: > > > > > Can we avoid deep copy? Why not use ref-count here as well? > > > > > > > > Hmm. We can, but I can't think of a way to do that without somehow > exposing > > > the > > > > internal wrapper class that makes base::DictionaryValue > > RefCountedThreadSafe. > > > I > > > > guess we could expose that wrapper publicly, but I personally don't think > > it's > > > > very clean. > > > > > > Can you check how capabilities are used? If deep copy happens once per > hours, > > I > > > am fine with deep copy with comments that why deep copy is not that a > problem. > > > Otherwise, you may have to define a kind of CapabilitityGetter or Accessor > > which > > > holds scoped_refprt capabilites and defines these GetFunctions. > > > > GetCapabilities() gets called in > EndPointWithApplication::SendReadyToReceiver(), > > which I think gets called once right after application gets launched? So I > think > > it's not necessarily once per hours. But it's also not very frequent. > > > > GetCapabilitiesString() gets called only when capabilities are written through > > CastReceiverImpl::OnCapabilitiesChanged(). > > > > GetCapability() currently is not getting called. > > > > What do you think? > > It would be nice to mention that these api are not expected to be called > frequently, otherwise need better approach. Hmm, I'm not happy leaving a comment like that. Let me think about a better approach.
https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... File chromecast/base/device_capabilities.h (right): https://codereview.chromium.org/1409173006/diff/80001/chromecast/base/device_... chromecast/base/device_capabilities.h:155: virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0; I can think there are 2 straightforward ways of doing this: 1) Expose a public class nested in DeviceCapabilities like this: class DictionaryReference { public: const DictionaryValue& Get() { return capabilities_data_->dictionary(); } private: friend class DeviceCapabilitiesImpl; DictionaryReference(const scoped_refptr<ImmutableCapabilitiesData>& capabilities_data) : capabilities_data_(capabilities_data) {} scoped_refptr<ImmutableCapabilitiesData> capabilities_data_; } The interface/implementation would look like: DictionaryReference DeviceCapabilitiesImpl::GetCapabilities() { scoped_refptr<ImmutableCapabilitiesData> capabilities_data_ref = GetCapabilitiesDataReference(); return DictionaryReference(capabilities_data_ref); } This way we are hiding the internal ImmutableCapabilitiesData wrapper class, and there's just a simple public way of accessing returned the dictionary reference. We would do same for GetCapabilitiesString(). 2) In both uses of GetCapabilities() and GetCapabilitiesString(), the client code is performing a deep copy anyways once it gets the return value. With this interface as it is now, there are 2 deep copies (one to receive return value, then once again in client code). We can bring this back down to 1 deep copy by making the interface: void DeviceCapabilitiesImpl::LoadCapabilities(scoped_ptr<DictionaryValue>* destination) { DCHECK(destination); scoped_refptr<ImmutableCapabilitiesData> capabilities_data_ref = GetCapabilitiesDataReference(); *destination = capabilities_data_ref->dictionary().CreateDeepCopy(); } This way we are doing a deep copy but at least deep copying into the ultimate destination. This would bring us back to the same performance as we had before we made any of these changes. Which if any of these do you prefer?
> Which if any of these do you prefer? I prefer #1, since it actually allows querying capabilities dictionary and string with zero copies, and it's up to the caller to perform any deep copy if necessary. Rather than DictionaryReference I would call the struct CapabilitiesData. When the caller calls GetData(), it will return a new instance of this struct that internally holds onto the immutable ref-ptr. The caller can query this data without performing any copies, and the data will always be valid even if the capabilities is later updated.
On 2015/10/28 20:11:17, maclellant wrote: > > Which if any of these do you prefer? > > I prefer #1, since it actually allows querying capabilities dictionary and > string with zero copies, and it's up to the caller to perform any deep copy if > necessary. > Rather than DictionaryReference I would call the struct CapabilitiesData. When > the caller calls GetData(), it will return a new instance of this struct that > internally holds onto the immutable ref-ptr. The caller can query this data > without performing any copies, and the data will always be valid even if the > capabilities is later updated. Either way is fine to me. As Tavis said, #1 doesn't any copy while it introduces extra hierarchy.
On 2015/10/28 20:26:03, byungchul wrote: > On 2015/10/28 20:11:17, maclellant wrote: > > > Which if any of these do you prefer? > > > > I prefer #1, since it actually allows querying capabilities dictionary and > > string with zero copies, and it's up to the caller to perform any deep copy if > > necessary. > > Rather than DictionaryReference I would call the struct CapabilitiesData. When > > the caller calls GetData(), it will return a new instance of this struct that > > internally holds onto the immutable ref-ptr. The caller can query this data > > without performing any copies, and the data will always be valid even if the > > capabilities is later updated. > > Either way is fine to me. As Tavis said, #1 doesn't any copy while it introduces > extra hierarchy. OK I'm going with #1. I was leaning that way also.
Discussed offline. Decided to just move ImmutableCapabilitiesData to public part DeviceCapabilities and make it part of the interface (with simpler name). Wrapping scoped_refptr<ImmutableCapabilitiesData> into a class would have hidden the fact that we were using refptrs, but I thought it was too verbose when I coded it. The simplicity/cleanliness of making ImmutableCapabilitiesData public outweighed the negative of exposing the refptr implementation detail.
lgtm
Jesse or Luke, can I get review/committer's stamp on this when you have a chance please? I got Byungchul's permission to submit (he's OOO), and I'll do follow-up CL if he has more comments.
rs lgtm, only took a cursory look
The CQ bit was checked by esum@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maclellant@chromium.org Link to the patchset: https://codereview.chromium.org/1409173006/#ps140001 (title: "Just changing unit test assertions to new format. No other changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409173006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409173006/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e834718b8525eee99660f2c063cb556c0bb5c4b5 Cr-Commit-Position: refs/heads/master@{#357042}
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
