With this we trim our binary size by 4Mb on both android and maemo and get across the board speed ups according to fennecbench (5% win for panning, 17% for page load lag etc.)

This patch will attempt to build thumb2 on the N810s (which are mistakenly considered maemo 5)

these nspr and js patches only enable thumb2 by default for android and maemo 6 (not maemo 5) based on mfinkle's comment. I'll make the same change to the main configure patch when pushing. --enable-thumb2 will still work for maemo 5 if passed to configure

Comment on attachment 443482 [details] [diff] [review] nspr patch ># HG changeset patch ># User Brad Lassey <blassey@mozilla.com> ># Date 1273012775 14400 ># Node ID c04eed45c9f4be290fcaedf2c7c3a969b3a2396f ># Parent b3d707fb93a6799eb5e08a9ada8dacbbc2c49fd4 >imported patch nspr-thumb2 > >diff -r b3d707fb93a6 -r c04eed45c9f4 nsprpub/configure.in >--- a/nsprpub/configure.in Tue May 04 18:38:03 2010 -0400 >+++ b/nsprpub/configure.in Tue May 04 18:39:35 2010 -0400 > dnl ======================================================== >+dnl = Maemo checks >+dnl ======================================================== >+ >+MAEMO_SDK_TARGET_VER=-1 >+ >+AC_ARG_WITH(maemo-version, >+[ --with-maemo-version=MAEMO_SDK_TARGET_VER >+ Maemo SDK Version], It seems a little weird to add all this machinery here just to enable thumb. Could you drop this from the NSPR configure, and instead just pass --enable-thumb2 to NSPR's sub-configure if MOZ_THUMB2 is set by the top-level configure? >+dnl ======================================================== >+dnl = Enable building the Thumb2 instruction set >+dnl ======================================================== >+AC_ARG_ENABLE(thumb2, >+ [ --enable-thumb2 Enable Thumb2 instruction set], >+ [ if test "$enableval" = "yes"; then >+ MOZ_THUMB2=1, >+ fi ]) >+ >+ >+if test -n "$MOZ_THUMB2"; then Seems like you probably want to check that the target CPU is arm, no? >+ CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ ASFLAGS="$ASFLAGS -march=armv7-a -mthumb" This should check GNU_CC. >+ AC_SUBST(CLFAGS) >+ AC_SUBST(CXXFLAGS) >+ AC_SUBST(ASFLAGS) You don't need these AC_SUBSTs, they're already substituted below. Also,

Comment on attachment 443481 [details] [diff] [review] js patch Basically the same comments as the NSPR patch.

Comment on attachment 443418 [details] [diff] [review] patch ># HG changeset patch ># User Brad Lassey <blassey@mozilla.com> ># Date 1273002047 14400 ># Node ID 6292aa8974cdd5b82d2b196db506619828574297 ># Parent 7939af53a1d16eebdcf7b84a16c5ffc5caf33e1b >[mq]: thumb2-configure-option > >diff -r 7939af53a1d1 -r 6292aa8974cd configure.in >--- a/configure.in Wed Apr 28 17:30:00 2010 -0700 >+++ b/configure.in Tue May 04 15:40:47 2010 -0400 >@@ -4915,6 +4915,7 @@ > MOZ_SPLASHSCREEN= > MOZ_STORAGE=1 > MOZ_SVG=1 >+MOZ_THUMB2= > MOZ_TIMELINE= > MOZ_TOOLKIT_SEARCH=1 > MOZ_UI_LOCALE=en-US >@@ -6766,7 +6767,11 @@ > if test -z "$_LIB_FOUND"; then > AC_MSG_ERROR([Hildon FM-2 is required when building for Maemo]) > fi >- >+ MOZ_THUMB2=1 >+ fi >+ >+ if test $MOZ_PLATFORM_MAEMO = 6; then >+ MOZ_THUMB2=1 > fi Might make sense to just make a separate block that says: if test "$MOZ_PLATFORM_MAEMO" -gt 5; then MOZ_THUMB2=1 fi > dnl ======================================================== >+dnl = Enable building the Thumb2 instruction set >+dnl ======================================================== >+MOZ_ARG_ENABLE_BOOL(thumb2, >+ [ --enable-thumb2 Enable Thumb2 instruction set], >+ MOZ_THUMB2=1,) >+ >+if test -n "$MOZ_THUMB2"; then >+ CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ ASFLAGS="$ASFLAGS -march=armv7-a -mthumb" >+ AC_SUBST(CLFAGS) >+ AC_SUBST(CXXFLAGS) >+ AC_SUBST(ASFLAGS) These already get AC_SUBSTed later in the file. (Also: CLFAGS?)

Comment on attachment 447030 [details] [diff] [review] patch v.2 >+ if test "$MOZ_PLATFORM_MAEMO" -ge 5; then just realized that if we don't want thumb2 builds for the n810 we should change this to -gt

