Codesize reduction of nsAppRunner.obj

VERIFIED FIXED in mozilla1.9alpha8

Status

Core Graveyard
Cmd-line Features
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

Trunk
mozilla1.9alpha8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

12 years ago
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!
(Assignee)

Comment 1

12 years ago
Created attachment 212946 [details] [diff] [review]
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.
Assignee: nobody → alfredkayser
Status: NEW → ASSIGNED
Attachment #212946 - Flags: review?(benjamin)
(Assignee)

Comment 2

12 years ago
Created attachment 212952 [details] [diff] [review]
Let's do both toolkit and xpfe

Note, this saves about 3.5Kilobytes of codesize on Windows platform.
Attachment #212946 - Attachment is obsolete: true
Attachment #212946 - Flags: review?(benjamin)

Comment 3

12 years ago
> * 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.
(Assignee)

Updated

12 years ago
Attachment #212952 - Flags: review?(benjamin)
(Assignee)

Comment 4

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

Updated

12 years ago
Attachment #212952 - Flags: review?(benjamin) → review+
(Assignee)

Updated

12 years ago
Attachment #212952 - Flags: superreview?(neil)

Comment 5

12 years ago
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.
Attachment #212952 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 6

12 years ago
Created attachment 218404 [details] [diff] [review]
With the two nits from Neil incorporated
Attachment #212952 - Attachment is obsolete: true
(Assignee)

Comment 7

12 years ago
Who can do the checkin for me? Thanks in advance!
Whiteboard: checkin required
(Assignee)

Comment 8

12 years ago
Who can do the check in for me? Thanks in advance, Alfred
(Assignee)

Comment 9

12 years ago
Mark, can you do the checkin of this patch?
This is a simple patch but saves about 3K codesize!
Assignee: alfredkayser → mark
Status: ASSIGNED → NEW

Comment 10

12 years ago
Sure, I'll check in for you when I get to the office.
Assignee: mark → alfredkayser

Comment 11

12 years ago
Checked in on the trunk:
mozilla/toolkit/xre/nsAppRunner.cpp 1.141
mozilla/xpfe/bootstrap/nsAppRunner.cpp 1.453
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: checkin required

Comment 12

12 years ago
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.
I backed this out, since it caused tinderbox orange (btek).

Comment 14

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

"Oops".

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

12 years ago
Created attachment 231290 [details] [diff] [review]
V3: With fixes for the build_id and mainResult

Jag, an updated version of the patch, including the fixes for the two 'oops'...
Attachment #218404 - Attachment is obsolete: true
Attachment #231290 - Flags: superreview?
(Assignee)

Updated

12 years ago
Attachment #231290 - Flags: superreview? → superreview?(jag)

Updated

12 years ago
Attachment #231290 - Flags: superreview?(jag) → superreview+

Comment 16

12 years ago
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)
(Assignee)

Comment 17

12 years ago
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.
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]
mozilla/xpfe/bootstrap/nsAppRunner.cpp 	1.457
mozilla/toolkit/xre/nsAppRunner.cpp 	1.150
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9beta
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: Cmd-line Features → Cmd-line Features
Product: Core → Core Graveyard
QA Contact: cmd-line
You need to log in before you can comment on or make changes to this bug.