Use new NSPR atomic macros - NSPR

RESOLVED FIXED in 4.8.6

Status

P2
enhancement
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: swsnyder, Assigned: swsnyder)

Tracking

(Blocks: 1 bug, {perf})

other
4.8.6
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
Summary: Use new NSPR atomic macros → Use new NSPR atomic macros - NSPR
(Assignee)

Comment 1

11 years ago
Created attachment 301302 [details] [diff] [review]
Replace PR_Atomic[operation] function calls with equivelent macros
(Assignee)

Updated

11 years ago
Blocks: 415565
(Assignee)

Updated

11 years ago
Blocks: 415566
(Assignee)

Updated

11 years ago
Blocks: 415568
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.
(Assignee)

Comment 3

11 years ago
Created attachment 301344 [details] [diff] [review]
Reformatted, with CVS
Attachment #301302 - Attachment is obsolete: true
Created attachment 433764 [details] [diff] [review]
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
Attachment #301344 - Attachment is obsolete: true
Attachment #433764 - Flags: review?(wtc)
Created attachment 433765 [details] [diff] [review]
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?
Attachment #433765 - Flags: review?(wtc)
Severity: normal → enhancement
Priority: -- → P2
Attachment #433764 - Flags: review?(wtc) → review+
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.
Attachment #433764 - Attachment description: Steve's patch brought forward to trunk → Steve's patch brought forward to trunk (checked in)
Review request ping.

Comment 9

9 years ago
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.
Attachment #433765 - Flags: review?(wtc) → review-

Updated

9 years ago
Target Milestone: --- → 4.8.5
OK, Wan-Teh, I'll let you take whatever subset of my patch suits your 
judgment and you can commit that.

Comment 11

9 years ago
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.
Attachment #433765 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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

Comment 14

8 years ago
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
Depends on: 559964
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

Comment 19

8 years ago
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

8 years ago
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.

Updated

8 years ago
Blocks: 575620

Comment 21

8 years ago
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

8 years ago
Created attachment 456343 [details] [diff] [review]
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

8 years ago
(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.
Status: REOPENED → ASSIGNED
Target Milestone: 4.8.5 → 4.8.6
(Assignee)

Comment 24

8 years ago
(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 26

8 years ago
(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

8 years ago
(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

8 years ago
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

8 years ago
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

8 years ago
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

8 years ago
Created attachment 456519 [details] [diff] [review]
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.
Attachment #456519 - Flags: review?(aki)

Comment 32

8 years ago
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.
Attachment #456519 - Flags: review?(aki) → review+

Updated

8 years ago
Target Milestone: 4.8.6 → 4.8.7

Comment 33

8 years ago
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
Attachment #456519 - Attachment description: Test __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 → Test __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 (checked in)

Updated

8 years ago
Attachment #433764 - Attachment description: Steve's patch brought forward to trunk (checked in) → Steve's patch brought forward to trunk (checked in and then backed out)

Comment 34

8 years ago
Created attachment 456759 [details] [diff] [review]
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
Attachment #433764 - Attachment is obsolete: true

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: 4.8.7 → 4.8.6

Updated

3 years ago
Blocks: 1256665
You need to log in before you can comment on or make changes to this bug.