+++ This bug was initially created as a clone of Bug #1334558 +++ Consider switching atoms to strings in nsIFolderListener.idl and nsIMsgFolder.idl. WIP patch included. The patch fixes the C++ part (needs to be refreshed after bug 1334558 has landed) and is missing the following: - Hard-coded strings should be made central defines - Kent suggested to clean up the names while we're at it (bug 1334558 comment #7 and 1334558 comment #10) Note there is "canFileMessages" and "CanFileMessages", hmm. The JS part is missing. We can identify call sites that need fixing here: https://dxr.mozilla.org/comm-central/search?q=mozilla.org%2Fatom-service&redirect=false In particular, Gloda's index_msg.js is one of those sites. Most of the others appear to be in tests.

Here is the current situation: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=10da34cc963b0daf9d9ded2dc1e099193ef3eb5f Given the relatively small (12) number of debug xpcshell failures due to the mailnews atoms, I'm not sure the time spent finishing this wouldn't be better spent fixing the nsMsgDBFolder leaks (which eventually should be fixed anyway). The mozmill bustage due to "leaking the world" won't be helped by an atom->string change in mailnews, and such a change may introduce new side-effects which may not be caught due to that remaining bustage. On the other hand, if the WIP patch is almost done and turns out to make a real difference, sure...

This bug is meant to be for the discussion about whether we want to switch from atoms to strings for nsIFolderListener.idl and nsIMsgFolder.idl. I've taken the liberty to hide comment #2 to comment #7 since they started being about test failures due to leaked atoms and nsMsgDBFolder objects not being cleaned up (and therefore dynamic atoms not released). Can we please move this discussion to bug 1334874 so we can clean up the mess before deciding whether to switch from atoms to strings. Right now, that transition doesn't buy us anything since a) it hides the leaks and b) it doesn't solve the other far worse atom leaks. Aleth, could you kindly summarise your findings so far in bug 1334874.

"Given the relatively small (12) number of debug xpcshell failures due to the mailnews atoms, I'm not sure the time spent finishing this wouldn't be better spent fixing the nsMsgDBFolder leaks (which eventually should be fixed anyway)." I'm not convinced that we will ever reliably fix the database leaks (which as I said to Jorg is what I believe you are really seeing) until we get rid of Mork, and change the design so that databases can be closed independently of destroying the C++ database object. That being said, it should be possible to trace out the cause of the database leaks in most of the tests, as something seems to go wrong when the folder database is reset. I'm not convinced this is our most urgent priority though, we are really chasing fixing tests due to some priority set by m-c. Either the strings or branching m-c to avoid the assertion would solve the immediate pain. Longterm, a Mork and database refactoring is an important priority.

This is really a tough call. One one hand, I spent some hours on this patch (which now needs refreshing) to do the atom->string transition. On the other hand, I use TB myself ;-) and it would be good to know that my data is safely stored and not corrupted. It is right that by crashing on leaked atoms M-C have set the priorities for us. But on the other hand, maybe, we should see the positive side here: A long-standing problem of unknown consequences is finally addressed. Surely progress in bug 1334874 will be slow. So far, after quite a complex analysis, Aleth found one programming error which fixed one of the 14 failing tests. I believe we need to continue along this path. If in the end we confirm that we can't reliably fix database leaks, then we can always sweep the remaining ones under the carpet of strings ;-)

It seems this is getting on topic again, it is mentioned in bug 1392883. Shall we refresh the patch? Do you want to finish the C++ part and I review and I look at the JS callers?

