ThreadChecker vs. NonThreadSafe

[Alexander Semashko]

Hi!

Currently it is recommended to use ThreadChecker wherever possible, see comment in thread_checker.h [1]

The reasoning is that style guide says to prefer composition over inheritance, which was mentioned in the commit message [2] when that comment was added.
However, there is one important thing: when DCHECKs are off, both ThreadChecker and NonThreadSafe turn into empty classes. This means that in production builds classes that use ThreadChecker waste up to 4 or 8 bytes per instance, depending on bitness of the build, and classes that use NonThreadSafe do not waste memory because of empty base optimization [3].

I think that the style guide rule was not meant to be enforced in cases like this. Looks like we have actually have a good reason to recommend using NonThreadSafe where possible (e.g. when it does not lead to multiple inheritance).

What do people think? Are there any downsides?

+cc Nico who made this question rise, and Ryan who added those comments.


[1] https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?l=41
[2] https://chromium.googlesource.com/chromium/src/+/81c309c235d2f7ca5854597a733cbd732ca1459e
[3] http://en.cppreference.com/w/cpp/language/ebo


—
Best regards,
Alexander Semashko
ah...@yandex-team.ru

[Peter Kasting]

On Fri, Dec 2, 2016 at 6:37 AM, Alexander Semashko <ah...@yandex-team.ru> wrote:

I think that the style guide rule was not meant to be enforced in cases like this. Looks like we have actually have a good reason to recommend using NonThreadSafe where possible (e.g. when it does not lead to multiple inheritance).

...if those extra bytes matter.  Do they?

Normally we don't have lots of instances of objects which incorporate ThreadChecker.

PK

[Wez]

We use them in frequently used features of base/, such as WeakPtr - so this impacts every ObserverList, CancelableCallback, etc.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[dan...@chromium.org]

On Fri, Dec 2, 2016 at 1:21 PM, Wez <w...@chromium.org> wrote:

We use them in frequently used features of base/, such as WeakPtr - so this impacts every ObserverList, CancelableCallback, etc.

I don't think that implies that these bytes matter tbh. These things live on a control classes which are relatively few.

[Alexander Semashko]

Here is some data. The numbers show how many instances of ThreadCheckerImpl existed in a process at a given point in time. 

after startup

browser: 1877

renderer: 500

gpu: 113

single tab, news site

browser: 3005

gpu: 207

renderer: 1933

15 tabs with "typical" sites

browser process: 18333

gpu process: 472

flash plugin: 83

renderers: 574 + 756 + 878 + 624 + 776 + 7457 + 966 + 961 + 382 + 2523 + 460 + 1411 + 381 + 1401 + 3354, total 22904

3 дек. 2016 г., в 0:39, Alexander Semashko <ah...@yandex-team.ru> написал(а):

I'll add a counter and see what it shows at runtime.

Any suggestions on which workloads to check?

3 дек. 2016 г, в 0:34, dan...@chromium.org написал(а):

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

—

[Yuta Kitamura]

I think this is one of the rare cases where inheritance works better than composition.

However, on Windows, EBO does not come into effect (i.e. an empty base takes one byte space) if the empty base is not the first element in the base class list. Doing EBO for such cases breaks the ABI, so clang-win does not do that either.

So, the difference between ThreadChecker and NonThreadSafe is relatively small on Windows: the extra space is optimized away if the NonThreadSafe is listed first in the base class list. I'm not sure this is a definite win for us on Windows. However, given that EBO is always effective on Android (am I right?), I guess we probably should always use NonThreadSafe.

[Alexander Semashko]

I'll add a counter and see what it shows at runtime.

Any suggestions on which workloads to check?

3 дек. 2016 г., в 0:34, dan...@chromium.org написал(а):

—

[Gabriel Charette]

Side note: I'd like us to move towards non-thread-safe => SequenceChecker (most checks are incorrectly thread-affine in classes that merely require thread-safety). I'll soon kickoff a separate thread about this but please keep that in mind (and me in the loop) if you decide to change recommendations around thread checking practices.

[David Benjamin]

On Mon, Dec 5, 2016 at 11:41 AM Gabriel Charette <g...@chromium.org> wrote:

