PSA: base::NonThreadSafe will no longer enforce thread-affinity

[Gabriel Charette]

tl;dr; Thread-safety is provided by sequences (base::SequenceChecker) and doesn't require thread-affinity (base::ThreadChecker). I will make that change in the coming days.

Most classes in Chromium merely care about thread-safety, not thread-affinity. Most classes use thread-affine checks today for legacy reasons (until now, things mostly ran on dedicated threads where thread-safety == thread-affinity). But this is changing and having thread-affine checks is slowing the progress of migrating code to run on TaskScheduler sequences.

If your class truly requires thread-affinity (i.e. uses thread-local storage or third-party APIs that are thread-affine), update it to use a base::ThreadChecker (and make sure to add an in-code comment so others know why when running into that check in the future).

Thanks,

Gab

[Ryan Sleevi]

On Thu, Apr 20, 2017 at 12:55 PM, Gabriel Charette <g...@chromium.org> wrote:

tl;dr; Thread-safety is provided by sequences (base::SequenceChecker) and doesn't require thread-affinity (base::ThreadChecker). I will make that change in the coming days.

Most classes in Chromium merely care about thread-safety, not thread-affinity. Most classes use thread-affine checks today for legacy reasons (until now, things mostly ran on dedicated threads where thread-safety == thread-affinity). But this is changing and having thread-affine checks is slowing the progress of migrating code to run on TaskScheduler sequences.

Can you clarify what, particularly, is slowing things down?

From reading this message, it sounds like you're saying the process of ensuring code intended thread-affine versus sequence-affine is slowing things down. The proposed solution is to assume things are all sequence-affine, and that everyone should check code to see if it meant thread-affine. That doesn't sound like a very scalable/safe approach, although it is consistent with moving quickly (even if things break), but perhaps I've misunderstood the message?

 

If your class truly requires thread-affinity (i.e. uses thread-local storage or third-party APIs that are thread-affine), update it to use a base::ThreadChecker (and make sure to add an in-code comment so others know why when running into that check in the future).

One concern here is that it assumes there is someone who is responsible for every line of code/every class, and that within "the coming days" will inspect all such classes to ensure they're properly thread-affine versus sequence-affine. I don't think that assumption holds - that is, there's a non-trivial amount of code that might be best phrased as "in maintenance mode," so do you have a plan to address that?

Or is the supposition that if it truly was in maintenance mode, it would have a sufficient number of tests that ensure all of its necessary preconditions, and as a consequence, this change is safe to make? Would these changes perhaps change the precondition testing of those tests, and thus allow them to pass, even though the precondition is now violated?

I highlight all of this because historically we've seemed to avoid changing such core invariants of classes, and instead introduced transition classes, migrated callers as appropriate, and then deprecated the existing class (and/or updated to the 'alternative' new class). This was certainly true with things like the introduction of MessageLoop/MessageLoopProxy and then later, the introduction of the TaskRunner. Similarly, base::SequenceChecker and base::ThreadChecker provide difference invariants, with the former being introduced and having converted many (but not all) of the latter's users over.

Given that we're talking thread-safety, the issues it causes may be subtle and difficult to attribute to this change, so it seems there is higher-than-normal risk, and less-than-usual assurance of its safety. 

[Wez]

I do not think we should simply change the behaviour of base::NonThreadSafe to mean non-sequence-safe.

Although most code using base::NonThreadSafe really just cares about sequence affinity, it is the case that there are special cases where actual thread affinity is important, especially in platform-specific code (e.g. various operations on Windows' window handles must be performed on the thread to which the handle is bound).  For cases like that you might reasonable assert sequence affinity, and separately assert that the sequence in question is actually a thread, but that doesn't sound like what you're describing?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACvaWvZ1hkRdmpbj_nKftkdGOjsWAxuUH9mTo%2BjREd5AEcTrNA%40mail.gmail.com.

[Gabriel Charette]

On Thu, Apr 20, 2017 at 1:46 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, Apr 20, 2017 at 12:55 PM, Gabriel Charette <g...@chromium.org> wrote:

tl;dr; Thread-safety is provided by sequences (base::SequenceChecker) and doesn't require thread-affinity (base::ThreadChecker). I will make that change in the coming days.

Most classes in Chromium merely care about thread-safety, not thread-affinity. Most classes use thread-affine checks today for legacy reasons (until now, things mostly ran on dedicated threads where thread-safety == thread-affinity). But this is changing and having thread-affine checks is slowing the progress of migrating code to run on TaskScheduler sequences.

Can you clarify what, particularly, is slowing things down?

From reading this message, it sounds like you're saying the process of ensuring code intended thread-affine versus sequence-affine is slowing things down. The proposed solution is to assume things are all sequence-affine, and that everyone should check code to see if it meant thread-affine. That doesn't sound like a very scalable/safe approach, although it is consistent with moving quickly (even if things break), but perhaps I've misunderstood the message?

I said it's slowing the migration of code to run on TaskScheduler sequences (not that it's affecting product performance) -- i.e. it forces sequence-friendly components to request a SingleThreadTaskRunner instead of a SequencedTaskRunner from the TaskScheduler because a leaf deep down its dependency tree -- inherited from its members -- incorrectly uses a thread-affine check when it merely needs thread-safety.

