Closed Bug 369007 Opened 17 years ago Closed 16 years ago

Enable high-memory feature on OS/2 by default

Categories

(Firefox Build System :: General, defect)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

References

Details

Attachments

(2 files, 1 obsolete file)

As promised in bug 351246#c128 this is the follow-up bug.

After we got enough testing, we should switch on the high-mem feature by default. And maybe add a switch so that people compiling with GCC 3.2.2 get a warning, as it probably doesn't work well enough with that compiler.

mkaply is already working on converting the nightly builds machine to produce highmem nightlies.

Let's revisit this in April or so...
I see one concern about enabling high-mem:
The java131 plugin crashes here!
From POPUPLOG.OS2

01-10-2007  01:30:20  SYS2070  PID 0059  TID 0001  Slot 00a3
D:\PROGRAMS\FIREFOX30\FIREFOX.EXE
NPOJI6->XPCOM.__dt__13nsCOMPtr_baseFv
127
------------------------------------------------------------

01-10-2007  01:30:38  SYS2070  PID 005c  TID 0001  Slot 00a3
D:\PROGRAMS\FIREFOX30\FIREFOX.EXE
NPOJI6->XPCOM.__vft17nsGetServiceByCID15nsCOMPtr_helper
127
------------------------------------------------------------

I see no problem with nightlies that were not high-mem enabled (before the date in comment #1).
However, no problems with java-1.4.2_09 plugin in high-mem builds.
The symbols listed in the log are weird, they don't correspond to any actual symbols in the code, there is no function nsCOMPtr_base::_dt(void) and to my knowledge never has been. Hmm, perhaps that is that something that ipluginw.dll creates?

I haven't heard anybody complain about that for my 1.8.1 branch highmem builds, so perhaps it's rather a problem of needing an updated plugin wrapper library...
(In reply to comment #3)
> 
> I haven't heard anybody complain about that for my 1.8.1 branch highmem builds,
> 
Peter, finally I installed Seamonkey-1.1.1+ highmem-enabled and I see the same crash with the IBM-1.3.1., but not with the innotek 1.4.2 java plugin. In this case however there is nothing in POPUPLOG.OS2. Maybe people in the newsgroup using your highmem-builds don't have java131 installed.
When I look at https://bugzilla.mozilla.org/show_bug.cgi?id=348533#c13 the old java plugin and its VAC code seems to make trouble also in getting a working xulrunner. Maybe we should make a poll if we should drop support for the ancient java plugin.
> The java131 plugin crashes here!
> From POPUPLOG.OS2
> 
> NPOJI6->XPCOM.__dt__13nsCOMPtr_baseFv
http://mxr.mozilla.org/seamonkey/source/xpcom/stub/nsOS2VACLegacy.cpp#182
> NPOJI6->XPCOM.__vft17nsGetServiceByCID15nsCOMPtr_helper

http://mxr.mozilla.org/seamonkey/source/xpcom/stub/nsOS2VACLegacy.cpp#446

Just for the records, these errors are related to nsOS2VACLegacy.cpp
Ah, so those things are not mangled C++ symbols that I was looking for but mangled-by-hand assembler functions. :-) As said in the other bug we should just forget about old Java. Both Innotek and GoldenCode Java seem to work fine with highmem, so that should not keep us from making it the default. The question is, does it also crash the browser when not linking nsOS2VACLegacy.cpp? Perhaps I should make a test-DLL...

I haven't actually heard anybody complain about my high-mem builds in quite a while, so once that test is done, we should switch it on by default.
Attached patch do it (obsolete) — Splinter Review
I don't pretend to understand all the magic use in configure.in but this seems to work and should not affect other platforms.
Attachment #316326 - Flags: review?(ted.mielczarek)
Comment on attachment 316326 [details] [diff] [review]
do it

+if test "$OS_ARCH" = "OS2" ; then
+    MOZ_OS2_HIGH_MEMORY=1
+fi

You don't need the if block there, this variable is only used on OS/2 anyway, right?

I can't review the NSPR/c-sdk bits, you'll have to ask peers of those modules. (Although the changes themselves look sane.)
Attachment #316326 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 316326 [details] [diff] [review]
do it

Mark, can you please check the changes in directory/c-sdk/configure.in from the LDAP point of view? (Would I need sr for that component, too?)
Attachment #316326 - Flags: review?(mcs)
Comment on attachment 316326 [details] [diff] [review]
do it

Wan-Teh, your comments on the change in nsprpub/configure.in, please? (Does one still need sr for NSPR?)
Attachment #316326 - Flags: review?(wtc)
Ted, yes you are right, I could remove the OS2 condition. All uses of MOZ_OS2_HIGH_MEMORY are only checked in OS/2 code or OS/2 sections of Makefiles. If the reviewers of the other components agree I will upload a new patch with that change.
Comment on attachment 316326 [details] [diff] [review]
do it

r=wtc on the nsprpub/configure.in change.

Please move the initialization of MOZ_OS2_HIGH_MEMORY=1 to
the beginning of nsprpub/configure.in (you can find the right place
by search for "dnl = Defaults") without the "$OS_ARCH" = "OS2"
test.
Attachment #316326 - Flags: review?(wtc) → review+
Added Rich to the bug CC (he has worked on the LDAP C SDK configure code somewhat recently).
Comment on attachment 316326 [details] [diff] [review]
do it

I think the LDAP C SDK changes are OK.  I do not think you need an sr for these changes.
Attachment #316326 - Flags: review?(mcs) → review+
OK, thanks for the reviews. I changed the location of the MOZ_OS2_HIGH_MEMORY=1 as suggested by Wan-Teh (moved near the top of the file in the listing of defaults), and as the LDAP configure.in had a similar listing I did that there, too. And removed the OS/2 check in all cases.

I guess I should ask for approval for the changes to Mozilla files (configure.in and xpcom/stub/Makefile.in). Both changes should just affect OS/2 even though they are in cross-platform files.

Do I see correctly that for NSPR I just have to check this into trunk? And LDAP should get this into trunk and LDAPCSDK_6_0_3_CLIENT_BRANCH?
Attachment #316326 - Attachment is obsolete: true
Attachment #318241 - Flags: review+
Attachment #318241 - Flags: approval1.9?
Comment on attachment 318241 [details] [diff] [review]
patch with suggested changes

a1.9+=damons
Attachment #318241 - Flags: approval1.9? → approval1.9+
For NSPR, it's a three-step process:
1. Check in the NSPR change on the NSPR trunk.
2. Create a new CVS static tag for the NSPR trunk.
3. Change mozilla/client.mk to set NSPR_CO_TAG to this new CVS tag.

I checked in your patch on the NSPR trunk (NSPR 4.7.1).

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.233; previous revision: 1.232
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.229; previous revision: 1.228
done

I will create a new CVS tag later.
Thanks, Wan-Teh.

I have now checked in the LDAP change, too.
- LDAP branch:
   Checking in configure;
   /cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
   new revision: 5.60.2.9; previous revision: 5.60.2.8
   done
   Checking in configure.in;
   /cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
   new revision: 5.54.2.9; previous revision: 5.54.2.8
   done

- LDAP trunk:
   Checking in configure;
   /cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
   new revision: 5.68; previous revision: 5.67
   done
   Checking in configure.in;
   /cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
   new revision: 5.62; previous revision: 5.61
   done

For the Mozilla changes I think I have to wait for the Firefox tinderboxes to get fixed.
Checked in Mozilla changes to trunk:

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1990; previous revision: 1.1989
done
Checking in xpcom/stub/Makefile.in;
/cvsroot/mozilla/xpcom/stub/Makefile.in,v  <--  Makefile.in
new revision: 1.24; previous revision: 1.23
done

As the changes are all in now, I'm marking this fixed.

Until the NSPR tag has been created (Wan-Teh, I better leave that in your hands) and made it into client.mk we have to remember to explicitely specify --enable--os2-high-mem or --disable-os2-high-mem. Will post to the newsgroup with that warning.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This will also bring in three other changes:
- bug 218846 (does not affect Mozilla clients, which don't call PR_Cleanup)
- bug 405992 (affects Linux/ARM only, and only if --enable-arm-kuser is specified)
- bug 430883 (Solaris only minor makefile cleanup)
Attachment #318844 - Flags: approval1.9?
Comment on attachment 318844 [details] [diff] [review]
Upgrade to NSPR_4_7_1_RTM

wtc, can we get a new bug on this, nominate it for blocking, and explain why we want to take it (even if it's just the OS/2 changes)
Attachment #318844 - Flags: approval1.9? → approval1.9-
Blocks: 431759
The new NSPR tag just made it into client.mk in bug 431759.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: