4830 - [ongoing] Turn off traceback tables in IDE_Options.h before each release

Status: VERIFIED WONTFIX

(SeaMonkey :: Build Config, defect, P1)

Description

[Pierre Saslawsky]

We have the following line in IDE_Options.h:

     #pragma traceback    on     /* leave on until the final release, so MacsBug
logs are interpretable */


This option must be turned off before we ship the final release.

Comment 1

[Pierre Saslawsky]

After today's discussion on <macdev> and since nobody objected, the Macsbug
symbols will be turned off for each Milestone release. They will stay on between
releases. It's not clear yet when exactly the option will be turned off but I
think it would more logical to do it on the branches that are created for the
milestones.

Setting Milestone to M6. When it's done for M6, the bug will be pushed to M7
etc...

Comment 2

[Pierre Saslawsky]

Chris, could you take care of that one? After creating a branch for a milestone
release, turn the 'traceback' off in IDE_Options.h and push the bug to the next
milestone. Thanks.

Comment 3

[Chris Yeh]

turned off traceback tables in SeaMonkey_M6_BRANCH. moved bug to M7.

Comment 4

[Chris Yeh]

i missed this bug before we cut the candidates for M7. I don't want to make this
change since we're done. It only affects size. Moving to M8.

Comment 5

[Chris Yeh]

as per chofmann, we should only do this for the beta, since it's useful to get
traceback tables for the milestone builds.

moving target fix milestone to M10

Comment 6

[Chris Yeh]

setting to m11/beta

Comment 7

[chris hofmann]

i think we just need to do this for beta.  moving to m14

Comment 8

[chris hofmann]

just beta baybee...

Comment 9

[Chris Yeh]

changed keyword to beta1, since we will want to do this to keep the binary size 
as small as possible.

Comment 10

[leger]

Putting on PDT+ radar for beta1.

Comment 11

[Chris Yeh]

remove pp, this only applies to mac

Comment 12

[Chris Yeh]

re-assign bug to jj, this will happen when i am out.

Comment 13

[J.J. Enser]

will flip the switch as soon as the beta branch is cut.
-- note: I presume that this must affect IDE_options.h in both worlds: mozilla/
build/mac & ns/build/mac

Comment 14

[Simon Fraser [no longer active]]

Yes. When you do this, make sure you only turn them off for non-DEBUG builds, and 
also turn them back on as soon as you have done the release build. We don't want 
to have a long period of time where optimized QA builds are useless for 
generating stack crawls.

Comment 15

[leger]

Need to fix on beta branch.

Comment 16

[J.J. Enser]

Simon: Yep, I was aware of the <#ifdef DEBUG> sections.
Question: Even when we make the change in IDE_options.h on the M14 branch, we 
cannot afford to keep the option turned off, even on the branch only, for the 
sake of our own debugging needs. Is there any way to flip the switch 
programatically, or substitute the entire file with another one when we do 
milestone builds ?
This would prevent successive (and 'opposite') checkins. 

Comment 17

[Simon Fraser [no longer active]]

This is obviously a compile-time option, so the only way to set it 
'programatically' for the release build would be to have the Perl scripts mess 
with IDE_options.h, or another include file.

Comment 18

[J.J. Enser]

according to chofmann, we won't respin M14 so this will only happen starting with 
M15, and I'll flip the switch as soon as we build milestone "candidates".

Comment 19

[Jim Roskind]

We have branched... please update the status whiteboard with the landing plan date.
Thanks,
Jim

Comment 20

[J.J. Enser]

Fixed on nscp_beta1_BRANCH. <#pragma traceback off> (optimized)

Comment 21

[chris hofmann]

We should run page loading anbd performance tests on the 
first build with this change (3/13 PM branch builds)
to see if we helped out any with this optimization.

adding leger to cc

Comment 22

[Simon Fraser [no longer active]]