Until most things are sequence-friendly, top-level components sadly can't be executed on sequences despite having no real thread-affinity requirements...

 

 

If your class truly requires thread-affinity (i.e. uses thread-local storage or third-party APIs that are thread-affine), update it to use a base::ThreadChecker (and make sure to add an in-code comment so others know why when running into that check in the future).

One concern here is that it assumes there is someone who is responsible for every line of code/every class, and that within "the coming days" will inspect all such classes to ensure they're properly thread-affine versus sequence-affine. I don't think that assumption holds - that is, there's a non-trivial amount of code that might be best phrased as "in maintenance mode," so do you have a plan to address that?

Or is the supposition that if it truly was in maintenance mode, it would have a sufficient number of tests that ensure all of its necessary preconditions, and as a consequence, this change is safe to make? Would these changes perhaps change the precondition testing of those tests, and thus allow them to pass, even though the precondition is now violated?

I highlight all of this because historically we've seemed to avoid changing such core invariants of classes, and instead introduced transition classes, migrated callers as appropriate, and then deprecated the existing class (and/or updated to the 'alternative' new class). This was certainly true with things like the introduction of MessageLoop/MessageLoopProxy and then later, the introduction of the TaskRunner. Similarly, base::SequenceChecker and base::ThreadChecker provide difference invariants, with the former being introduced and having converted many (but not all) of the latter's users over.

Given that we're talking thread-safety, the issues it causes may be subtle and difficult to attribute to this change, so it seems there is higher-than-normal risk, and less-than-usual assurance of its safety. 

Right, the original approach I was considering was indeed to introduce a new transition class and mass shard reviews for owners as a force function to look at their code and confirm it's not thread-affine (using thread-local storage or some other form of thread ID based mapping).

But then I figured we might be able to move faster (just do it). The goal isn't just to skip a step, code is migrated to TaskScheduler as we speak and every component which is forced to request a SingleThreadTaskRunner instead of SequencedTaskRunner because of a bogus check is hurting the overall system by encoding an unnecessary requirement (TaskScheduler is built with the idea that thread-affinity is the odd case -- i.e. it creates one thread per SingleThreadTR instance...).

Sequences are effectively Chromium-controlled user-threads and most classes shouldn't care about the difference (and I hope none that do encoded that through base::NonThreadSafe which is awfully named to support such a strong requirement...).

I mostly sent this PSA to shake out thoughts on this issue, to be honest, I'm not really comfortable with either option but am more annoyed by having to make things single-threaded per bogus checks than I think we really have thread-affine classes around that depend on base::NonThreadSafe (note: it's not the same thing for a class to be thread-affine and for your embedder to decide to run you in a single-threaded environment -- e.g. say //net/foo had a single truly thread-affine class but every other class in //net/foo needs to be mutually exclusive with it: then other classes are all sequence-friendly but happen to be forced to run on the same sequence as that class which is single-threaded (threads are also sequences)). Each class should encode its minimum self requirements, not the environment it expects to inherit from its embedder.

[Gabriel Charette]

Sounds like multiple people are voicing what I'd been thinking to this date: we need to introduce something like base::SequenceAffine as a transition class and shard to owners of base::NonThreadSafe instances for review.

I just thought today that maybe this is all overkill (hence this PSA)... especially since in the meantime it's incorrectly holding back many things from running on sequences.

Is anyone in the "just do it" camp?

[Ryan Sleevi]

