1334834 - Replace nsIAtom with nsACString in nsIFolderListener.idl and nsIMsgFolder.idl

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (obsolete, but shows the idea).

+++ 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.

Comment 1

[aleth [:aleth]]

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...

Comment 2

[aleth [:aleth]]

Here is an example for how the xpcshell failures may help here: 

I looked at mailnews/local/test/unit/test_folderLoaded.js.

It's easy to comment out individual test parts and narrow down the leak to this line: http://searchfox.org/comm-central/source/mailnews/local/test/unit/test_folderLoaded.js#71

That line is allowed behaviour in the idl: http://searchfox.org/comm-central/source/mailnews/base/public/nsIMsgFolder.idl#498

But clearly it leads directly to a folder leak.

Comment 3

[Jorg K (CEST = GMT+2)]

I appreciate the comment.

(In reply to aleth [:aleth] from comment #1)
> fixed anyway). The mozmill bustage due to "leaking the world" won't be
> helped by an atom->string change in mailnews, ...
Let's be a little more precise. We're not planning to rip out atoms from mailnews/ in general.

The knee-jerk reaction Kent and I had yesterday was to remove atoms from nsIFolderListener.idl and nsIMsgFolder.idl and fix anything where this cascades down to.

I agree with Aleth: We shouldn't sweep existing problems under the carpet by replacing atoms with strings. By shutting up the warning voices we just keep leaking worse things like nsMsgDBFolder objects, of which we seem to be leaking n-1 if we have n folders.

Comment 4

[Jorg K (CEST = GMT+2)]

So we're talking about
  gTargetFolder.msgDatabase = null;
and
  * The attribute can also be set to another database or to null to force the
  * folder to reopen the same database when it is needed again.

Surely the set-function should do the right thing in this case:
http://searchfox.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#938
(I'm not so keen on 'searchfox' since it doesn't offer permalinks on C-C).

BTW, looking for SetMsgDatabase
http://searchfox.org/comm-central/search?q=SetMsgDatabase&case=false&regexp=false&path=
one can see that this is called all the time with nullprt.

However, how does nsMsgDBFolder::SetMsgDatabase() hang together with not destructing the nsMsgDBFolder object (since the instance count mInstanceCount never returns to zero)?

Comment 5

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #4)
> Surely the set-function should do the right thing in this case:
> http://searchfox.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.
> cpp#938

My *guess* is that as nsMsgDatabase contains a folder attribute, it retains a reference to the folder, and while the folder may forget about the database, the database retains the reference to the folder and that's the problem.

Comment 6

[aleth [:aleth]]

> My pet hate is this message:
> WARNING: some msg dbs left open: '!m_dbCache.Length()', file ... /mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 85

It seems likely that whatever folder is referenced by this db is the one causing the atom leak. And in the case of the test above, the SetMsgDatabase(null) call for some reason ends up preventing the db from being closed.

Comment 7

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #6)
> > My pet hate is this message:
> > WARNING: some msg dbs left open: '!m_dbCache.Length()', file ... /mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 85
> 
> It seems likely that whatever folder is referenced by this db is the one
> causing the atom leak. And in the case of the test above, the
> SetMsgDatabase(null) call for some reason ends up preventing the db from
> being closed.

Actually, that can't be quite right, as with the one line commented out, the test passes, but with four databases left open at shutdown instead of one.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Kent James (:rkent)]

"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.

Comment 10

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 11

[:aceman]

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 12

[:aceman]

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 .

Comment 13

[Jorg K (CEST = GMT+2)]

(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?

Comment 14

[:aceman]

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.

Comment 15

[Jorg K (CEST = GMT+2)]

No, not after bug 1393692 lands today.

Comment 16

[:aceman]

Ah, OK.

Comment 17

[Jorg K (CEST = GMT+2)]

(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 ;-)

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v3) - C++ part

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.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v3+) - C++ part

Oops, didn't add the new file.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms-js.patch (v1) - JS part

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 23

[:aceman]

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?

Comment 24

[:aceman]

(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 25

[:aceman]

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.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms-js.patch (v2) - JS part

Thanks for the hint, found some more.

Comment 27

[Jorg K (CEST = GMT+2)]

Gotta love "ImapHdrDownloaded" :-(

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v4) - C++ part

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.

Comment 29

[Jorg K (CEST = GMT+2)]

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

Comment 30

[Jorg K (CEST = GMT+2)]

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

Comment 31

[:aceman]

(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 32

[:aceman]

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 ?

Comment 33

[Jorg K (CEST = GMT+2)]

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.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms-js.patch (v3) - JS part

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

Comment 35

[Jorg K (CEST = GMT+2)]

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

Comment 36

[:aceman]

(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 37

[:aceman]

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() ?

Comment 38

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v5) - C++ part

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

Comment 40

[Jorg K (CEST = GMT+2)]

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.

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms-js.patch (v4) - JS part

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

Comment 42

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=912422c9c23acf790ad697d3b0c2fdf233827a60

I'll be afk for a few hours now.

Comment 43

[:aceman]

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.

Comment 44

[Jorg K (CEST = GMT+2)]

(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.

Comment 45

[:aceman]

Attachment: 1334558-remove-atoms.patch (v6) - C++ part

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 46

[:aceman]

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.

Comment 47

[Jorg K (CEST = GMT+2)]

(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.

Comment 48

[Jorg K (CEST = GMT+2)]

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 49

[Jorg K (CEST = GMT+2)]

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.

Comment 50

[:aceman]

*(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")'

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v6b) - C++ part

Reverted experiment, fixed comment.

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v6c) - C++ part

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

Comment 53

[Jorg K (CEST = GMT+2)]

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 54

[Jorg K (CEST = GMT+2)]

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=

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v7)

Another option that saves a few bytes ;-)

Comment 56

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v7b)

Maximising the saving.

Comment 57

[:aceman]

Attachment: 1334558-remove-atoms.patch (v8) - C++ part

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

Comment 58

[Nathan Froyd [:froydnj]]

(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...

Comment 59

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v9) - C++ part

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.

Comment 60

[Jorg K (CEST = GMT+2)]

Attachment: 1334558-remove-atoms.patch (v9b) - C++ part

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

Comment 61

[:aceman]

(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 62

[:aceman]

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.

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: 1334834-remove-atoms.patch (v10) - C++ part

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 64

[:aceman]

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.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
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

Comment 66

[Jorg K (CEST = GMT+2)]

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.

Comment 67

[:aceman]

(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.