415563 - Use new NSPR atomic macros - NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P2)

Description

[Steve Snyder]

Use the recently-added NSPR atomic Increment/Decrement/Set/Add macros rather than function calls in existing code.  The goal is to improve general performance by moving those atomic operations to inline code.

The inline code is much faster than the function calls at the cost of increasing the code size by about 8 bytes per use.

For platforms that do not support the use of inline atomic operations, the original function calls are used, making this patch a no-op.

Comment 1

[Steve Snyder]

Attachment: Replace PR_Atomic[operation] function calls with equivelent macros

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Please submit patches that are CVS diffs against the properly named files
in the source tree, so that bugzilla's patch reviewing tools can work 
properly on those patches.  Please use "cvs diff -Npu5" to generate the
patches.  Thanks.

Comment 3

[Steve Snyder]

Attachment: Reformatted, with CVS

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Steve's patch brought forward to trunk (checked in and then backed out)

Sadly, Steve's patch languished for 2 years for lack of a review request. :(
So I've brought it forward to the current trunk, and fixed a couple of line
length issues.  

Wan-Teh, please review, especially the change to pratom.h

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: sypplemental patch: make NSPR tests use new macro, too

This patch makes NSPR's test programs use the new macros, too.
I produced it with my gsr script.

Wan-Teh, The review question here is really if this change is 
desirable.  It seems that with the test programs, we are faced
with a choice, which is to never use the inline macros, or to 
always use them when possible.  I think the latter is preferable.
What do you think?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 433764 [details] [diff] [review]
Steve's patch brought forward to trunk (checked in and then backed out)

Since this patch is still fundamentally Steve's patch, I think I can review it, even though I've altered it some.  
So, r+.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bug 415563: Use new NSPR atomic macros in NSPR
Patch contributed by Steve Snyder <[e-mail]>, r=nelson

include/pratom.h;              new revision: 3.13; previous revision: 3.12
src/misc/prinit.c;             new revision: 3.54; previous revision: 3.53
src/pthreads/ptsynch.c;        new revision: 3.34; previous revision: 3.33
src/pthreads/ptthread.c;       new revision: 3.86; previous revision: 3.85
src/threads/prtpd.c;           new revision: 3.13; previous revision: 3.12
src/threads/combined/pruthr.c; new revision: 3.38; previous revision: 3.37

The first of these two patches is committed.  
The second awaits Wan-Teh's review.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Review request ping.

Comment 9

[Wan-Teh Chang]

Comment on attachment 433765 [details] [diff] [review]
sypplemental patch: make NSPR tests use new macro, too

Nelson, thanks for the patch.  Some of these test programs should
continue to use the atomic functions, in particular atomic.c, which
already tests both the functions and macros:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/tests/atomic.c&rev=3.12&mark=133,137,143,149,156#132

As for the other test programs, we can modify half of them to use
the macros and leave the other half using the functions.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

OK, Wan-Teh, I'll let you take whatever subset of my patch suits your 
judgment and you can commit that.

Comment 11

[Wan-Teh Chang]

Comment on attachment 433765 [details] [diff] [review]
sypplemental patch: make NSPR tests use new macro, too

Nelson, you can close this bug now.  It's not necessary to change
any of the test programs to use the macros.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

I pushed the NSPR_HEAD_20100524 tag to mozilla-central and the 32bit linux builders failed two runs (logs below). The same happened when I pushed to try this morning. Pushing that tag to try with the patch from this bug backed out built and tested successfully.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274733715.1274733805.20465.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274737619.1274737724.5911.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274733715.1274733817.20490.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274737620.1274737711.5883.gz

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

taras pointed me to this, which seems to indicate that this is caused by a gcc bug:
http://stackoverflow.com/questions/130740/link-error-when-compiling-gcc-atomic-operation-in-32-bit-mode

Comment 14

[Wan-Teh Chang]

blassey: thanks for the info.  Did you try the -march=pentium solution?

The tinderbox seems to be using gcc 4.3.3.  I wonder if we should upgrade
it to gcc 4.3.5 and see if that has a fix for this gcc bug.

We should build libnspr4.so with the -z defs linker flag so that we
can detect this unresolved symbol at an earlier stage.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Taras has further informed me that this is fixed with gcc 4.5, so I'm marking bug 559964 as blocking

Comment 16

[(not currently active) Ted Mielczarek]

I backed this out because it breaks trunk Firefox builds. If we can figure out a sane solution here we can certainly re-land it:
Checking in pr/include/pratom.h;
/cvsroot/mozilla/nsprpub/pr/include/pratom.h,v  <--  pratom.h
new revision: 3.14; previous revision: 3.13
done
Checking in pr/src/misc/prinit.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prinit.c,v  <--  prinit.c
new revision: 3.55; previous revision: 3.54
done
Checking in pr/src/pthreads/ptsynch.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptsynch.c,v  <--  ptsynch.c
new revision: 3.35; previous revision: 3.34
done
Checking in pr/src/pthreads/ptthread.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v  <--  ptthread.c
new revision: 3.87; previous revision: 3.86
done
Checking in pr/src/threads/prtpd.c;
/cvsroot/mozilla/nsprpub/pr/src/threads/prtpd.c,v  <--  prtpd.c
new revision: 3.14; previous revision: 3.13
done
Checking in pr/src/threads/combined/pruthr.c;
/cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c,v  <--  pruthr.c
new revision: 3.39; previous revision: 3.38
done

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Ted, in what manner does it break firefox builds?
There's no record of any problem in this bug.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #17)
> Ted, in what manner does it break firefox builds?
> There's no record of any problem in this bug.

