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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jim Dunn]

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

Comment 1

[Jim Dunn]

accepting

Comment 2

[Jim Dunn]

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();

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 ";"
)

Comment 4

[Jim Dunn]

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();
 

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

sr=shaver.  (Boy, I'd love to have put that on an attachment.)

Comment 6

[Jerome Lacoste]

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? 

Comment 7

[Jim Dunn]

this didn't make 0.9.7...

Comment 8

[Jim Dunn]

fix checked in 

Comment 9

[Suresh Duddi (gone)]

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

Is that for real ? c++ experts ?
 

Comment 10

[Chris Waterson]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Only one of the 3 startup tinderboxes changed.

Comment 12

[Jim Dunn]

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

Comment 13

[Suresh Duddi (gone)]

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.

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Jim Dunn]

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!)
;-)

Comment 16

[Jim Dunn]

ifdef AIX checked in... marking fixed...