200389 - better constness in mailnews/imap

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Andrew Taylor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030401
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030401

Some constants in mailnews/imap can be made const, ala alecf's
netscape.public.mozilla.seamonkey posting.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Andrew Taylor]

Attachment: A patch that constifies some constants

The patch just constifies consts and depointerizes string contants.  I made the
string constants in EnvelopeTable arrays of chars rather than pointers; this
might not be the right thing to do.  The other changes are independant if that
change is rejected.

Comment 2

[Andrew Taylor]

Attachment: More contificationness, this time for addrbook

Another patch that does the same for mailnews/addrbook.

Comment 3

[David :Bienvenu]

I don't like the envelope change (that will cause more static data to get
allocated than needed, won't it?) but the others look OK.

Comment 4

[Andrew Taylor]

I collected some information about the above mentioned envelope change. 
Incidentally, I changed the array size from 20 to 12, the minimum needed.  These
numbers are from Linux.

The envelope change adds 32 bytes of .rodata, in addition to moving 64 bytes
from .data to .rodata (as measured with objdump -h).  So, yes, it does add
footprint.  However, it is worth noting that the number of shared library
relocation records is decreased by 10 (as measured with objdump -R).  The
relocation records take up disk footprint, but not memory footprint;  they also
effect startup time since the runtime loader must patch the in-memory image from
each record.

While this particular change is mostly irrelevent, I think the relocation
records are an important factor to consider when addressing footprint (at least
on ELF/Unix type OSes).

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 119224 [details] [diff] [review]
A patch that constifies some constants

>-	const char *name;
>+	const char name[20];

Why?

Comment 6

[Andrew Taylor]

> Why?

Using an array of characters rather than a pointer eliminates the need for a
relocation record for each name.  Anything that gets relocated is not
necessarily (never?) shared between multiple running instances.  Is making it a
fixed width field too much of a hack?

Comment 7

[Alec Flett]

sorry I haven't gotten to this, I've been focusing more on embedding libraries,
so IMAP fell below the radar. I'll take a look at this ASAP.

Comment 8

[Alec Flett]

Comment on attachment 119229 [details] [diff] [review]
More contificationness, this time for addrbook

sr=alecf, getting that easy stuff out of the way :)

Comment 9

[Alec Flett]

Comment on attachment 119224 [details] [diff] [review]
A patch that constifies some constants

ah, I see what you were trying for, but actually const char name[20] doesn't
really buy you much. there's no extra allocation that goes on here, and you're
actually preventing the linker from re-using these strings. Besides, while
you're saving 4 bytes per string for the pointer to the string, you're wasting
it again in the envelope table because most strings are less than 16 bytes.

I think you can just leave it as const char*, but if you're really desperate,
you should make it 
const char* const name;

and that should work.

Comment 10

[Alec Flett]

andrew over to you, since you're fixing this!

Comment 11

[Alec Flett]

oh heh, relocation. In any case, dont' optimize for relocation - if we get
relocated, we're screwed anyway. re-basing the dlls is something that the build
system should do automatically... (though it might not right now, I have no idea)

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 119229 [details] [diff] [review]
More contificationness, this time for addrbook

r=jst

Comment 13

[Andrew Taylor]

Attachment: A patch that constifies some constants, v2

Here is a new version of the imap patch that gets rid of the offending char
pointer to char array change.

Comment 14

[Alec Flett]

Comment on attachment 119968 [details] [diff] [review]
A patch that constifies some constants, v2

yeah, this works
sr=alecf

thanks for the work! It will be good to see this done on other modules too :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119229 [details] [diff] [review]
More contificationness, this time for addrbook

Checked in the addressbook patch. Leaving open, since the other patch still
needs review.

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 119968 [details] [diff] [review]
A patch that constifies some constants, v2

r=sspitzer, assuming you did some basic smoketest of imap.

Comment 17

[Andrew Taylor]

Comment on attachment 119968 [details] [diff] [review]
A patch that constifies some constants, v2

This patch has been smoke tested; it is ready to check in.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Patch checked in.

Comment 19

[Andrew Taylor]

This is checked in, marking bug fixed.

Comment 20

[Boris Zbarsky [:bzbarsky]]

This change:

-	const char *name;
+	const char * const name;

in nsImapServerResponseParser.cpp got reverted because it broke the win32 build.
 The structs in the array are const, so that should be sufficient....