Comment on attachment 447029 [details] [diff] [review] nspr patch v.2 >diff -r 28c9048d8ae4 -r 5aa90111be10 nsprpub/configure.in >--- a/nsprpub/configure.in Tue May 04 15:40:47 2010 -0400 >+++ b/nsprpub/configure.in Tue May 04 18:39:35 2010 -0400 >+if test -n "$MOZ_THUMB2"; then >+ case "$target" in >+ *arm*) I think you can just do case "$target_cpu" in arm) can't you? >+ if test "$GNU_CC"; then >+ CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ ASFLAGS="$ASFLAGS -march=armv7-a -mthumb" >+ else >+ AC_MSG_ERROR([--enable-thumb2 specified for non-gnu toolchain]) >+ fi >+ ;; >+ *) >+ AC_MSG_ERROR([--enable-thumb2 specified for an arch other than arm]) >+ ;; I'd probably reword these two errors: --enable-thumb2 is not supported for non-GNU toolchains --enable-thumb2 is not supported for non-ARM CPU architectures r=me with those nits fixed.

Comment on attachment 447028 [details] [diff] [review] js patch v.2 r=me with the same nits as the NSPR patch.

Comment on attachment 447030 [details] [diff] [review] patch v.2 ># HG changeset patch ># User Brad Lassey <blassey@mozilla.com> ># Date 1273002047 14400 ># Node ID 28c9048d8ae48cc89ecda89e163956acb74863a6 ># Parent 6ef424688579e4ecd0b7e9c9935262e94522a53c >[mq]: thumb2-configure-option > >diff -r 6ef424688579 -r 28c9048d8ae4 configure.in >--- a/configure.in Fri May 21 15:27:24 2010 -0400 >+++ b/configure.in Tue May 04 15:40:47 2010 -0400 >@@ -5014,6 +5014,7 @@ > MOZ_SPLASHSCREEN= > MOZ_STORAGE=1 > MOZ_SVG=1 >+MOZ_THUMB2= > MOZ_TIMELINE= > MOZ_TOOLKIT_SEARCH=1 > MOZ_UI_LOCALE=en-US >@@ -6854,6 +6855,17 @@ > > fi > >+ if test "$MOZ_PLATFORM_MAEMO" -ge 5; then >+ MOZ_THUMB2=1 >+ case "$ac_configure_args" in >+ *--disable-thumb2*) >+ ;; >+ *) >+ _SUBDIR_CONFIG_ARGS="$_SUBDIR_CONFIG_ARGS --enable-thumb2" >+ ;; >+ esac You don't need to set this way up here, you can just check MOZ_THUMB2 right before we run the sub-configures, and add to ac_configure_args there: http://mxr.mozilla.org/mozilla-central/source/configure.in#8963 That should make that a bit cleaner.

carrying r+

carrying r+

Comment on attachment 447132 [details] [diff] [review] nspr patch, nits addressed Landed in NSPR CVS: Checking in configure; /cvsroot/mozilla/nsprpub/configure,v <-- configure new revision: 1.274; previous revision: 1.273 done Checking in configure.in; /cvsroot/mozilla/nsprpub/configure.in,v <-- configure.in new revision: 1.278; previous revision: 1.277 done

Comment on attachment 447140 [details] [diff] [review] patch v.3 >+if test -n "$MOZ_THUMB2"; then >+ case "$target" in $target_cpu like the other patches. >+ *arm*) >+ if test "$GNU_CC"; then >+ CFLAGS="$CFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ CXXFLAGS="$CXXFLAGS -march=armv7-a -mthumb -Wa, -march=armv7-a -Wa, -mthumb" >+ ASFLAGS="$ASFLAGS -march=armv7-a -mthumb" >+ else >+ AC_MSG_ERROR([--enable-thumb2 specified for non-gnu toolchain]) >+ fi >+ ;; >+ *) >+ AC_MSG_ERROR([--enable-thumb2 specified for an arch other than arm]) Fix the error messages to match the other patches. >@@ -8977,6 +9007,9 @@ > if test -n "$USE_ARM_KUSER"; then > ac_configure_args="$ac_configure_args --with-arm-kuser" > fi >+ if test -n "$MOZ_THUMB2"; then >+ ac_configure_args="$ac_configure_args --enable-thumb2" >+ fi > AC_OUTPUT_SUBDIRS(nsprpub) > ac_configure_args="$_SUBDIR_CONFIG_ARGS" > fi I think you need to add this block to the js/src/configure invocation as well. r=me with those fixes.

I tagged NSPR CVS with NSPR_HEAD_20100524. rs=me to update the m-c copy.

tried pushing that nspr tag, but it caused linux 32 bit builders to burn: 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

pushed http://hg.mozilla.org/mozilla-central/rev/5b6f96b1a706 and http://hg.mozilla.org/mozilla-central/rev/c3af7e83e26f going to close this out, but still need to get nspr updated