Side note: I'd like us to move towards non-thread-safe => SequenceChecker (most checks are incorrectly thread-affine in classes that merely require thread-safety). I'll soon kickoff a separate thread about this but please keep that in mind (and me in the loop) if you decide to change recommendations around thread checking practices.

Le sam. 3 déc. 2016 09:34, Alexander Semashko <ah...@yandex-team.ru> a écrit :

Here is some data. The numbers show how many instances of ThreadCheckerImpl existed in a process at a given point in time. 

after startup

browser: 1877

renderer: 500

gpu: 113

single tab, news site

browser: 3005

gpu: 207

renderer: 1933

15 tabs with "typical" sites

browser process: 18333

gpu process: 472

flash plugin: 83

renderers: 574 + 756 + 878 + 624 + 776 + 7457 + 966 + 961 + 382 + 2523 + 460 + 1411 + 381 + 1401 + 3354, total 22904

How do the numbers look if you also count SequenceChecker? In particular, I see WeakReference::Flag has a SequenceChecker.

David

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...@chromium.org.

[Gabriel Charette]

If we find that the memory cost matters, it might suffice to put the most used checkers behind #if DCHECK_IS_ON() (I suspect a few checkers make the bulk of the usage)

[Alexander Semashko]

5 дек. 2016 г., в 20:13, Gabriel Charette <g...@chromium.org> написал(а):

If we find that the memory cost matters, it might suffice to put the most used checkers behind #if DCHECK_IS_ON() (I suspect a few checkers make the bulk of the usage)

This way the usual code like DCHECK(thread_checker_.CalledOnValidThread()) won't compile.

See comment in logging.h:

// DCHECK et al. make sure to reference |condition| regardless of

// whether DCHECKs are enabled; this is so that we don't get unused

// variable warnings if the only use of a variable is in a DCHECK.

Le lun. 5 déc. 2016 12:06, David Benjamin <davi...@chromium.org> a écrit :

On Mon, Dec 5, 2016 at 11:41 AM Gabriel Charette <g...@chromium.org> wrote:

Side note: I'd like us to move towards non-thread-safe => SequenceChecker (most checks are incorrectly thread-affine in classes that merely require thread-safety). I'll soon kickoff a separate thread about this but please keep that in mind (and me in the loop) if you decide to change recommendations around thread checking practices.

Le sam. 3 déc. 2016 09:34, Alexander Semashko <ah...@yandex-team.ru> a écrit :

Here is some data. The numbers show how many instances of ThreadCheckerImpl existed in a process at a given point in time. 

after startup

browser: 1877

renderer: 500

gpu: 113

single tab, news site

browser: 3005

gpu: 207

renderer: 1933

15 tabs with "typical" sites

browser process: 18333

gpu process: 472

flash plugin: 83

renderers: 574 + 756 + 878 + 624 + 776 + 7457 + 966 + 961 + 382 + 2523 + 460 + 1411 + 381 + 1401 + 3354, total 22904

How do the numbers look if you also count SequenceChecker? In particular, I see WeakReference::Flag has a SequenceChecker.

David

Ok, I will add a counter and post an update.

Re: Yuta Kitamura

I think this is one of the rare cases where inheritance works better than composition.

However, on Windows, EBO does not come into effect (i.e. an empty base takes one byte space) if the empty base is not the first element in the base class list. Doing EBO for such cases breaks the ABI, so clang-win does not do that either.

So, the difference between ThreadChecker and NonThreadSafe is relatively small on Windows: the extra space is optimized away if the NonThreadSafe is listed first in the base class list. I'm not sure this is a definite win for us on Windows. However, given that EBO is always effective on Android (am I right?), I guess we probably should always use NonThreadSafe.

The style guide allows multiple inheritance only when all superclasses, with the possible exception of the first one, are pure interfaces.

So, unless this thread leads to the decision to deviate from this rule too, the benefits from EBO will be equal on all platforms, as they are limited to the cases where NonThreadSafe is the first base (including ones where there are no others).

