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.

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.

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

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

Bug 415563: Use new NSPR atomic macros in NSPR Patch contributed by Steve Snyder <swsnyder@snydernet.net>, 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.

Review request ping.

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.

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

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.

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

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

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.

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

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

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

(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

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.

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.

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

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

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

(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

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

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

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

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.

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.

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.

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

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