See comments 12 - 15

Comment 19

[Kai Engert [:KaiE:]]

Hmm, we missed coordination. I did a tryserver build that tested nspr485+nss3127, in preparation for landing, and saw the breakage.

The bug for upgrading mozilla-central to a newer NSPR and NSS is 575620 and I conclude we are still blocked by 559964.

Comment 20

[Kai Engert [:KaiE:]]

This bugzilla is for component NSPR:
If the only remaining issue is about landing into mozilla-central, I propose to close this bugzilla, and track the landing in bug 575620.

Comment 21

[Kai Engert [:KaiE:]]

If I understand correctly:

* the patch from this bug has been backed out from NSPR
* someone will do a new patch, now or later
* fixing this bug is an optional enhancement, not a requirement,
  not blocking anyone

Can you please confirm or clarify?
Thanks

Comment 22

[Wan-Teh Chang]

Attachment: Steve's patch for pratom.h (checked in)

In comment 16, Ted also backed out the changes to pratom.h, which are
unrelated to the problem we had with gcc 4.3.3 on the tinderbox.  So I'm
checking the pratom.h changes back in.  While doing that, I added parentheses
"()" around the arguments in the macro definitions to be safe.

Steve, do you remember why you added these type casts?  I guess it's
because PRInt32 is defined as 'int' on Windows, so we get compiler warnings
if we pass a PRInt32* pointer to a function that expects a long * pointer?

I checked in this patch on the NSPR trunk (NSPR 4.8.6).

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

Comment 23

[Wan-Teh Chang]