On Thu, Apr 20, 2017 at 2:18 PM, Gabriel Charette <g...@chromium.org> wrote:

I said it's slowing the migration of code to run on TaskScheduler sequences (not that it's affecting product performance) -- i.e. it forces sequence-friendly components to request a SingleThreadTaskRunner instead of a SequencedTaskRunner from the TaskScheduler because a leaf deep down its dependency tree -- inherited from its members -- incorrectly uses a thread-affine check when it merely needs thread-safety.

Right, apologies, but we were saying the same thing. It sounds from your later message that you confirmed it - that this was an attempt to skip a step for expediency. I wanted to confirm, because I don't think we should skip that step, since it's easy to mask incorrectness by doing so.

 

I mostly sent this PSA to shake out thoughts on this issue, to be honest, I'm not really comfortable with either option but am more annoyed by having to make things single-threaded per bogus checks than I think we really have thread-affine classes around that depend on base::NonThreadSafe (note: it's not the same thing for a class to be thread-affine and for your embedder to decide to run you in a single-threaded environment -- e.g. say //net/foo had a single truly thread-affine class but every other class in //net/foo needs to be mutually exclusive with it: then other classes are all sequence-friendly but happen to be forced to run on the same sequence as that class which is single-threaded (threads are also sequences)). Each class should encode its minimum self requirements, not the environment it expects to inherit from its embedder.

While I disagree with your statement there (and perhaps due to a misunderstanding of your message), I do want to highlight that the proposed resolution - moving to base::ThreadChecker - doesn't really change that.

In general, I'm not a fan of base::NonThreadSafe, if only because it's a concrete interface that implements a particular behaviour, rather than being a type annotation or abstract interface. Thus everything implementing base::NonThreadSafe (versus having a base::ThreadChecker or base::SequenceChecker) which also implements another interface is violating our "multiple concrete inheritance is bad" rule. To date, we've all just sort of agreed to look the other way, because it'd be a lot of code churn and the benefits aren't that clear, beyond consistency.

I think given the precedent, and the risk, doing the option of introducing the migration class, and working through the cases, does sound like a more desirable outcome. You could further improve this outcome by moving to explicit members rather than multiple concrete inheritance - two birds, one stone - but that's a bonus-add, not a must-have.

[Gabriel Charette]

On Thu, Apr 20, 2017 at 2:30 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, Apr 20, 2017 at 2:18 PM, Gabriel Charette <g...@chromium.org> wrote:

I said it's slowing the migration of code to run on TaskScheduler sequences (not that it's affecting product performance) -- i.e. it forces sequence-friendly components to request a SingleThreadTaskRunner instead of a SequencedTaskRunner from the TaskScheduler because a leaf deep down its dependency tree -- inherited from its members -- incorrectly uses a thread-affine check when it merely needs thread-safety.

Right, apologies, but we were saying the same thing. It sounds from your later message that you confirmed it - that this was an attempt to skip a step for expediency. I wanted to confirm, because I don't think we should skip that step, since it's easy to mask incorrectness by doing so.

 

I mostly sent this PSA to shake out thoughts on this issue, to be honest, I'm not really comfortable with either option but am more annoyed by having to make things single-threaded per bogus checks than I think we really have thread-affine classes around that depend on base::NonThreadSafe (note: it's not the same thing for a class to be thread-affine and for your embedder to decide to run you in a single-threaded environment -- e.g. say //net/foo had a single truly thread-affine class but every other class in //net/foo needs to be mutually exclusive with it: then other classes are all sequence-friendly but happen to be forced to run on the same sequence as that class which is single-threaded (threads are also sequences)). Each class should encode its minimum self requirements, not the environment it expects to inherit from its embedder.

While I disagree with your statement there (and perhaps due to a misunderstanding of your message), I do want to highlight that the proposed resolution - moving to base::ThreadChecker - doesn't really change that.

I'm not proposing moving to base::ThreadChecker at all. I'm saying only the few classes that are truly thread-affine should.

You disagree that classes should only encode their minimum requirements? What I'm saying is if a class Foo doesn't have thread-affine state it should declare itself thread-affine because a thread-affine class Bar uses it. In other words, each usage of base::ThreadChecker should be accompanied with a comment explaining why (and saying "Foo is used by thread-affine class Bar" isn't a good reason -- Bar can have thread-affinity checks, while Foo only has sequence-affinity checks and they'll still be able to work together in single-threaded environments but Foo can now also be used by a non thread-affine embedder).