The performance effects of this change will be imperceptible, I think. It should 
reduce code size by 5% or so, which might affect code cacheing by the CPU a 
little, but I wouldn't bother to run extra performance tests just for this.

Comment 23

[leger]

Todays Mac perf data for 
ftp://sweetlou/products/client/seamonkey/macos/8.x/ppc/2000-03-14-12-M15
looks good.  Not any better, but not any worse.  marking  Verified.

Comment 24

[Simon Fraser [no longer active]]

A quick size comparison of the DLLs before and after the change would be 
interesting, but not vital.

Comment 25

[J.J. Enser]

Actually, I just realized that the change was checked in the ns tree only. 
Updated mozilla.build/mac/IDE_Options.h with same fix.

Library size can be compared by looking at the "_sizes.txt" file pushed next to 
each build on sweetlou.

Comment 26

[Pierre Saslawsky]

Reopening: this bug is a reminder. It should never be closed but pushed from a 
milestone to the next after doing the requested change on the branch that's going 
to be shipped.

jj- we need some data on the size of the binaries before and after this change. 
The only way to verify whether it is indeed on the branch is to compare the file 
sizes. If it worked, you can replace beta1 with beta2 and remove the PDT+ tag.

Comment 27

[chris hofmann]

ok pierre, add update to -> beta2

Comment 28

[chris hofmann]

removing the pdt+ so it gets off the beta1 radar.

Comment 29

[chris hofmann]

Attachment: appearent savings when turning off traceback tables in beta1

Comment 30

[J.J. Enser]

fixed on the SeaMonkey_M15_BRANCH. moving bug to M16 [reminder]

Comment 31

[Chris Yeh]

mass re-assign of all bugs where i was listed as the qa contact

Comment 32

[leger]

Putting on [nsbeta2+] radar for beta2 fix.

Comment 33

[J.J. Enser]

fixed on 6/7 on the SeaMonkey_M16_BRANCH. moving bug to M17 [reminder]

Comment 34

[J.J. Enser]

fixed on M17 Branch. moving target milestone to M18

Comment 35

[chris hofmann]

taking the beta2- off since this is now a tracking bug for the next release.

Comment 36

[J.J. Enser]

Reassigning to Leaf for M18 since branching will happen during my vacation.

Apply this diff to line 75 (but NOT to line 69)
< #pragma traceback 			on
--
> #pragma traceback 			off

Comment 37

[Phil Peterson]

PDT agrees P1 on B3 branch

Comment 38

[lchiang]

