328357 - Codesize reduction of nsAppRunner.obj

Status: VERIFIED FIXED

(Core Graveyard :: Cmd-line Features, defect)

Description

[Alfred Kayser]

Easy and quick codesize reduction of nsAppRunner.obj by more than 3 Kilobytes.

* Replace all multiple printf's with one printf with multiple strings
* Replace %s, HELP_SPACER_* with plain \t in the strings themselves.
* Inline two small functions only called from one place.
* Cleanup DumpVersion

Note, no functionality change at all!

Comment 1

[Alfred Kayser]

Attachment: Quick, cheap and easy codesize reduction!

Note, same kind of patch applies also to /toolkit/xre/nsAppRunner.cpp, where an allmost the same version of nsAppRunner.cpp exists.

Comment 2

[Alfred Kayser]

Attachment: Let's do both toolkit and xpfe

Note, this saves about 3.5Kilobytes of codesize on Windows platform.

Comment 3

[Jesse Ruderman]

> * Replace %s, HELP_SPACER_* with plain \t in the strings themselves.

You could keep the string macros, and allow the text to line up in the source, with code like:

printf("foo"      HELP_SPACER_2  "bar");
printf("foo baz"  HELP_SPACER_2  "bar foopy");

If you had a reason for removing the macros, feel free to ignore me.

Comment 4

[Alfred Kayser]

The macros are removed because it just hinders code readability, and doesn't add any value.

Comment 5

[[e-mail]]

Comment on attachment 212952 [details] [diff] [review]
Let's do both toolkit and xpfe

>+  if (!arg || arg[0]=='\0' || arg[1]=='\0')
Nit: spaces around ==

>+  printf("%s %s, Copyright (c) 2003-2006 mozilla.org", 
>+         NS_STRINGIFY(MOZ_APP_DISPLAYNAME), NS_STRINGIFY(MOZ_APP_VERSION));
> 
>-  printf("%s %s, Copyright (c) 2003-2006 mozilla.org", NS_STRINGIFY(MOZ_APP_DISPLAYNAME), NS_STRINGIFY(MOZ_APP_VERSION));
>-
>-  if(buildID) {
>-    printf(", build %u\n", (unsigned int)buildID);
>+  if(NS_BUILD_ID) {
>+    printf(", build %u\n", (unsigned int)NS_BUILD_ID);
>   } else {
>     printf(" <developer build>\n");
>   }
I was going to nit you for missing the space between if and (, but then it occurred to me that this should be an #ifdef anyway. sr=me with that fixed. You could even drop the #ifdef inside the previous printf if you don't think that would be too confusing.

Comment 6

[Alfred Kayser]

Attachment: With the two nits from Neil incorporated

Comment 7

[Alfred Kayser]

Who can do the checkin for me? Thanks in advance!

Comment 8

[Alfred Kayser]

Who can do the check in for me? Thanks in advance, Alfred

Comment 9

[Alfred Kayser]

Mark, can you do the checkin of this patch?
This is a simple patch but saves about 3K codesize!

Comment 10

[Mark Mentovai]

Sure, I'll check in for you when I get to the office.

Comment 11

[Mark Mentovai]

Checked in on the trunk:
mozilla/toolkit/xre/nsAppRunner.cpp 1.141
mozilla/xpfe/bootstrap/nsAppRunner.cpp 1.453

Comment 12

[jag (Peter Annema)]

I think NS_BUILD_ID is always defined, but to 0000000000 when it's not an official build. So the original test was correct (and the compiler can optimize away |if (0) { ... }|). #if NS_BUILD_ID would probably also be fine.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I backed this out, since it caused tinderbox orange (btek).

Comment 14

[jag (Peter Annema)]

-  return TranslateReturnValue(mainResult);
+  return NS_FAILED(rv) ? 1 : 0;

"Oops".

Comment 15

[Alfred Kayser]

Attachment: V3: With fixes for the build_id and mainResult

Jag, an updated version of the patch, including the fixes for the two 'oops'...

Comment 16

[[e-mail]]

Comment on attachment 231290 [details] [diff] [review]
V3: With fixes for the build_id and mainResult

>-static nsresult DumpVersion(char *appname)
>+static inline void DumpVersion()
> {
>-  nsresult rv = NS_OK;
>   long buildID = NS_BUILD_ID;  // 10-digit number
>-
>-  printf("%s %s, Copyright (c) 2003-2006 mozilla.org", NS_STRINGIFY(MOZ_APP_DISPLAYNAME), NS_STRINGIFY(MOZ_APP_VERSION));
>-
>+  printf("%s %s, Copyright (c) 2003-2006 mozilla.org", 
>+         NS_STRINGIFY(MOZ_APP_DISPLAYNAME), NS_STRINGIFY(MOZ_APP_VERSION));
>   if(buildID) {
>     printf(", build %u\n", (unsigned int)buildID);
>   } else {
>     printf(" <developer build>\n");
>   }
>-
>-  return rv;
> }
You could have used the v1 version here (i.e. replacing buildID with NS_BUILD_ID) but if you do don't forget to the space: if (NS_BUILD_ID)

Comment 17

[Alfred Kayser]

Just to be sure I kept the 'build_ID' code with the old code, as any change don't provide any benefits (except code cleaning...) but as seen above can have unwanted side effects.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

mozilla/xpfe/bootstrap/nsAppRunner.cpp 	1.457
mozilla/toolkit/xre/nsAppRunner.cpp 	1.150