Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1297093004:
Enable debug builds in the Android framework (Closed)
DescriptionEnable debug builds in the Android framework
Committed: https://skia.googlesource.com/skia/+/ab5d5de420503018b385b73e91b24812916c2080
Patch Set 1 #
Total comments: 6
Patch Set 2 : add more comments #Messages
Total messages: 21 (6 generated)
djsollen@google.com changed reviewers: + bungeman@google.com, reed@google.com
reed@google.com changed reviewers: + mtklein@google.com
mike and ben should possibly chat about this w/ you
I'm still thinking about this, and all the other people who want to do similar things. https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:77: if (1 == sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel)) { Not that I'm advocating this, but as a note to the future, what seems to be wanted (and what has been desired before) is something like int prev = sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel); if (2 == prev) { this->become_unique(); } else if (1 == prev) { this->internal_dispose(); } where become_unique() is a protected method. The idea is to allow factory caches to write wrappers that do something when they are the only owners. This is essentially what the Android code is doing, which is using the current internal_dispose() as the proposed 'become_unique()', and using unique_ptr as the actual 'internal_dispose()'. This is something like allowing weak reference counting, but with only one weak reference (holdable by the factory).
https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:35: fRefCnt = 0; // illegal value, to catch us if we reuse after delete This check may have less weight now that we use valgrind and the various *sans. However, if we have a change like this, maybe this should be changed to -1?
https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:35: fRefCnt = 0; // illegal value, to catch us if we reuse after delete On 2015/08/20 22:18:13, bungeman1 wrote: > This check may have less weight now that we use valgrind and the various *sans. > However, if we have a change like this, maybe this should be changed to -1? Changing SkRefCnt to support start at 1, 0 means disposed, -1 means checked destructed, all goes well until unique(). If unique() can return true for a reference count of 0 or 1 things go haywire, and for good reason. In the case where a factory is holding the SkRefCnt object without holding a ref itself (so that it can be notified when there are no outstanding refs) it cannot legally ref(), except from 0 to 1. The reason for this is that it has, effectively, stolen unique(). Even the managed type itself cannot use unique() on itself, as even if unique() returns true, the cache may still (against the API specification) ref its bare pointer to the object (without holding a valid reference). As a result, no one other that the cache can use unique(), including the SkRefCnt class' implementation. https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:77: if (1 == sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel)) { On 2015/08/20 21:55:13, bungeman1 wrote: > Not that I'm advocating this, but as a note to the future, what seems to be > wanted (and what has been desired before) is something like > > int prev = sk_atomic_fetch_add(&fRefCnt, -1, sk_memory_order_acq_rel); > if (2 == prev) { > this->become_unique(); > } else if (1 == prev) { > this->internal_dispose(); > } > > where become_unique() is a protected method. The idea is to allow factory caches > to write wrappers that do something when they are the only owners. This is > essentially what the Android code is doing, which is using the current > internal_dispose() as the proposed 'become_unique()', and using unique_ptr as > the actual 'internal_dispose()'. This is something like allowing weak reference > counting, but with only one weak reference (holdable by the factory). Note that 'become_unique()' cannot work correctly, as it would need to exclude competing refs. In other words, internal_dispose() works because the current thread was the only owning thread, but become_unique() cannot work because it is running on the thread which just gave up the last shared reference, but the remaining reference may be in the process of grabbing another reference. The code in Android gets around this by using a full mutex around this last reference.
https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:62: // go to zero, but not below, prior to reusing the object. So after much back and forth and looking into this, SkRefCnt can't really support what they're trying to do without something breaking. I'm ok with this change for now if we add to this comment // This breaks the use of unique() on such objects. // This should be removed once the Android code is fixed.
https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): https://codereview.chromium.org/1297093004/diff/1/include/core/SkRefCnt.h#new... include/core/SkRefCnt.h:62: // go to zero, but not below, prior to reusing the object. On 2015/08/21 21:44:35, bungeman1 wrote: > So after much back and forth and looking into this, SkRefCnt can't really > support what they're trying to do without something breaking. I'm ok with this > change for now if we add to this comment > > // This breaks the use of unique() on such objects. > // This should be removed once the Android code is fixed. Done.
lgtm
reed@google.com changed required reviewers: + bungeman@google.com, mtklein@google.com, reed@google.com
If we don't hang together, by Heavens we shall hang separately lgtm
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1297093004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1297093004/20001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-08-24 20:52 UTC
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer from https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS
Sigh. lgtm
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1297093004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1297093004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/ab5d5de420503018b385b73e91b24812916c2080 |