In general, I'm not a fan of base::NonThreadSafe, if only because it's a concrete interface that implements a particular behaviour, rather than being a type annotation or abstract interface. Thus everything implementing base::NonThreadSafe (versus having a base::ThreadChecker or base::SequenceChecker) which also implements another interface is violating our "multiple concrete inheritance is bad" rule. To date, we've all just sort of agreed to look the other way, because it'd be a lot of code churn and the benefits aren't that clear, beyond consistency.

I think given the precedent, and the risk, doing the option of introducing the migration class, and working through the cases, does sound like a more desirable outcome. You could further improve this outcome by moving to explicit members rather than multiple concrete inheritance - two birds, one stone - but that's a bonus-add, not a must-have.

 

Given a transition class rename is automatable while moving off of base::NonThreadSafe to a member base::Thread/SequenceChecker is not (or not as easily at least) I definitely will *not* be deprecating the unloved pattern of inheriting the checker a la base::NonThreadSafe. 

[Wez]

Although we've got the principle of preferring composition (i.e. base::SequenceChecker) to inheritance (i.e. base::SequenceAffine), and similarly for things like SupportsWeakPtr vs WeakPtrFactory, there is the issue of the impact on performance of production builds, since the compiler still has to allocate non-zero space for null-class members of an object, but most compilers have at least some support for optimizing out null base classes.

Which is to say: Yup, let's focus on migrating things that use base::NonThreadSafe to use a base::SequenceAffine, where possible, and deal with the base-class vs member question separately.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LKHHTf6za%2BPKaonufvC%2B7R-562mQEHusZp0zN_c2ksCAw%40mail.gmail.com.

[François Doray]

Do most people agree with these statements?:

1) A class shouldn't check for thread affinity unless it really needs it (e.g. because it uses thread-local storage or an external API which requires thread affinity). A class that merely needs mutual exclusion between accesses should check for *sequence* affinity.

2) base::NonThreadSafe causes violation of the  "multiple concrete inheritance is bad" rule.

3) base::NonThreadSafe and base::ThreadChecker are two different ways of doing the same thing.

4) The compiler has to allocate non-zero space for base::ThreadChecker and base::SequenceChecker members.

If people agree with this, what do you think of this migration plan?:

1) Introduce THREAD/SEQUENCE_CHECKER and ASSERT_CALLED_ON_VALID_THREAD/SEQUENCE macros that can be used like this:

class A {

 public:

  void MyMethod() {

    ASSERT_CALLED_ON_VALID_THREAD(thread_checker_);

  }

 private:

  THREAD_CHECKER(thread_checker_);

};

These macros are expanded to nothing when !DCHECK_IS_ON().

2) Move existing uses of ThreadChecker and SequenceChecker to these macros.

3) For every ~230 instance of base::NonThreadSafe (https://cs.chromium.org/search/?q=base::NonThreadSafe&p=2&type=cs), upload a CL that replaces the base::NonThreadSafe inheritance with a SEQUENCE_CHECKER member. In the CL message, ask the reviewer to validate that the use of SEQUENCE_CHECKER instead of THREAD_CHECKER is appropriate (with documentation of differences between the two options). Use an automated script to switch to THREAD_CHECKER when the reviewer asks for it.

4) Remove base::NonThreadSafe.

Yes, to expedite the migration, I skipped the step of adding a base::SequenceAffine base class. Since I believe we all agree that we don't want to keep it in the long term (cf 2) and 3) at the top of this message), what's the benefit of exposing many reviewers to it during the migration?

[Alexei Svitkine]

+1

It would be great to eliminate two different ways to do the same thing - and the macros would allow not paying the size cost of membership when there are no DCHECKs. Should also help existing uses of base::ThreadChecker by reducing the overhead for them. SGTM.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAGD3t5H2Zc3fxHqVVJf%3DJ%2BptQcqQDOiqSKNABQs6_2KA6yc-OA%40mail.gmail.com.

[dan...@chromium.org]

On Fri, Apr 21, 2017 at 12:34 PM, François Doray <fdo...@chromium.org> wrote:

Do most people agree with these statements?:

1) A class shouldn't check for thread affinity unless it really needs it (e.g. because it uses thread-local storage or an external API which requires thread affinity). A class that merely needs mutual exclusion between accesses should check for *sequence* affinity.