And it is unclear (yet) what portion of objects could be optimized in terms of instance count. There is a lot of small classes with no parents, and there is also a lot of classes with non-pure bases (e.g., many delegate and observer interfaces have default implementations of their methods).

[Alexei Svitkine]

If we do find it's worth fixing, one option would have macros for thread checker membership and checking.

e.g.

  DCHECK_THREAD_CHECKER_MEMBER(thread_checker_);

would expand to an #if DCHECK_IS_ON() block.

And we can have a similar macro to replace DCHECK(thread_checker_.CalledOnValidThread()).

This way, we still get all the benefits in DCHECK builds but no runtime overhead, at the cost of more obscurity in terms of code constructs.

Chromium Developers mailing list: chromium-dev@chromium.org

View archives, change email options, or unsubscribe: 
http://groups.google.com/a/chromium.org/group/chromium-dev

-- 
-- 
Chromium Developers mailing list: chromium-dev@chromium.org

View archives, change email options, or unsubscribe: 
http://groups.google.com/a/chromium.org/group/chromium-dev

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
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.

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

[Wez]

Alexander: Note that SequenceChecker falls-back to ThreadChecker internally, so your ThreadChecker numbers should be correct, I think.

If we prefer to recommend inheritance for these cases then we should add a Clang plugin to check that the potentially-empty class(es) are the first in the inheritance list (or is it possible to have MSVC configured to emit a warning for the 1-byte case)?

In terms of potential impact, we have a few of these cases of classes with an empty impl in release, and I'd expect their effects to be more of a problem in terms of fragmentation than anything else (e.g. where the extra byte(s) lead to extra gaps, alignment padding, etc).

Chromium Developers mailing list: chromi...@chromium.org

[Scott Hess]

Is this behavior portable enough to rely on?

class EmptyClass {
};

struct EmptyStruct {
};

struct EmptyArrayStruct {
char a_[0];
};

EmptyClass ec;
LOG(ERROR) << "sizeof(EmptyClass) " << sizeof(ec);
EmptyStruct es;
LOG(ERROR) << "sizeof(EmptyStruct) " << sizeof(es);
EmptyArrayStruct eas;
LOG(ERROR) << "sizeof(EmptyArrayStruct) " << sizeof(eas);

ERROR:connection_unittest.cc(1975)] sizeof(EmptyClass) 1
ERROR:connection_unittest.cc(1977)] sizeof(EmptyStruct) 1
ERROR:connection_unittest.cc(1979)] sizeof(EmptyArrayStruct) 0

-scott

-scott

>>>>>>> Chromium Developers mailing list: chromi...@chromium.org
>>>>>>> View archives, change email options, or unsubscribe:
>>>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>>
>>>>>>
>>>>>> --
>>>>>> --
>>>>>> Chromium Developers mailing list: chromi...@chromium.org
>>>>>> View archives, change email options, or unsubscribe:
>>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>

>>>>> —

>>>>> Best regards,
>>>>> Alexander Semashko
>>>>> ah...@yandex-team.ru
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> —
>>>>> Best regards,
>>>>> Alexander Semashko
>>>>> ah...@yandex-team.ru
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> --
>>>>> Chromium Developers mailing list: chromi...@chromium.org
>>>>> View archives, change email options, or unsubscribe:
>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>
>>>>
>>>> --
>>>> --
>>>> 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...@chromium.org.
>>
>>
>> —

[Elliott Sprehn]

On Mon, Dec 5, 2016 at 10:25 AM, Alexander Semashko <ah...@yandex-team.ru> wrote:

5 дек. 2016 г., в 20:13, Gabriel Charette <g...@chromium.org> написал(а):

If we find that the memory cost matters, it might suffice to put the most used checkers behind #if DCHECK_IS_ON() (I suspect a few checkers make the bulk of the usage)

This way the usual code like DCHECK(thread_checker_.CalledOnValidThread()) won't compile.

See comment in logging.h:

// DCHECK et al. make sure to reference |condition| regardless of

// whether DCHECKs are enabled; this is so that we don't get unused

// variable warnings if the only use of a variable is in a DCHECK.

This is the reason the blink ASSERT() macros actually compile away the condition instead. DCHECK unfortunately means you end up with debugging code in your release binary unless the compiler is smart enough to strip it.

