1278502 - TSan: data races netwerk/cache2/CacheFileIOManager.{h,cpp} on CacheFileHandle::{mSpecialFile, mInvalid}

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Julian Seward [:jseward]]

TSan reports data races on CacheFileHandle::{mSpecialFile, mInvalid} for more
or less any surfing performed with a TSan-enabled Firefox build.  It is easy
to reproduce.

As far as I can tell, CacheFileHandle::mSpecialFile and CacheFileHandle::mInvalid
are properly synchronised from a logical point of view.  mSpecialFile is accessed
only on the main thread and mInvalid only on the IO thread.  However, because they
are declared as single-bit booleans and they fall in the same byte, the hardware
cannot update them atomically (independently) and so TSan correctly reports a 
race on the containing byte.

Comment 1

[Julian Seward [:jseward]]

Attachment: TSan complaint

Comment 2

[Julian Seward [:jseward]]

Attachment: Proposed fix

This puts mSpecialFile into its own byte, so that it isn't in the same
byte as mInvalid et al.  I also put mPriority into its own byte for
consistency.  This isn't (as far as I can see) actually necessary, but
there's no loss either, because once mSpecialFile is in its own byte then
the compiler will allocate a whole byte for "mPriority : 1" anyway.

Comment 3

[Honza Bambas (:mayhemer)]

Hmm.. so the compiler doesn't recognize that the first two are bool const, while the rest is a r/w bool?  That's odd.  Could we just move the two flags your are converting somewhere around the class so that those are separated?  Or make them uint16_t or so?  Not sure what's better for performance.

Comment 4

[Julian Seward [:jseward]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Hmm.. so the compiler doesn't recognize that the first two are bool const,
> while the rest is a r/w bool?  That's odd.

It can recognise them as bool const, sure, but why should that imply that it
should put them in separate bytes?  I don't see that.

> Could we just move the two flags
> your are converting somewhere around the class so that those are separated? 
> Or make them uint16_t or so?  Not sure what's better for performance.

I seriously doubt you can detect any performance change unless these flags are
being set and queried millions of times per second.  That said:

Changing them to be non-bitfields actually improves performance, because it
means assigments are a single store instruction instead of the 
read-shift-AND-OR-write sequence which is what the bitfield version requires,
and similarly for uses of the value.

8 bit loads/stores are just as cheap as 16 bit load/stores, so I don't think
uint16_t is going to help at all.

I can move the flags around if you really want, in the hope of finding some
combination that works, but that (presumably) makes the .h file harder to understand,
and I don't even know if it will succeed.  Do you want me to try anyway?

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 8760681 [details] [diff] [review]
Proposed fix

Review of attachment 8760681 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Could we just move the two flags your are converting somewhere around 
> the class so that those are separated? 

I like the patch how it is, feel free to land it. If Honza really needs these flags packed into a single byte the following should work, right?

bool const           mPriority : 1;	
bool const           mSpecialFile : 1;	
bool                 : 0;
  // These bit flags are all accessed only on the IO thread
bool mInvalid : 1;

Comment 6

[Julian Seward [:jseward]]

Honza, are you ok with the patch as-is or do you want me to work
on it further, per comment 4 ?

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Julian Seward [:jseward] from comment #4)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> > Hmm.. so the compiler doesn't recognize that the first two are bool const,
> > while the rest is a r/w bool?  That's odd.
> 
> It can recognise them as bool const, sure, but why should that imply that it
> should put them in separate bytes?  I don't see that.

It's a different type actually, but whatever.

> 
> > Could we just move the two flags
> > your are converting somewhere around the class so that those are separated? 
> > Or make them uint16_t or so?  Not sure what's better for performance.
> 
> I seriously doubt you can detect any performance change unless these flags
> are
> being set and queried millions of times per second.  That said:
> 
> Changing them to be non-bitfields actually improves performance, because it
> means assigments are a single store instruction instead of the 
> read-shift-AND-OR-write sequence which is what the bitfield version requires,
> and similarly for uses of the value.

These two are const, so only read (AND/TEST).

> 
> 8 bit loads/stores are just as cheap as 16 bit load/stores, so I don't think
> uint16_t is going to help at all.
> 
> I can move the flags around if you really want, in the hope of finding some
> combination that works, but that (presumably) makes the .h file harder to
> understand,
> and I don't even know if it will succeed.  Do you want me to try anyway?

No, let's go with it as it is, despite that I think it's just waste of memory for nothing...



(In reply to Julian Seward [:jseward] from comment #6)
> Honza, are you ok with the patch as-is or do you want me to work
> on it further, per comment 4 ?

Rather per comment 5 if that works ;)  But don't waste that much time on this detail.  Up to you.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f812f8563fb
TSan: data races netwerk/cache2/CacheFileIOManager.{h,cpp} on CacheFileHandle::{mSpecialFile, mInvalid}.  r=michal.novotny.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f812f8563fb