2) base::NonThreadSafe causes violation of the  "multiple concrete inheritance is bad" rule.

3) base::NonThreadSafe and base::ThreadChecker are two different ways of doing the same thing.

4) The compiler has to allocate non-zero space for base::ThreadChecker and base::SequenceChecker members.

If people agree with this, what do you think of this migration plan?:

1) Introduce THREAD/SEQUENCE_CHECKER and ASSERT_CALLED_ON_VALID_THREAD/SEQUENCE macros that can be used like this:

class A {

 public:

  void MyMethod() {

    ASSERT_CALLED_ON_VALID_THREAD(thread_checker_);

  }

 private:

  THREAD_CHECKER(thread_checker_);

};

These macros are expanded to nothing when !DCHECK_IS_ON().

2) Move existing uses of ThreadChecker and SequenceChecker to these macros.

3) For every ~230 instance of base::NonThreadSafe (https://cs.chromium.org/search/?q=base::NonThreadSafe&p=2&type=cs), upload a CL that replaces the base::NonThreadSafe inheritance with a SEQUENCE_CHECKER member. In the CL message, ask the reviewer to validate that the use of SEQUENCE_CHECKER instead of THREAD_CHECKER is appropriate (with documentation of differences between the two options). Use an automated script to switch to THREAD_CHECKER when the reviewer asks for it.

4) Remove base::NonThreadSafe.

Yes, to expedite the migration, I skipped the step of adding a base::SequenceAffine base class. Since I believe we all agree that we don't want to keep it in the long term (cf 2) and 3) at the top of this message), what's the benefit of exposing many reviewers to it during the migration?

I think this is a wonderful proposal. Thanks for working on this.

[Ryan Sleevi]

On Fri, Apr 21, 2017 at 12:55 PM, <dan...@chromium.org> wrote:

On Fri, Apr 21, 2017 at 12:34 PM, François Doray <fdo...@chromium.org> wrote:

Do most people agree with these statements?:

1) A class shouldn't check for thread affinity unless it really needs it (e.g. because it uses thread-local storage or an external API which requires thread affinity). A class that merely needs mutual exclusion between accesses should check for *sequence* affinity.

2) base::NonThreadSafe causes violation of the  "multiple concrete inheritance is bad" rule.

3) base::NonThreadSafe and base::ThreadChecker are two different ways of doing the same thing.

4) The compiler has to allocate non-zero space for base::ThreadChecker and base::SequenceChecker members.

If people agree with this, what do you think of this migration plan?:

1) Introduce THREAD/SEQUENCE_CHECKER and ASSERT_CALLED_ON_VALID_THREAD/SEQUENCE macros that can be used like this:

class A {

 public:

  void MyMethod() {

    ASSERT_CALLED_ON_VALID_THREAD(thread_checker_);

  }

 private:

  THREAD_CHECKER(thread_checker_);

};

These macros are expanded to nothing when !DCHECK_IS_ON().

2) Move existing uses of ThreadChecker and SequenceChecker to these macros.

3) For every ~230 instance of base::NonThreadSafe (https://cs.chromium.org/search/?q=base::NonThreadSafe&p=2&type=cs), upload a CL that replaces the base::NonThreadSafe inheritance with a SEQUENCE_CHECKER member. In the CL message, ask the reviewer to validate that the use of SEQUENCE_CHECKER instead of THREAD_CHECKER is appropriate (with documentation of differences between the two options). Use an automated script to switch to THREAD_CHECKER when the reviewer asks for it.

4) Remove base::NonThreadSafe.

Yes, to expedite the migration, I skipped the step of adding a base::SequenceAffine base class. Since I believe we all agree that we don't want to keep it in the long term (cf 2) and 3) at the top of this message), what's the benefit of exposing many reviewers to it during the migration?

I think this is a wonderful proposal. Thanks for working on this.

+1 - I think that's the best of all timelines :) 

[Wez]

Yes, this proposal sounds great to me too. :)

[Reilly Grant]

base::ThreadCheckerDoNothing has no member variables so does the compiler really allocate any space for base::ThreadChecker on a non-DCHECK build?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJfMmwZAbTOdGKXKnUf1MheYrdBJ8%2BGb080vA1-C77ZfnA%40mail.gmail.com.

[Torne (Richard Coles)]