I do think we should reconsider this behavior for DCHECK.

 

Chromium Developers mailing list: chromium-dev@chromium.org

View archives, change email options, or unsubscribe: 
http://groups.google.com/a/chromium.org/group/chromium-dev

-- 
-- 
Chromium Developers mailing list: chromium-dev@chromium.org

View archives, change email options, or unsubscribe: 
http://groups.google.com/a/chromium.org/group/chromium-dev

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
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.

—

Best regards,

Alexander Semashko

ah...@yandex-team.ru

[Wez]

FWIW I hit some issues with DCHECK_OP() with declarations of operator<<() becoming unused when I made a similar change on that macro.  IIRC there was a similar but lesser issue with a few DCHECK call-sites, in the cases of trailing operator<<() logging.

Chromium Developers mailing list: chromi...@chromium.org

[Primiano Tucci]

On Mon, Dec 5, 2016, 20:10 Scott Hess <sh...@chromium.org> wrote:

Is this behavior portable enough to rely on?

class EmptyClass {
};

struct EmptyStruct {
};

struct EmptyArrayStruct {
  char a_[0];
};

unfortunately it seems to be a gcc/clang extension only. I had the same thought and tried on https://codereview.chromium.org/2542403003/ but msvc doesn't really like it (CrOs failures are unrelated there) 

  EmptyClass etc

[Alexander Semashko]

I hope there will be a recap on this thread from someone who takes part in defining chromium's style guide and policies.

Does it make sense to recommend deriving from NonThreadSafe when there are no other bases?

The gain seem to be not critical (however, in some cases it may be larger because of cache locality etc.).

However, the style guide does not require to use composition instead of inheritance, it only says that "Composition is often more appropriate".

And it seems to be not really appropriate in this case.

6 дек. 2016 г., в 0:43, Primiano Tucci <prim...@chromium.org> написал(а):

—

С уважением,

Александр Семашко

ah...@yandex-team.ru

[Wez]

Yes, do we think the current recommendation to use ThreadChecker rather than NonThreadSafe is still what we want?

On the one hand it's nice not to special-case this, and to consistently recommend composition. It's also technically safer to compose, since you can then thread-check before the rest of your class' destructor runs, rather than after. Even with inheritance, EBO won't always kick-in (see the comments above re EBO for Windows).

On the other hand, composition costs us some [memory] performance in Release builds, and unlike some of our other mix-in base-classes (e.g. SupportsWeakPtr), NonThreadSafe makes sense as part of the interface contract of a class.

My 2c: I'd suggest we stick with the current recommendation, in the absence of stronger evidence of a real performance penalty.

>>>>>>> Chromium Developers mailing list: chromium-dev@chromium.org

>>>>>>> View archives, change email options, or unsubscribe:
>>>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>>
>>>>>>
>>>>>> --
>>>>>> --

>>>>>> Chromium Developers mailing list: chromium-dev@chromium.org

>>>>>> View archives, change email options, or unsubscribe:
>>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>
>>>>> —
>>>>> Best regards,
>>>>> Alexander Semashko
>>>>> ah...@yandex-team.ru
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> —
>>>>> Best regards,
>>>>> Alexander Semashko
>>>>> ah...@yandex-team.ru
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> --

>>>>> Chromium Developers mailing list: chromium-dev@chromium.org

>>>>> View archives, change email options, or unsubscribe:
>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>
>>>>
>>>> --
>>>> --

>>>> Chromium Developers mailing list: chromium-dev@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.

>>
>>
>> —
>> Best regards,
>> Alexander Semashko
>> ah...@yandex-team.ru
>>
>>
>>
>> --
>> --

>> Chromium Developers mailing list: chromium-dev@chromium.org

>> View archives, change email options, or unsubscribe:
>> http://groups.google.com/a/chromium.org/group/chromium-dev
>
>
> --
> --

> Chromium Developers mailing list: chromium-dev@chromium.org

> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

--
--

Chromium Developers mailing list: chromium-dev@chromium.org

View archives, change email options, or unsubscribe:
    http://groups.google.com/a/chromium.org/group/chromium-dev