Comment on attachment 8831471 [details] [diff] [review] 1334558-remove-atoms.patch (obsolete, but shows the idea). Review of attachment 8831471 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/src/nsMsgAccountManager.cpp @@ +822,5 @@ > rv = aOldAccount->GetIncomingServer(getter_AddRefs(server)); > if (NS_SUCCEEDED(rv) && server) { > rv = server->GetRootFolder(getter_AddRefs(rootFolder)); > if (NS_SUCCEEDED(rv) && rootFolder) > + rootFolder->NotifyBoolPropertyChanged("DefaultServer", Yeah, I would also like some constants for this, but not put them back in nsMsgDataSource.cpp as I think that file isn't used in TB (I compile it out in bug 464710). I'd say it could be in nsMsgDBFolder.h or similar. They could be defines so we do not need to somehow centralize their allocation and destruction (as is currently done with the atoms, but then I don't know what nsMsgDBFolderAtomList.h is for). Unless you can do static const strings (that will exist once in the binary). ::: mailnews/base/src/nsMsgFolderDataSource.cpp @@ +776,5 @@ > int64_t oldValue, > int64_t newValue) > { > nsCOMPtr<nsIRDFResource> resource(do_QueryInterface(folder)); > + if (strcmp("TotalMessages", property) == 0) Do you prefer (strcmp() == 0) to !strcmp() ? ::: mailnews/local/src/nsPop3IncomingServer.cpp @@ -227,5 @@ > // if isDeferred state has changed, send notification > if (aAccountKey.IsEmpty() != deferredToAccount.IsEmpty()) > { > - nsCOMPtr <nsIAtom> deferAtom = MsgGetAtom("isDeferred"); > - nsCOMPtr <nsIAtom> canFileAtom = MsgGetAtom("CanFileMessages"); Yes, it is interesting this is capitalized CanFileMessages. We didn't create such atom in nsMsgFolderDataSource .

(In reply to :aceman from comment #11) > It seems this is getting on topic again, it is mentioned in bug 1392883. > Shall we refresh the patch? > Do you want to finish the C++ part and I review and I look at the JS callers? Hmm, I'm not sure what M-C's long time plans are. Bug 1393692 comment #8 says: "something like that patch will likely be necessary". Since bug 1393692 is removing atom use from JS, I think there will be nothing left to do here for JS, or am I missing something?

We still have nsIAtomService usage in c-c JS so it seems we have JS usage of nsIAtoms that will need to be looked at.

No, not after bug 1393692 lands today.

Ah, OK.

(In reply to Jorg K (GMT+2) from comment #15) > No, not after bug 1393692 lands today. OK, the use of nsIAtomService in JS is gone now, but *not* the use of nsIAtoms. You can look at .toString() here: https://hg.mozilla.org/comm-central/rev/ac14ca6da238 - 6 times So when we pass strings instead of atoms, we remove the .toString(), although I guess that a .toString() on a string is a no-op, right? So your JS part is extra simple ;-)

I've refreshed the patch, but I'll do something else inspired by bug 1393692. Let's use Mozilla strings. New patch coming tomorrow.

This compiles and seems to run. I concentrated the strings in nsMsgDBFolder.h and added a few more. There are still individual events in other places: NS_LITERAL_CSTRING("DefaultServer") - 2x NS_LITERAL_CSTRING("msgLoaded") NS_LITERAL_CSTRING("AboutToCompact") NS_LITERAL_CSTRING("MRMTimeChanged") NS_LITERAL_CSTRING("isSecure") NS_LITERAL_CSTRING("FolderCreateCompleted") NS_LITERAL_CSTRING("FolderCreateFailed") - x2 NS_LITERAL_CSTRING("CompactCompleted") NS_LITERAL_CSTRING("isDeferred") NS_LITERAL_CSTRING("CanFileMessages") - different to canFileMessages NS_LITERAL_CSTRING("SortOrder") There are also some more uses which don't include nsMsgDBFolder.h: if (aProperty.Equals("DefaultServer")) { if (aProperty.Equals("BiffState") && mFoldersWithNewMail) if (aProperty.Equals("TotalUnreadMessages")) { if (aProperty.Equals("FolderFlag")) if (aProperty.Equals("DefaultServer")) { if (property.Equals("SortOrder")) if (property.Equals("isSecure")) if (property.Equals("canFileMessages")) if (property.Equals("BiffState") && mCurrentBiffState != newValue) { So far it's a good clean-up but we could clean up a little harder by making all these strings named literals and moving them into their own include file.

This should be perfect now. All the strings collected in one file, all consistently named. I didn't include "msgLoaded" since this is only used once.

Oops, didn't add the new file.

Just removing the .toString() that got added here: https://hg.mozilla.org/comm-central/rev/ac14ca6da238 Let's see whether it works, sadly Mac and Linux are busted and Xpcshell and Mozmill have one failure each :-( (bug 1393704 and yet unfiled but coming up). https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=18c0c1fb403abd7d1ca542d0d533ae000e62e55f

Comment on attachment 8901422 [details] [diff] [review] 1334558-remove-atoms-js.patch (v1) - JS part Review of attachment 8901422 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Will we also convert the 3 uses of OnItemEvent in Seamonkey similarly?

(In reply to Jorg K (GMT+2) from comment #19) > Created attachment 8901410 [details] [diff] [review] > 1334558-remove-atoms.patch (v2) > > This compiles and seems to run. I concentrated the strings in > nsMsgDBFolder.h and added a few more. There are still individual events in > other places: > > NS_LITERAL_CSTRING("DefaultServer") - 2x > NS_LITERAL_CSTRING("msgLoaded") > NS_LITERAL_CSTRING("AboutToCompact") > NS_LITERAL_CSTRING("MRMTimeChanged") > NS_LITERAL_CSTRING("isSecure") > NS_LITERAL_CSTRING("FolderCreateCompleted") > NS_LITERAL_CSTRING("FolderCreateFailed") - x2 > NS_LITERAL_CSTRING("CompactCompleted") > NS_LITERAL_CSTRING("isDeferred") > NS_LITERAL_CSTRING("CanFileMessages") - different to canFileMessages > NS_LITERAL_CSTRING("SortOrder") > > There are also some more uses which don't include nsMsgDBFolder.h: > if (aProperty.Equals("DefaultServer")) { > if (aProperty.Equals("BiffState") && mFoldersWithNewMail) > if (aProperty.Equals("TotalUnreadMessages")) { > if (aProperty.Equals("FolderFlag")) > if (aProperty.Equals("DefaultServer")) { > if (property.Equals("SortOrder")) > if (property.Equals("isSecure")) > if (property.Equals("canFileMessages")) > if (property.Equals("BiffState") && mCurrentBiffState != newValue) { > > So far it's a good clean-up but we could clean up a little harder by making > all these strings named literals and moving them into their own include file. It looks to me some of them are folder properties and some of them are account (or account manager) properties. It could be worth to keep the distinction and put them in separate .h files. That may also be why there are two CanFileMessages variants. I looked at addon usage and some of them have .rdf files that define CanFileMessagesOnServer and CanFileMessages properties.

Comment on attachment 8901421 [details] [diff] [review] 1334558-remove-atoms.patch (v3+) - C++ part Review of attachment 8901421 [details] [diff] [review]: ----------------------------------------------------------------- I have yet to build this but I have some comments. ::: mailnews/base/src/nsMsgAccountManagerDS.cpp @@ +1142,4 @@ > bool aOldValue, > bool aNewValue) > { > + if (aProperty.Equals("DefaultServer")) { Account manager constant. ::: mailnews/base/src/nsMsgFolderDataSource.h @@ +264,1 @@ > You could kill this space while here ;) ::: mailnews/base/src/nsMsgStatusFeedback.cpp @@ +130,5 @@ > { > // not sending this notification is not a fatal error... > (void) msgUrl->GetMessageHeader(getter_AddRefs(msgHdr)); > if (msgFolder && msgHdr) > + msgFolder->NotifyPropertyFlagChanged(msgHdr, NS_LITERAL_CSTRING("msgLoaded"), 0, 1); I think we should include the the stringlist and use the constant here too. ::: mailnews/base/util/nsMsgDBFolderStringList.h @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ I would call this file nsMsgDBFolderPropertyList.h ::: mailnews/local/src/nsLocalUndoTxn.cpp @@ +549,3 @@ > { > if (mTxn && mFolder && aItem == mFolder) { > + if (aEvent.Equals("FolderLoaded")) Constant. This one does look like from the other group, not a folder property/constant. ::: mailnews/local/src/nsPop3IncomingServer.cpp @@ +232,2 @@ > !deferredToAccount.IsEmpty(), deferredToAccount.IsEmpty()); > + folderListenerManager->OnItemBoolPropertyChanged(rootFolder, kCanFileMessagesUpper, Account manager constants.

Thanks for the hint, found some more.

Gotta love "ImapHdrDownloaded" :-(

Thanks for looking at it. (In reply to :aceman from comment #25) > > + if (aProperty.Equals("DefaultServer")) { > Account manager constant. I don't agree, it notified for a folder, so it might as well go with the others. Let's not over-complicate things here. All those strings are either properties or events to do with folders, and we keep them in a central place now. Before the same atom was allocated twice in some cases. > You could kill this space while here ;) Done. > > + msgFolder->NotifyPropertyFlagChanged(msgHdr, NS_LITERAL_CSTRING("msgLoaded"), 0, 1); > I think we should include the the stringlist and use the constant here too. Not for one use, no. > ::: mailnews/base/util/nsMsgDBFolderStringList.h > I would call this file nsMsgDBFolderPropertyList.h Maybe you would, but that would be wrong since not all of them are properties, like: AboutToCompact. They are just strings like we had atoms before. > > + if (aEvent.Equals("FolderLoaded")) > Constant. This one does look like from the other group, not a folder > property/constant. Missed that one. > > + folderListenerManager->OnItemBoolPropertyChanged(rootFolder, kCanFileMessagesUpper, > Account manager constants. Same comment as above, only added complication trying to classify them. It's used next to kIsDeferred.

Very few changes from previous version, only white-space and the missed one.

Oops, forgot to paste: https://bugzilla.mozilla.org/attachment.cgi?oldid=8901421&action=interdiff&newid=8901437&headers=1

(In reply to Jorg K (GMT+2) from comment #28) > (In reply to :aceman from comment #25) > > > + if (aProperty.Equals("DefaultServer")) { > > Account manager constant. > I don't agree, it notified for a folder, so it might as well go with the > others. > Let's not over-complicate things here. All those strings are either > properties or events to do with folders, and we keep them in a central place > now. Before the same atom was allocated twice in some cases. Separating properties and events would explain why there are two similar ones only differing in case. > > > + msgFolder->NotifyPropertyFlagChanged(msgHdr, NS_LITERAL_CSTRING("msgLoaded"), 0, 1); > > I think we should include the the stringlist and use the constant here too. > Not for one use, no. Why? Using the constant would imply semantincs that it is a folder property, the same as in other files. Do we loose anything by including the StringList.h in every file where the constants are used? Does it instantiate the strings at every inclusion? > > ::: mailnews/base/util/nsMsgDBFolderStringList.h > > I would call this file nsMsgDBFolderPropertyList.h > Maybe you would, but that would be wrong since not all of them are > properties, like: AboutToCompact. > They are just strings like we had atoms before. Didn't I propose to split them? Then nsMsgDBFolderPropertyList.h would only contain properties so this problem would be void. And the other file would contain events. > > > + folderListenerManager->OnItemBoolPropertyChanged(rootFolder, kCanFileMessagesUpper, > > Account manager constants. > Same comment as above, only added complication trying to classify them. It's > used next to kIsDeferred. So you could at least group them inside nsMsgDBFolderPropertyList.h, prepend with a comment which group is folder 'properties' and which is 'events'.

Comment on attachment 8901435 [details] [diff] [review] 1334558-remove-atoms-js.patch (v2) - JS part Review of attachment 8901435 [details] [diff] [review]: ----------------------------------------------------------------- What about e.g. https://dxr.mozilla.org/comm-central/rev/8d0293462cd9ccfdddd16a0a447638c6c362b35c/mail/base/modules/dbViewWrapper.js#202 ?

I can see that this will be a slow way forward here discussing every detail. I should wait until nsIAtom disappears and then land it as a bustage-fix ;-) I can add "msgLoaded" if you want. I haven't looked at NS_NAMED_LITERAL_CSTRING(), but I think they are all instantiated on every include, which is also not the end of the world: http://searchfox.org/mozilla-central/source/xpcom/string/nsLiteralString.h#29 I'll see whether I can group them.

More. I won't put r? since it creates too much noise. We're both aware of what's going on.

BTW, kKeywords, "Keywords" was never used and kCanFileMessagesLower, "canFileMessages" is only tested while kCanFileMessagesUpper, "CanFileMessages" is emitted. So that looks wrong.

(In reply to Jorg K (GMT+2) from comment #33) > I can see that this will be a slow way forward here discussing every detail. > I should wait until nsIAtom disappears and then land it as a bustage-fix ;-) No, I think we are almost done, just label (with a comment) the two groups in the single file you created. And apparently nsIAtom will not go away in C++ so there may be no bustage :) > I can add "msgLoaded" if you want. I haven't looked at > NS_NAMED_LITERAL_CSTRING(), but I think they are all instantiated on every > include, which is also not the end of the world: The old atoms were only created once and then only referenced (if I understand correctly). That is why I proposed something that would only be created once so we do not regress in this aspect. E.g. defines. Or 'static nsCString' if it only creates one copy of the variable in whole binary.

Comment on attachment 8901439 [details] [diff] [review] 1334558-remove-atoms-js.patch (v3) - JS part Review of attachment 8901439 [details] [diff] [review]: ----------------------------------------------------------------- https://dxr.mozilla.org/comm-central/rev/8d0293462cd9ccfdddd16a0a447638c6c362b35c/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1730 ? https://dxr.mozilla.org/comm-central/rev/8d0293462cd9ccfdddd16a0a447638c6c362b35c/mailnews/base/content/folderWidgets.xml#250 Doesn't this need to check all occurrences of OnItemEvent and if their event argument is called with .toString() ?

Would you mind taking over the JS part as you initially suggested. I'm almost done sorting and shuffling the strings. Also still compiling. You're mistaken, nsIAtom, the XPCOM call *will* be removed, replaced with a pure C++ class, see bug 1393088 and bug 1380413/bug 1381011. So there would be bustage. Anyway, looks like we're coming to an agreement here ;-)

OK, kKeywords was used, anyway, I merged the "CanFileMessages".

Comment on attachment 8901441 [details] [diff] [review] 1334558-remove-atoms.patch (v5) - C++ part Review of attachment 8901441 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/util/nsMsgDBFolderStringList.h @@ +2,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +// We define strings for properties and evens. Typo. I'll fix it later.

I think I caught them all now. Anyway, doing a .toString() on a string won't hurt.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=912422c9c23acf790ad697d3b0c2fdf233827a60 I'll be afk for a few hours now.

Comment on attachment 8901441 [details] [diff] [review] 1334558-remove-atoms.patch (v5) - C++ part Review of attachment 8901441 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't compile as there are still leftover implementations of the notification listeners in nsMessengerUnixIntegration.cpp and nsMessengerOSXIntegration.cpp that need to be converted to nsACString. I'll attach a new patch.

(In reply to :aceman from comment #43) > I'll attach a new patch. Thanks, any ETA? I can do it too, unless you've already done and compiled it. Maybe if we all agree, we could land this in the next push.

This converts the mentioned files and also removes some unneeded inclusions of nsIAtom.h (then I had to fight a bug in m-c (a m-c file again not including one of its dependencies) so it took some time). There are still some uses of nsIAtom left for another bug. Do we have the solution for creating the strings only once? Declaring the variables static doesn't work (the kConstants appear many times in the binary). It seems they would need to be static members of a class (and initialized once per binary). I've used #define for one of the constants "NewMailReceived". It appears only once in the binary (with gcc on linux) and there is no 'kNewMailReceived'. Please check with your compiler. Do we know if the removal of atoms and using plain strings may be faster or slower? Using the define may incur a penalty of always creating a nsCString from the literal at leach comparison. Maybe we could then use .EqualsLiteral()

Comment on attachment 8901442 [details] [diff] [review] 1334558-remove-atoms-js.patch (v4) - JS part Review of attachment 8901442 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this seems to cover all places now.

(In reply to :aceman from comment #45) > This converts the mentioned files and also removes some unneeded inclusions > of nsIAtom.h (then I had to fight a bug in m-c (a m-c file again not > including one of its dependencies) so it took some time). Great clean-up job. > There are still some uses of nsIAtom left for another bug. Ah yeah? We'll check that in DXR/Searchfox after landing this. > Do we have the solution for creating the strings only once? Not really. But it's not an issue. > Declaring the variables static doesn't work (the kConstants appear many > times in the binary). It seems they would need to be static members of a > class (and initialized once per binary). Yes, that's how the atoms were done in nsMsgDBFolder.h, but then the other files/classes can't access them. > I've used #define for one of the constants "NewMailReceived". It appears > only once in the binary (with gcc on linux) and there is no > 'kNewMailReceived'. > Please check with your compiler. Sure that compiles. > Do we know if the removal of atoms and using plain strings may be faster or > slower? Faster since we save all the lookups, especially those using the nsIAtomservice in JS. > Using the define may incur a penalty of always creating a nsCString > from the literal at each comparison. Maybe we could then use > .EqualsLiteral() No. Because the notification goes via an nsCString. Soon as you go #define kNewMailReceived "NewMailReceived" you'll get a million compile errors. I've tried that at around 3:30 AM last night :-( I think my suggestion is best. You have a few duplicated string objects hanging around, and otherwise the compilers might duplicate up the fixed strings.

BTW, try results are coming in on https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=912422c9c23acf790ad697d3b0c2fdf233827a60 Mozmill looks good no further failure than the known one from bug 1394081. Xpcshell not ready. I've see that take 86 minutes which is double the estimated time. So can we land this (assumed working) or do you want another try run of the final version, with the define reverted?

Comment on attachment 8901482 [details] [diff] [review] 1334558-remove-atoms.patch (v6) - C++ part The Xpcshell test only shows on know failure, so this is ready to go. I noticed that the bug number is wrong in the patches.

*(In reply to Jorg K (GMT+2) from comment #47) > > There are still some uses of nsIAtom left for another bug. > Ah yeah? We'll check that in DXR/Searchfox after landing this. Yes, but I didn't touch them (those that weren't dead) because this bug is labeled for nsIMsgFolder*.idl only. > > I've used #define for one of the constants "NewMailReceived". It appears > > only once in the binary (with gcc on linux) and there is no > > 'kNewMailReceived'. > > Please check with your compiler. > Sure that compiles. I mean if it only creates the string once in the binary. > > Using the define may incur a penalty of always creating a nsCString > > from the literal at each comparison. Maybe we could then use > > .EqualsLiteral() > No. Because the notification goes via an nsCString. Soon as you go > #define kNewMailReceived "NewMailReceived" > you'll get a million compile errors. I've tried that at around 3:30 AM last > night :-( Why? It does compile for me, otherwise I would have uploaded the experiment. But I did '#define kNewMailReceived NS_LITERAL_CSTRING("NewMailReceived")'

Reverted experiment, fixed comment.

Grrr, now reverted the comment and fixed the bug number in the patch. Still wrong in the JS part.

OK, so let's document this again: NS_NAMED_LITERAL_CSTRING(kBiffState, "BiffState"); create that object once statically in every source .cpp file. Advantage: One static object, no creating of objects at runtime for call(NS_LITERAL_CSTRING("xxx")). Disadvantage: Wastes some bytes of program size. #define kNewMailReceived NS_LITERAL_CSTRING("NewMailReceived") See above: Created at runtime over and over. The compiler may also create multiple copies of "NewMailReceived" which would defeat any advantage of saving space. #define kNewMailReceived "NewMailReceived" Doesn't compile since the thing is passed as nsACString. All clear now?

Comment on attachment 8901486 [details] [diff] [review] 1334558-remove-atoms.patch (v6c) - C++ part I think this is good to go. Any attempt to save a few bytes will make things more complicated. Not even M-C care about saving a few bytes: http://searchfox.org/comm-central/search?q=NS_NAMED_LITERAL_CSTRING(objectDataKeyString%2C+%22object_data_key%22)%3B&case=false&regexp=false&path=

Another option that saves a few bytes ;-)

Maximising the saving.

This is my proposal. Uses static members of nsMsgDBFolder class which should be available or can be easily included in files that already use nsIMsgFolder.h. The strings and variables are stored only once in the resulting binary. Try run seems to pass: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c2d097ad800f7d53b927565f12f10f5859617e56

(In reply to Jorg K (GMT+2) from comment #54) > I think this is good to go. Any attempt to save a few bytes will make things > more complicated. > > Not even M-C care about saving a few bytes: > http://searchfox.org/comm-central/ > search?q=NS_NAMED_LITERAL_CSTRING(objectDataKeyString%2C+%22object_data_key%2 > 2)%3B&case=false&regexp=false&path= Note that a decent linker will merge at least the string data together, so you'll just have duplicate copies of the nsString object. Another option is |extern| nsLiteralString (or whatever the right string type is) declarations and declaring the strings once in a particular source file...

Thanks Aceman for working on "my" bug trying to make the solution more memory efficient. As mentioned in IRC, your approach is really a little complicated. How about something simpler inspired by Nathan's comment #58. Looks like we made so much noise in this bug that some M-C people came to join the spectacle ;-) - Thanks Nathan! Interdiff v9 and v6c.

Silly me got the extern business all wrong. Now it should be right.

(In reply to Nathan Froyd [:froydnj] from comment #58) > Note that a decent linker will merge at least the string data together, so > you'll just have duplicate copies of the nsString object. Yes, that is what I saw. One string only, but many occurrences of the k* variable names in the binary.

Comment on attachment 8901545 [details] [diff] [review] 1334558-remove-atoms.patch (v9b) - C++ part Review of attachment 8901545 [details] [diff] [review]: ----------------------------------------------------------------- My solution had the advantage that it used an already existing technique (see e.g. nsMsgDBFolder::kLocalizedInboxName) and the contants we scoped to the folder class, so it was clearly seen throughout the code (outside nsMsgDBFolder class) that they are folder constants, not some random app-global constants. So that would be an improvement over the atoms. But I can live with the 'extern' version, it seems to be semantically the same as the atoms, just slower. It fulfills my wish of one string and one variable name in the binary :) I would initialize the strings in nsMsgDBFolder.cpp instead of nsMsgDBFolderStringList.h.

Maybe this will make us happy then. I've lost the extra include file, put the declaration into the .h and the definition into the .cpp file. Basically your patch with two hunks changed and the nsMsgDBFolder:: prefix removed that you had painfully added where necessary. Maybe interdiff with v8 will get the best result here.

Comment on attachment 8901553 [details] [diff] [review] 1334834-remove-atoms.patch (v10) - C++ part Review of attachment 8901553 [details] [diff] [review]: ----------------------------------------------------------------- Then I don't see why you don't like having the constants inside the class when you now have the same inclusions as I had :) But I'm fine with this version too.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/93d7402586ec remove use of nsIAtom from mailnews/nsIFolder*.idl. r=aceman https://hg.mozilla.org/comm-central/rev/9df74592471a remove use of nsIAtom from mailnews/nsIFolder*.idl, JS part. r=aceman

Thanks for your patience. We spent an over-average amount of time on this bug, but we reached a solution that everyone could accept. (In reply to :aceman from comment #64) > Then I don't see why you don't like having the constants inside the class > when you now have the same inclusions as I had :) I basically used your patch since you had done such a nice clean-up. If considered using other inclusions, but those you had were fine and I didn't have to cut it too finely. Yes, I guess I disliked bloating the class with those strings. In the end, it's just a matter of taste. As for using atoms: While the atom comparison was faster than the string comparison, that benefit got defeated by constant lookups. What do the think this costs: JS: - let atomService = Cc["@mozilla.org/atom-service;1"] - .getService(Ci.nsIAtomService); - let kFolderLoadedAtom = atomService.getAtom("FolderLoaded"); or this, C++: - mSortOrder = order; - nsCOMPtr<nsIAtom> sortOrderAtom = MsgGetAtom("SortOrder"); - NotifyIntPropertyChanged(sortOrderAtom, oldOrder, order); Especially hilarious would be a situation where both emitter and consumer do a lookup and then compare the atom. I haven't checked whether such a case existed.

(In reply to Jorg K (GMT+2) from comment #66) > Thanks for your patience. We spent an over-average amount of time on this > bug, but we reached a solution that everyone could accept. As this wasn't a bustage fix, we should strive to improve the architecture/readability/semantics where possible. That's why I wanted to play with this more ;) > As for using atoms: While the atom comparison was faster than the string > comparison, that benefit got defeated by constant lookups. What do the think > this costs: > JS: > - let atomService = Cc["@mozilla.org/atom-service;1"] > - .getService(Ci.nsIAtomService); > - let kFolderLoadedAtom = atomService.getAtom("FolderLoaded"); > or this, C++: > - mSortOrder = order; > - nsCOMPtr<nsIAtom> sortOrderAtom = MsgGetAtom("SortOrder"); > - NotifyIntPropertyChanged(sortOrderAtom, oldOrder, order); > > Especially hilarious would be a situation where both emitter and consumer do > a lookup and then compare the atom. I haven't checked whether such a case > existed. Yes, that is probably why there is not much sense creating atoms in JS and then sending them into C++ for comparison. Only that the interface required it so far. So maybe that's why m-c is removing atoms from being seen from JS. Anyway, even if m-c keeps using atoms in C++, we do not need to if they are complicated to use. The folder properties and events we used atoms for are NOT being called 1000 times per second that any perf difference would matter (I could understand atoms may be needed in gecko e.g. for fast css lookups or similar where this rate of properties/events may be necessary). Maybe the "DeleteOrMoveMsgCompleted" event could matter if one deletes 1 million of messages (bug is filed), but in that case we have other bottlenecks there and this is not a common operation.