Yes, because otherwise it would have the same address as the next member of the containing class, which is not permitted.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEmk%3DMbA8o5GzraY8RpWi3ZceFSJoQBGtsy_iE-809nrpj8Rsw%40mail.gmail.com.

[Wez]

FWIW it looks like MSVC will only allocate a single byte for each empty-class member - unfortunately unless the subsequent class member is also single-byte-aligned, you'll typically end up with it taking up four bytes.

Since Francois' suggested macro approach seems to have lots of support, I've taken the liberty of filing https://bugs.chromium.org/p/chromium/issues/detail?id=714835 for it.

[Bruce]

> you'll typically end up with it taking up four bytes.

Or eight bytes if the class contains any pointer or size_t types (given the prevalence of 64-bit builds of Chrome).

Stan has been working on crbug.com/710933 to fix some of the more problematic padding errors.

[Gabriel Charette]

Thanks all for the feedback, I had also considered Francois' proposal but had initially opted against it in an attempt to dodge a bigger refactor than necessary to solve my immediate problem.

After further thoughts and discussions though, Francois and I concluded that the mass rename to base::SequenceAffine is almost as hard as moving directly from base::NonThreadSafe to SEQUENCE_CHECKER macros (both require renaming methods in matching .cc files, the latter merely requires an extra script method to find the matching closing bracket to drop the member at end of class definition instead of renaming in place).

I'll take a stab at this.

Cheers,
Gab

[Wez]

Thanks for taking that on, Gabriel. :)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LKNAG9HyypzUWvxC%2Bc4o_aKoobApvxRV5DJh54HTyXUnA%40mail.gmail.com.

[Gabriel Charette]

And.... done!! base::NonThreadSafe is gone gone gone, replaced in vast majority with SequenceCheckers (maybe 5% ThreadCheckers). Will send a broader PSA later today about this and a sequencification update.

[Alexei Svitkine]

Awesome!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LKyF4Z9xqN_HEmkHKUOcwUjW%2BJ-%2B7gd9HGGYyi1vxgHSQ%40mail.gmail.com.

[Gabriel Charette]

PS: the fun happened @ https://crbug.com/676387. Thanks +Francois Pierre Doray for git cl split =P! 

[Wez]

Nice - thanks for taking the time to clean all this up. :)

[Yuri Wiitala]

Now thread-safe base::WeakPtr's make a lot more sense, eh? :)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJcrTVpmMOtvEpApSBiwmJfT9hM0kEeveY25wPqRF74S-w%40mail.gmail.com.

[Gabriel Charette]

On Wed, Jun 7, 2017, 03:33 Yuri Wiitala <m...@chromium.org> wrote:

Now thread-safe base::WeakPtr's make a lot more sense, eh? :)

WeakPtr were already made sequence-friendly already, but they're still not thread-safe and will never be (it can't be safe to check and use the pointer unless we make usage transactional but that'd imply locks and I don't think we want that).

[Alex Clarke]

Best effort cross-thread task cancellation likely is useful to have mind you, however it's implemented.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2B%3D-W_g45o371iqVfMSV%3Ddb8YLhU9iv3%2Bd__Sb7veFX3Q%40mail.gmail.com.

[Gabriel Charette]

On Wed, Jun 7, 2017, 10:14 Alex Clarke <alexc...@google.com> wrote:

Best effort cross-thread task cancellation likely is useful to have mind you, however it's implemented.

That's essentially owner_sequence->PostTask(&WeakPtrFactory::InvalidateWeakPtrs, ...) though (and in practice that's more likely to be a owner_sequence->DeleteSoon(&object_that_owns_factory)), so unless that's very common and we need a helper we essentially already "have it".

The one thing I've been meaning to add for a while though (and a prerequisite for pruning cancelled tasks in task_scheduler) is thread-safe validity check (i.e. WeakPtr::operator book()). I might just do that today.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Bernhard Bauer]

Is that a valid operation? What if the owner deletes the object immediately after the check?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7LKeZfPSRAUGQS5Os%3DV34q%2BocUkFx47wgE0R1Tj-xv7MHQ%40mail.gmail.com.

[Gabriel Charette]

On Wed, Jun 7, 2017, 11:11 Bernhard Bauer <bau...@google.com> wrote:

Is that a valid operation? What if the owner deletes the object immediately after the check?