(In reply to comment #21)
Kai, your understanding is correct.

The build log files mentioned in comment 12 contain these linker errors:

ccache /tools/gcc-4.3.3/installed/bin/g++ -o js  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -pedantic -Wno-long-long -gdwarf-2 -fno-strict-aliasing -pthread -pipe  -DNDEBUG -DTRIMMED -gdwarf-2 -Os -freorder-blocks -fno-reorder-functions -fomit-frame-pointer   js.o jsworkers.o   -lpthread    -Wl,-rpath-link,/bin -Wl,-rpath-link,/lib  -L../../../dist/bin -L../../../dist/lib -L/builds/slave/mozilla-central-linux/build/obj-firefox/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl ../editline/libeditline.a ../libjs_static.a -ldl -lm     
../../../dist/bin/libnspr4.so: undefined reference to `__sync_sub_and_fetch_4'
../../../dist/bin/libnspr4.so: undefined reference to `__sync_add_and_fetch_4'
collect2: ld returned 1 exit status

Based on my web search for the linker error messages, it seems that this can be
fixed by compiling with -march=i686 (or an arch >= i486).  See, for example,
https://bugzilla.redhat.com/show_bug.cgi?id=455712

Is it possible for me to log in to the "Linux mozilla-central build" tinderbox to
take a look?  I can go to Mozilla's office in Mountain View if this cannot be done
remotely using my SSH account.  Thank you.

Comment 24

[Steve Snyder]

(In reply to comment #22)
> Steve, do you remember why you added these type casts?  I guess it's
> because PRInt32 is defined as 'int' on Windows, so we get compiler warnings
> if we pass a PRInt32* pointer to a function that expects a long * pointer?

Yes, the casts are to match the types that Visual Studio is expecting for their intrinsic functions.  I don't recall if errors or just a lot of warnings result from using PRInt32* typed variables as inputs to the intrinsics.

Note that this is for VS2003 & VS2005.  I don't have access to later versions of Microsoft's compiler so I can't attest to their expectations.

http://msdn.microsoft.com/en-us/library/hd9bdb82(v=VS.80).aspx

Comment 25

[Wan-Teh Chang]

Steve: thanks for the reply.  I found that it is NSPR that declares these intrinsic functions:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/pratom.h&rev=3.14&mark=116,119,122,125#113

I also found that the declarations of these functions in MSDN do not have 'volatile'
except for InterlockedExchangeAdd:
http://msdn.microsoft.com/en-us/library/2ddez55b(v=VS.80).aspx
http://msdn.microsoft.com/en-us/library/f24ya7ct(v=VS.80).aspx
http://msdn.microsoft.com/en-us/library/1s26w950(v=VS.80).aspx
http://msdn.microsoft.com/en-us/library/191ca0sk(v=VS.80).aspx

Comment 26

[Aki Sasaki (not active)]

(In reply to comment #23)
> Based on my web search for the linker error messages, it seems that this can be
> fixed by compiling with -march=i686 (or an arch >= i486).  See, for example,
> https://bugzilla.redhat.com/show_bug.cgi?id=455712

I think this can be tested via a mozconfig tweak on Try.
https://wiki.mozilla.org/Build:TryServerAsBranch#How_to_push_to_try
https://wiki.mozilla.org/Build:TryServerAsBranch#Using_a_custom_mozconfig

> Is it possible for me to log in to the "Linux mozilla-central build" tinderbox
> to
> take a look?  I can go to Mozilla's office in Mountain View if this cannot be
> done
> remotely using my SSH account.  Thank you.

This isn't actually a tinderbox, but a pool of slaves behind buildbot that report to tinderbox.m.o which are dynamically allocated to a build on demand.  To upgrade the version of gcc on our linux builders, I believe we need to upgrade gcc on 95 build slaves currently, and that can potentially affect all linux-based builds on all branches.

That shouldn't stop us from upgrading if needed, but does mean we need to test thoroughly when we do.

Comment 27

[Aki Sasaki (not active)]

(In reply to comment #23)
> Is it possible for me to log in to the "Linux mozilla-central build" tinderbox
> to
> take a look?  I can go to Mozilla's office in Mountain View if this cannot be
> done
> remotely using my SSH account.  Thank you.

Sorry, I didn't actually answer your question.

We can take a slave out of the pool and make it available to you via ssh if needed. However, the Try push should probably allow you to do what you need.

Comment 28

[Wan-Teh Chang]

aki: thanks for your reply.  I'd prefer to log in to a buildbot slave via ssh so
that I can perform multiple experiments with gcc quickly.  We should not
need to upgrade gcc to fix this bug.

Comment 29

[Aki Sasaki (not active)]

I took moz2-linux-slave20 out of the pool and ran some experiments for Wan-Teh, since that seemed like it would be the fastest route.

|CFLAGS="-march=i686" ./configure| in mozilla/nsprpub fixed the above linker issue; I sent Wan-Teh email and we'll figure out how to proceed from here.

Comment 30

[Wan-Teh Chang]

Thanks to Aki's help, I have a solution to this bug.

I'm going to test the GCC predefined macro __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
to determine if the built-in atomic functions are supported for the x86 target architecture.
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is added in GCC 4.3, so for GCC 4.2 and older
we won't be using the built-in atomic functions for x86 target.  If that's a problem, I
can also test __i486__.  The problem with __i486__ is that it's not always defined.  For
example, if you specify -march=i686, then __i686__ is defined but __i486__ is not
defined.

NOTE: this issue points out that the GCC used in those buildbot slaves may not be
generating the best x86 code, unless you add -march=i486 or -march=i686 to
CFLAGS.  I hope Mozilla is not building the releases using that GCC.

Comment 31

[Wan-Teh Chang]

Attachment: Test __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 (checked in)

1. Use GCC built-in atomic functions for 64-bit Mac OS X builds.
2. Test __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 (supported by
GCC 4.3 or later) for x86 Linux because the i386 instruction set
doesn't have atomic instructions.

Aki, here are the additional predefined macros if we pass -march=i486
to the GCC used in the buildbot slave:

dhcp-172-22-71-69:~ wtc$ diff temp1 temp2
55a56,58
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
64a68,69
> #define __i486 1
> #define __i486__ 1
105a111
> #define __tune_i486__ 1

Here are the additional predefined macros if we pass -march=i686
to the GCC used in the buildbot slave:

dhcp-172-22-71-69:~ wtc$ diff temp1 temp3
55a56,59
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
> #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
64a69,70
> #define __i686 1
> #define __i686__ 1
86a93,94
> #define __pentiumpro 1
> #define __pentiumpro__ 1

So I think __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 is the most
appropriate predefined macro to test for.

Comment 32

[Aki Sasaki (not active)]

Comment on attachment 456519 [details] [diff] [review]
Test __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 (checked in)

I patched nsprpub and tried again on moz2-linux-slave20 without specifying CFLAGS="-march=i686", and it built and tested fine.

Comment 33

[Wan-Teh Chang]

Comment on attachment 456519 [details] [diff] [review]
Test __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 (checked in)

I decided to fix this bug in NSPR 4.8.6 so that we don't need
to back out the big NSS patch in bug 415565.

I checked in this patch on the NSPR trunk (NSPR 4.8.6).

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

Comment 34

[Wan-Teh Chang]

Attachment: The rest of Steve's patch (checked in)

I checked in the rest of Steve's patch, again, on
the NSPR trunk (NSPR 4.8.6).

Checking in pr/src/misc/prinit.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prinit.c,v  <--  prinit.c
new revision: 3.56; previous revision: 3.55
done
Checking in pr/src/pthreads/ptsynch.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptsynch.c,v  <--  ptsynch.c
new revision: 3.36; previous revision: 3.35
done
Checking in pr/src/pthreads/ptthread.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v  <--  ptthread.c
new revision: 3.88; previous revision: 3.87
done
Checking in pr/src/threads/prtpd.c;
/cvsroot/mozilla/nsprpub/pr/src/threads/prtpd.c,v  <--  prtpd.c
new revision: 3.15; previous revision: 3.14
done
Checking in pr/src/threads/combined/pruthr.c;
/cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c,v  <--  pruthr.c
new revision: 3.40; previous revision: 3.39
done