per email w/PDT (and Phil's previous comments), marking nsbeta3++

Comment 39

[Jon Granrose]

I'm on this.

Comment 40

[Jon Granrose]

I have the diff, will check in to the mozilla and ns branch when the branch is open.

Comment 41

[lchiang]

per PDT:  will not hold nsbeta3 for.  Marking nsbeta3- and nominate for rtm.   
Let us know (email: [e-mail]) if you get a fix, allowing for the 
possiblity of still taking a super-safe fix if we have to respin for something 
else

Comment 42

[Jon Granrose]

I do have the fix for this but since we're not respinning mac, and we're
continuing to do daily builds off this branch til rtm I'm thinking we should
just leave this on for PR3.

Comment 43

[Pierre Saslawsky]

Lisa and Jon: you misunderstood what this bug is for!!! So, one more time, in 
caps so that future generations don't miss it...

********************************************************************************
********************************************************************************
********************************************************************************
********************************************************************************
********************************************************************************

   I M P O R T A N T    ---    I M P O R T A N T   ---    I M P O R T A N T 
   I M P O R T A N T    ---    I M P O R T A N T   ---    I M P O R T A N T 
   I M P O R T A N T    ---    I M P O R T A N T   ---    I M P O R T A N T 
   I M P O R T A N T    ---    I M P O R T A N T   ---    I M P O R T A N T 

********************************************************************************
********************************************************************************
********************************************************************************
********************************************************************************
********************************************************************************

THIS BUG IS A REMINDER TO THE INTENT OF THE BUILD TEAM SO THAT WE DON'T FORGET TO 
TURN OFF A COMPILER OPTION EACH TIME WE GENERATE BITS THAT GO ON THE WIRE.

THE COMPILER OPTION MUST BE TURNED OFF EACH TIME WE SHIP SOMETHING.

IF THE CODE WE SHIP IS ON A BRANCH, THE OPTION MUST BE TURNED OFF ON THE BRANCH 
ONLY - NOT ON THE TRUNK.

IT MUST BE TURNED OFF JUST BEFORE WE SHIP, THEN IT MUST BE TURNED BACK ON JUST 
AFTER THE BITS ARE GENERATED, AND THEN THE BUG MUST BE PUSHED TO THE NEXT 
MILESTONE SO THAT WE DON'T FORGET TO DO THE SAME THING NEXT TIME.

          THIS BUG MUST NEVER BE CLOSED.
          THIS BUG MUST NEVER BE CLOSED.
          THIS BUG MUST NEVER BE CLOSED.
          THIS BUG MUST NEVER BE CLOSED.

THIS BUG MUST BE ALWAYS APPROVED FOR CHECKIN. IT IS PART OF THE BUILD TEAM 
PROCESS TO GENERATE THE BITS. IT IS NOT SOMETHING THAT ENGINEERING CAN APPROVE OR 
DENY.

********************************************************************************
********************************************************************************
********************************************************************************
********************************************************************************
********************************************************************************

Comment 44

[Pierre Saslawsky]

JJ Enser used to take care of that. I think he usally turned the option off on 
the branches 2 or 3 days, maybe a week, before we released the bits.

Once you have done it for a particular release - let's say beta3 - you can clear 
the corresponding keywords and move the bug to the next milestone.

Of course, if you have a better idea not to forget about that thing, suggestions 
are welcome...

Comment 45

[lchiang]

Your idea is fine.  PDT was not aware that this is a must-fix.  I've emailed PDT.

Comment 46

[lchiang]

What are the side effect(s) if this is not fixed in a beta?  Size?  Performance?

Comment 47

[Scott Collins]

Lisa, the effect of this fix is to remove large hunks of debugging info from
each the build libraries.  This is a savings in space, at no cost in runtime
performance.  The savings usually average at least 5%, or nearly a megabyte
(given the current numbes) off of our download size and RAM footprint.  In my
opinion, this is a very important thing to do for release bits.

Comment 48

[Peter Trudelle]

We discussed this in PDT yesterday.  The savings in RAM would be nice, but IIRC,
it introduces a small chance of uncovering existing defects that are sensitive
to the location of code or data in RAM. For instance, a stray write that is now
clobbering traceback data could start affecting something more critical.
Removing this data would thus require retesting of the entire app.  Such changes
are okay a few days before the bits freeze, but not once we have final
candidates. We don't respin for 5% improvement, even if near zero risk. This
should have been done early in the week, but now.
nsbeta3-.  Let's be sure to get this done for RTM, preferably soon after 10/16.

Comment 49

[Pierre Saslawsky]

Clearing [nsbeta3-] for reconsideration. The saving is not 5% as Scott but MUCH 
larger, up to 30% for Layout which is the one where the saving is the largest, I 
think. For instance in PR2 (generated without debug info), the Layout shared lib 
takes 3.5Mb while a daily build from last week (generated with debug info) was 
taking 4.9Mb. We certainly added some code in Layout between August 17th and 
September 22nd but not to the extent of 40% more.

Comment 50

[Peter Trudelle]

How much savings overall?  What is the delta for disk footprint & bloat?

Comment 51

[Pierre Saslawsky]

These are questions for the build team. They can generate a build with traceback 
tables, one without and compare. The only things I can compare are a PR2 build 
with a recent nightly build. It shows the following sizes for the largest shared 
libs:
               PR2      Nightly build     Diff
Layout        3.5Mb       4.9Mb            40%
Editor Core   784K        948K             21%
RDF           676         816              21%
Necko         600         764              27%
IMAP          516         640              24%
MailNews      512         664              29%
html parser   480         588              22%

Apparently, the code is between 20 and 30% bigger - and up to 40% - with 
traceback tables.

Comment 52

[Pierre Saslawsky]

Result of not turning off this option: the code size increased by 4.8Mb or 23% 
between PR2 (08/07) and PR3 (09/29).


                            PR2                        PR3
                     -------------------        -------------------
Essential Files:      5.3Mb for 32 items         6.2Mb for 32 items
Components:          15.1Mb for 99 items        19.0Mb for 93 items

Comment 53

[Peter Trudelle]

Just curious, since the bits are already gone: How did you isolate the traceback
tables as being soley responsible for the delta?
In any case, let's make sure that these are removed well before RTM.

Comment 54

[Pierre Saslawsky]

C'mon Peter... I did not say that the traceback table were entirely responsible 
of the bloat. I just said that between PR2 and PR3, the code size went up 4.8Mb 
and you cannot say that it all comes from the checkins in that 6 weeks period. 
Which you are not saying either.

Comment 55

[Peter Trudelle]

I'm not trying to put words in your mouth either, but you did say the table was
"Result of not turning off this option:", which sure seemed to imply that it was
the cause.

Comment 56

[Simon Fraser [no longer active]]

This is a grand example of a change where we've wasted more time debating it than 
would be spent fixing it. And dang, I just wasted some of my own time...

Please let's not repeat this fiasco at RTM.

Comment 57

[Pierre Saslawsky]

Reassigned to granrose who is taking over the mac build while jj is away.
See jj's comments from [2000-08-25 14:58].

Comment 58

[Peter Trudelle]

If this is an example of anything, it is of trying to make a change too late in
a release cycle, with too little information about risk vs benefit.  Despite
Pierre's all-caps comment, releases are unlikely to be delayed or even respun
for this bug.  Please consider how soon this can land for rtm; the later it
lands, the harder it will be to justify, even for RTM.

Comment 59

[Simon Fraser [no longer active]]

There is a very good reason why this should land as a "last-minute" thing before 
shipping, and that is that it makes it much harder to interpret MacsBug logs from 
optimized builds.

Comment 60

[Peter Trudelle]

There is also a very good reason why we we are extremely cautious about allowing
last minute changes to bits that are ready to ship: each change comes with a
risk, which must be weighed against the benefit of landing.  If this bug waits
until the last minute before RTM, it should have a clearer risk/benefit analysis
than it currently has.  Landing early would give more time to ensure the bits
are still good, or fix problems if they are not.  Doesn't Talkback generate
stack traces and other log info for optimized release bits?  Wouldn't most bugs
be worked on in debug builds anyway?  BTW, I have seen release delays due to
problems exposed by removing this type of data.  We can't afford to have that
happen at the last minute.

Comment 61

[Jon Granrose]

accepting bug.

I'll check this in as soon as the branch opens.

Comment 62

[Simon Fraser [no longer active]]

granrose: be sure to change the correct line; we only want them off for non-debug 
builds, so touch only the line with the comment 

/* leave on until the final release, so MacsBug logs are interpretable */

changing the "on" to "off".

Comment 63

[Jon Granrose]

attaching diffs for mozilla and ns trees.

sfraser can I get an r=?
leaf can I get an a=?

Comment 64

[Jon Granrose]

Attachment: patch to turn off for mozilla

Comment 65

[Jon Granrose]

Attachment: patch to turn off for ns

Comment 66

[Daniel (Leaf) Nunes]

i've lost my a= powers, the r= is what you need, along with the ++

Comment 67

[Scott Collins]

you can get an a= from me, if you want it.  a=scc.

Comment 68

[Simon Fraser [no longer active]]

r=sfraser on the patch.

Comment 69

[Pierre Saslawsky]

Peter: JJ usually turned the option off a couple of days after the branch was 
created, which means one week or so before the bits were shipped. It was (imo) an 
acceptable compromise between keeping the symbols as long as possible and making 
sure that the bits were stable enough. This was for beta releases. For the final 
version, the PDT team may prefer a longer testing period with the option off. 
Your call.

Jon: if you want, you can get in touch with JJ; I think he's coming back from 
sabbatical today.

Comment 70

[Jon Granrose]

fix checked in.

how should I mark this so that it shows up on "fixed bug" queries but not "to be 
fixed" queries?  Should I just go ahead and resolved/fixed it and we can file 
similar bugs for future releases?

Comment 71

[Pierre Saslawsky]

Fixed on the RTM branch ==> Cleared Keywords/Status and moved milestone to M19 
(or should it be mozilla0.6?). Reassigned to JJ.

Comment 72

[J.J. Enser]

wow, that was kinda entertaining... accepting bug :-)

