661596 - Prevent NSPR from falling back to PR_Atomic functions on Mac when compiler intrinsics are available

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Justin Lebar (not reading bugmail)]

I was profiling on Mac and I noticed many calls to PR_Atomic{Increment,Decrement}.  We'd attempted to get rid of these in bug 592557.

It appears that in the js engine (and perhaps elsewhere) DARWIN is not defined.  This causes pratom.h to decide that __sync_add_and_fetch isn't defined, and we then fall back to the slow path.

Filing under the js component until I can verify that this happens when building other parts of the code too.

Comment 1

[Justin Lebar (not reading bugmail)]

This happens outside js, too.  Moving to core/general.

I think it's Mac and Linux, but I haven't tested Linux yet.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: WIP v1

Works on Mac.  Need to test on Linux.

Comment 3

[Justin Lebar (not reading bugmail)]

To be fair, we're not falling back to the method which uses locks; there's pr/include/md/_darwin.h, which points to an assembly version of atomic increment for darwin.

But it's silly -- we should be using the compiler intrinsic, saving a function call, and letting the compiler optimize around the atomic increment.

Comment 4

[Justin Lebar (not reading bugmail)]

Linux and Windows seem to work fine as-is.  I'll see if I can test Android.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to comment #4)
> Linux and Windows seem to work fine as-is.  I'll see if I can test Android.

Android's gcc 4.4 and 4.4.3 don't have __sync_* built-in functions.  It is required gcc 4.5.2 (for ARM).

If I should write inline assembler for atomic for arm gcc 4.4.x platform, please file a new bug for NSPR.

Comment 6

[Justin Lebar (not reading bugmail)]

Looks like the atomics for ARM were done in bug 405992.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 537148 [details] [diff] [review]
Patch v1

Josh, do you think those #undef DARWINs were there for a reason?  We include Cocoa.h plenty of times without undef'ing DARWIN.

The only harm of leaving the undefs in is that we'll call the PR_Atomic functions from these files instead of using the macros.  If you want to leave them, that's OK with me.

Comment 9

[Wan-Teh Chang]

Comment on attachment 537148 [details] [diff] [review]
Patch v1

We should fix the NSPR pratom.h header.  NSPR public headers should be
self-sufficient and should not require the client to define any macro.

We can change pratom.h to test defined(__APPLE__) instead of defined(DARWIN).

Comment 10

[Justin Lebar (not reading bugmail)]

We'd similarly want to change LINUX to __LINUX__ in that case.

Comment 11

[Justin Lebar (not reading bugmail)]

...actually, you'd probably want to change all of the OS defines, then, right?

Maybe we can add a header which defines DARWIN, LINUX, etc.  It appears there are many such defines.

Comment 12

[Justin Lebar (not reading bugmail)]

Ah, I see; not all of these headers are publicly-consumable.  The include/md folder is private?  If so, looks like we just need to modify pratom.h.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to comment #5)
> (In reply to comment #4)
> > Linux and Windows seem to work fine as-is.  I'll see if I can test Android.
> 
> Android's gcc 4.4 and 4.4.3 don't have __sync_* built-in functions.  It is
> required gcc 4.5.2 (for ARM).

So just to check, gcc 4.4.x doesn't define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4?  Or does it define it and export a busted __sync_add_and_fetch?

Comment 14

[Makoto Kato [:m_kato]]

(In reply to comment #13)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Linux and Windows seem to work fine as-is.  I'll see if I can test Android.
> > 
> > Android's gcc 4.4 and 4.4.3 don't have __sync_* built-in functions.  It is
> > required gcc 4.5.2 (for ARM).
> 
> So just to check, gcc 4.4.x doesn't define
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4?  Or does it define it and export a
> busted __sync_add_and_fetch?

gcc 4.4/4.4.3 for *arm* doesn't defined it and doesn't support __sync_* atomic. You can see more details by https://wiki.linaro.org/WorkingGroups/ToolChain/AtomicMemoryOperations.

If __sync_* functions are supported by gcc, __GCC_HAVE_SYNC_COMPARE_AND_SWAP_* is defined by cpp.  I think that you should use __GCC_HAVE_SYNC_COMPARE_AND_SWAP_* instead of LINUX.

Comment 15

[Justin Lebar (not reading bugmail)]

> I think that you should use __GCC_HAVE_SYNC_COMPARE_AND_SWAP_* instead of LINUX.

The problem, AIUI, is that on some architectures GCC defines __GCC_HAVE_SYNC_COMPARE_AND_SWAP_* but exports __sync_* intrinsics which aren't actually threadsafe.  So we can only use the intrinsics on architectures where we know they're safe.

Comment 16

[Josh Aas]

Comment on attachment 537148 [details] [diff] [review]
Patch v1

I don't know why those DARWIN undefs are there, I'm fine with getting rid of them.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: NSPR Patch v1

Comment 18

[Wan-Teh Chang]

Comment on attachment 537552 [details] [diff] [review]
NSPR Patch v1

r=wtc.  Note that it is fine to test the LINUX macro because
it is defined by NSPR (in mozilla/nsprpub/pr/include/md/_linux.cfg).

Comment 19

[Justin Lebar (not reading bugmail)]

I'm not clear what the process is for making changes to NSPR.  Do I check this patch in to m-c, or somewhere else?

Comment 20

[(not currently active) Ted Mielczarek]

NSPR changes need to land in NSPR CVS. wtc or I can land them for you. Getting them into m-c requires one of us to also tag the NSPR repository, and then someone to update the NSPR snapshot in m-c.

Comment 21

[Justin Lebar (not reading bugmail)]

(In reply to comment #20)
> NSPR changes need to land in NSPR CVS. wtc or I can land them for you.
> Getting them into m-c requires one of us to also tag the NSPR repository,
> and then someone to update the NSPR snapshot in m-c.

I'll sit tight then.  :)

Comment 22

[(not currently active) Ted Mielczarek]

Sorry, I always forget about these bugs:

Checking in pr/include/pratom.h;
/cvsroot/mozilla/nsprpub/pr/include/pratom.h,v  <--  pratom.h
new revision: 3.20; previous revision: 3.19
done