It's a best effort operation, you still can't use the object (usage will still perform a sequence check). It's meant to be used be by callers that merely care to know if the task *may* still be valid but don't intend to use it (i.e. the scheduler when pruning cancelled delayed tasks).

[Bernhard Bauer]

Okay, that makes sense. I would't really use the bool() operator for it, because may-exist seems like a weaker guarantee than truthiness, but with that said, I'm going to disappear into the peanut gallery again :)

Bernhard.

[Gabriel Charette]

FYI, I made an attempt at thread-safe WeakPtr validity check this morning (https://chromium-review.googlesource.com/c/527413/).

But it can't work for now because we support WeakPtr::reset() -- wasn't aware of that, thought InvalidateWeakPtrs() was only reset. I think this is only for the SupportsWeakPtr paradigm which is deprecated but nevertheless must keep working until stripped from codebase... yay another mass migration from inheritance to member variable... not sure if/when/whether I'll get around to that... Will send another announcement then.

[Daniel Cheng]

Fwiw, I would be really happy to see SupportsWeakPtr go away. However, I poked at this awhile back and it's tricky to automate because SupportsWeakPtr allows automatic downcasting from WeakPtr<Base> to WeakPtr<T> where T is a subclass that inherits SupportWeakPtr<T> from an ancestor class Base.

Daniel

[Wez]

Re SupportsWeakPtr: One of the legitimate reasons for the base is that it can dispense WeakPtrs based on |this| without needing to waste bytes storing |this| in the object itself (unlike WeakPtrFactory as a member).  Unless we can find a clean way to provide that same optimization, I'd be reluctant to move lots of classes from SupportsWeakPtr base to WeakPtrFactory members.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJTZ7L%2B%3D-W_g45o371iqVfMSV%3Ddb8YLhU9iv3%2Bd__Sb7veFX3Q%40mail.gmail.com.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Wez]

It's just the way WeakPtrFactory & SupportsWeakPtr work:

- To allow WeakPtr<sub-class> to be dispensed by SupportsWeakPtr<base-class>, WeakPtrs contain a raw pointer to the target (rather than that being stored in the internal Flag).

- Because of that, WeakPtrFactory holds both a reference to the Flag, and a raw pointer to the target object.

- SupportsWeakPtr knows that the target object is |this|, so only needs to store the Flag reference. It just down-casts |this| to the derived object's type, and populates dispensed WeakPtrs with that.

=> SupportsWeakPtr saves sizeof(ptr_t) bytes in each object. :)

On 12 June 2017 at 11:10, Albert Wong (王重傑) <ajw...@google.com> wrote:

Any chance you got a link to an example off hand here?

[Jeremy Roman]

On Mon, Jun 12, 2017 at 2:58 PM, Wez <w...@chromium.org> wrote:

It's just the way WeakPtrFactory & SupportsWeakPtr work:

- To allow WeakPtr<sub-class> to be dispensed by SupportsWeakPtr<base-class>, WeakPtrs contain a raw pointer to the target (rather than that being stored in the internal Flag).

- Because of that, WeakPtrFactory holds both a reference to the Flag, and a raw pointer to the target object.

- SupportsWeakPtr knows that the target object is |this|, so only needs to store the Flag reference. It just down-casts |this| to the derived object's type, and populates dispensed WeakPtrs with that.

=> SupportsWeakPtr saves sizeof(ptr_t) bytes in each object. :)

This could be changed by having WeakPtrFactory take the pointer as an argument to GetWeakPtr rather than to the constructor, at a cost of making usage slightly more verbose and subtle. (Aside: this would also make it easier to pass out weak pointers to member variables, on the rare occasion that that is desirable.)

base::internal::WeakReferenceOwner is flexible here, but at present we hide some of its flexibility to keep the interfaces to WeakPtrFactory and SupportsWeakPtr simple.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJeTVH2cgxtE3Npz7%3D85xR-%3DXcQX0jLzxPHnzbjfq6zBnA%40mail.gmail.com.

[Wez]

Thanks Jeremy - I agree that there are ways around this, but one goal for improvements to base::WeakPtr and friends should be to simplify the APIs & semantics, I think.

Re passing out pointers to member variables, one potential goal for WeakPtr is actually to move the target pointer into the Flag, so that the WeakPtr itself takes up less space (since there are more WeakPtrs than flags) - dcheng@ looked at doing that, and I believe we have numbers to suggest it's worthwhile, so we should also bear that in mind.