fyi, I have always turn the traceback option off as soon as I can after a new 
branch is created off of the tip to give enough time for QA to detect any problem 
with the changed configuration, and I don't recall this to be ever causing 
problems with previous milestones. Therefore, I consider the risk factor to be 
'near zero' (I know, Peter, near zero is NOT zero)

Comment 73

[Peter Trudelle]

Everyone seems to consider *their changes* near-zero risk. It must be a wierd 
statistical anomaly that there have been 60,000 bugs in this product.

Comment 74

[Jon Granrose]

cleaning up a bit.  jj - this still needs to go into 0.9.1, right?

Comment 75

[J.J. Enser]

Yep. Not sure if this happened for 0.9, but I'll make sure that 'traceback' flags 
are turned off on the 0.9.1 branch once it gets cut.

Comment 76

[J.J. Enser]

build/mac/IDE_Options.h updated on both 0.9.1 branches (mozilla and netscape)
updating summary & tfv to 0.9.2 as a reminder

Comment 77

[J.J. Enser]

build/mac/IDE_Options.h updated on both 0.9.2 branches (mozilla and netscape)
updating summary & tfv to 0.9.3 as a reminder

Comment 78

[Daniel (Leaf) Nunes]

build/mac/IDE_Options.h updated on both 0.9.3 branches (mozilla and netscape)
updating summary & tfv to 0.9.4 as a reminder
(and it looks like we have to respin mac anyway, so this won't be the only 
issue holding 0.9.3)

