change for PermanentAtoms (92141) broke AIX xlC 5.0x builds

RESOLVED FIXED in mozilla0.9.8

Status

()

Core
XPCOM
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Jim Dunn, Assigned: Jim Dunn)

Tracking

Trunk
mozilla0.9.8
Other
AIX
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

16 years ago
The checkin for http://bugzilla.mozilla.org/show_bug.cgi?id=92141
broke the AIX's xlC 5.0x builds, xlC 3.6.4 works fine.  

Here is the output of mozilla/xpcom/tests/TestPermanentAtoms
PASS: string is correct
PASS: atom is not permanent
FAIL: string is correct
PASS: atom is permanent
PASS: atoms are equal

------
A quick fix is:
Index: nsAtomTable.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsAtomTable.cpp,v
retrieving revision 3.44
diff -u -r3.44 nsAtomTable.cpp
--- nsAtomTable.cpp     2001/10/24 23:36:58     3.44
+++ nsAtomTable.cpp     2001/11/29 20:51:59
@@ -180,6 +180,10 @@
 
 NS_IMPL_THREADSAFE_ISUPPORTS1(AtomImpl, nsIAtom)
 
+PermanentAtomImpl::PermanentAtomImpl()
+  : AtomImpl()
+{ }
+
 NS_IMETHODIMP_(nsrefcnt) PermanentAtomImpl::AddRef()
 {
   return 2;
Index: nsAtomTable.h
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsAtomTable.h,v
retrieving revision 1.6
diff -u -r1.6 nsAtomTable.h
--- nsAtomTable.h       2001/10/20 23:18:54     1.6
+++ nsAtomTable.h       2001/11/29 20:51:59
@@ -76,6 +76,8 @@
 
 class PermanentAtomImpl : public AtomImpl {
 public:
+  PermanentAtomImpl();
+
   NS_IMETHOD_(nsrefcnt) AddRef();
   NS_IMETHOD_(nsrefcnt) Release();
 
------------
But dbaron would like to see this constructor either inlined
or ifdef'd AIX for performance reasons.  So I am looking into
that.
FAIL: string is correct
PASS: atom is permanent
PASS: string is correct
FAIL: atom is permanent
FAIL: atoms are equal
(Assignee)

Comment 1

16 years ago
accepting
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
(Assignee)

Comment 2

16 years ago
dbaron... how about this diff?  will you sr= or r= it?
I am just inlining the constructor

Index: nsAtomTable.h
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsAtomTable.h,v
retrieving revision 1.6
diff -u -r1.6 nsAtomTable.h
--- nsAtomTable.h       20 Oct 2001 23:18:54 -0000      1.6
+++ nsAtomTable.h       5 Dec 2001 11:55:53 -0000
@@ -76,6 +76,7 @@
 
 class PermanentAtomImpl : public AtomImpl {
 public:
+  inline PermanentAtomImpl() : AtomImpl() { return; };
   NS_IMETHOD_(nsrefcnt) AddRef();
   NS_IMETHOD_(nsrefcnt) Release();
Summary: change for 92141 broke AIX xlC 5.0x builds → change for PermanentAtoms (92141) broke AIX xlC 5.0x builds
r=dbaron if you change the added line to:

PermanentAtomImpl() : AtomImpl() {}

(The differences are:
 * no "inline" (it's implied for any function written within a class definition)
 * no "return;"
 * no trailing ";"
)
(Assignee)

Comment 4

16 years ago
scc, shaver or brendan;  I sure could use a sr= from one of you.
Basically the nsPermanentAtom change broke aix (version 5 of the
compiler).  It didn't like not having a constructor for the
replacement new object.  Here is the revised diff based on
dbaron's comments.

Index: nsAtomTable.h
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsAtomTable.h,v
retrieving revision 1.6
diff -u -r1.6 nsAtomTable.h
--- nsAtomTable.h       20 Oct 2001 23:18:54 -0000      1.6
+++ nsAtomTable.h       6 Dec 2001 10:55:20 -0000
@@ -76,6 +76,7 @@
 
 class PermanentAtomImpl : public AtomImpl {
 public:
+  PermanentAtomImpl() : AtomImpl() {}
   NS_IMETHOD_(nsrefcnt) AddRef();
   NS_IMETHOD_(nsrefcnt) Release();
 
sr=shaver.  (Boy, I'd love to have put that on an attachment.)

Comment 6

16 years ago
One should modify the keywords here: missing patch, review etc.. so that it
doesn't disappear in the still huge list of bug for 0.9.7.
Bug has simple fix that should perhaps be checked in? 
(Assignee)

Comment 7

16 years ago
this didn't make 0.9.7...
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Comment 8

16 years ago
fix checked in 
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 9

16 years ago
This checkin caused startup to go from 6004ms to 6046ms : a .7% increase

Is that for real ? c++ experts ?
 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 10

16 years ago
I'm skeptical. jdunn: can you put an AIX-specific #ifdef around this temporarily
to see if startup time drops back down? (If so, we can dig further.)
Only one of the 3 startup tinderboxes changed.
(Assignee)

Comment 12

16 years ago
I checked in the ifdef... so this morning's build should
have it

Comment 13

16 years ago
Cool. I see sleestack startup time go down from 6025ms -> 5993ms (-33msec)

Explanation ? Maybe this is the cost diff betn having your dummy consturctor and
the default constructor. Atoms are used heavily and small cost diff added up.
Would it be a mistake to conclude that we should use #ifdefs in general to work
around specific platform/compiler bugs, with big ugly comments?  Otherwise, this
case shows that we risk regressing the platforms that didn't need the workaround.

/be
(Assignee)

Comment 15

16 years ago
For the record I still am not sure who is right in this case.
Since you guys are much smarter than I, I will assume that the
AIX compiler is wrong, but that is a concession, not something
I know for fact.  Just cause all the other compilers compile
this (who heard of creating a class, especially a replacement new
class without a constructor)...
whatever, you win, i concede... ifdef is in (it is ugly)
you guys have to live with it (xpcom AINT my code!)
;-)
(Assignee)

Comment 16

16 years ago
ifdef AIX checked in... marking fixed...
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.