Comment 79

[Jaime Rodriguez, Jr.]

Putting nsbranch+ into keywords and removing it from Status Whiteboard.

Comment 80

[grega]

JJ, Is this on your radar for 0.9.4?  Thanks!

Comment 81

[Jon Granrose]

jj's out til Thursday.  This is a simple change we do for every milestone, so if
we reach the point where 0.9.4 is ready and this is one of the few remaining
bugs, anyone on the build team can land it.

Comment 82

[J.J. Enser]

fixed on MOZILLA_0_9_4_BRANCH.
moving tfv to m0.9.5

Comment 83

[Jon Granrose]

removing nsbranch+ to drop off 0.9.4 radar.

Comment 84

[Daniel (Leaf) Nunes]

fixed for 0.9.5, updating target milestone.

Comment 85

[Jonadab the Unsightly One (Nathan Eady)]

Looks like this won't make it in for 0.9.6.

Comment 86

[chris hofmann]

regular rollover

Comment 87

[J.J. Enser]

updated summary and removed "blocker"

Comment 88

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.7

Comment 89

[J.J. Enser]

Attachment: new patch to impact only classic (non-carbon) builds

Comment 90

[J.J. Enser]

patch checked in on the 0.9.7 branch (attachment 62049 [details] [diff] [review])
moving tfv to next milestone.

Comment 91

[J.J. Enser]

same patch (attachment 62049 [details] [diff] [review]) checked in on the 0.9.8 branch
moving tfv to 0.9.9.

Comment 92

[Simon Fraser [no longer active]]

The patch in attachment 62049 [details] [diff] [review] appears not to work correctly. We're seeing stack 
traces from the 0.9.8 build that lack symbols, e.g. the one at 

  <http://home.attbi.com/~brian-clark2/mail-freeze-2.txt>

My guess is that TARGET_CARBON has not been #defined yet for this file.

Comment 93

[Simon Fraser [no longer active]]

Attachment: Patch to NGLayoutConfigInclude.h

Patch to fix this.

Comment 94

[Simon Fraser [no longer active]]

I checked the patch in attachment 67957 [details] [diff] [review] into the 0.9.8 branch.

Comment 95

[Jon Granrose]

nsbeta1+ just so it shows up on the radar.

Comment 96

[J.J. Enser]

patch from attachment 62049 [details] [diff] [review] checked in on the 0.9.9 branch
moving tfv to 1.0

Comment 97

[lchiang]

(Question:  Do we need to do this for any embedding customers who embed just
Gecko for the Mac?  If so, I will add topembed to this bug report.)

Comment 98

[J.J. Enser]

Lisa: I would assume that we also need to turn this off on embedding releases,
to favor performance rather than debugging. Simon?

Comment 99

[Simon Fraser [no longer active]]

I think embedders will need to tell us what they want. During their development
and beta release phases, they might well want symbols left on.

Comment 100

[lchiang]

Ok, I will follow up separately with our main embedding customer and post here
if we need to do this.

Comment 101

[lchiang]

embeddors would like this done.  Do I file a new bug or can we reuse this bug
report with a topembed keyword?

Comment 102

[J.J. Enser]

Lisa: Using this ongoing bug# as a reminder for other branches is ok with me,
but if you want to log a separate one for embedding, go ahead. whatever is
easier for you.

Comment 103

[lchiang]

topembed keyword added to track within this bug for now.  Assuming mac embeddors
will be using gecko based off mozilla1.0 milestone.

Comment 104

[Dave Barrowman]

Topembed plussing - this needs to be turned off for embedding releases as well.

Comment 105

[chris hofmann]

emmbedding customers do there own builds so I think this can be minused.
It should just be on the list as a reminder that we tell them
that it's advisable to make this change at the end of product
development cycles.  if this is not right put the + back on topembed

Comment 106

[saari (gone)]

Not all embedding customers do their own builds...

Comment 107

[Jon Granrose]

ADT1 - required for all releases.

Comment 108

[Jaime Rodriguez, Jr.]

adt1.0.0+ per ADT. Pls check it in when appropriate.

Comment 109

[J.J. Enser]

fixed on 1.0.0 branches. moving tfv to 1.0.1 to get it off the radar until next
milestone

Comment 110

[Jon Granrose]

verified for 1.0.

removing 1.0 keywords to make ADT's job easier.  We should probably add this to
the release checklist and start filing a bug for each milestone to make it
easier to track (and so we can spread the load to the team so jj doesn't have to
worry about this every release)

Comment 111

[Asa Dotzler [:asa]]

adding branch verification keyword.  This bug has been verified in a comment,
just moving that to the keyword field.

Comment 112

[J.J. Enser]

1.0.1 automatically inherited this fix from 1.0 since it was released from the
same branch.
checking in on the 1.1. branch now (a=leaf)
pushing milestone to 1.2 as a reminder

Comment 113

[Greg K.]

Is this still relevant in the FizzillaMach era?

Comment 114

[Simon Fraser [no longer active]]

Nope.

Comment 115

[Greg K.]

Resolving WONTFIX per comment 114.

Comment 116

[Tracy Walker [:tracy]]

verified per last comments