The default bug view has changed. See this FAQ.

Eliminate PLDHashtable in nsMsgDatabase

NEW
Assigned to

Status

MailNews Core
Database
4 years ago
a month ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Old, outdated, hard-to-read.
(Assignee)

Comment 1

4 years ago
Created attachment 728244 [details] [diff] [review]
Kill it dead

Hey look, I also killed a few static_cast<nsMsgHdr*>! :-)

This requires the memory reporter to apply properly. I also did some code cleanup regarding the ownership of nsIMdbRow by nsMsgHdr and eliminated a few return values to reflect operation infallibility.
Attachment #728244 - Flags: review?(kent)

Comment 2

4 years ago
"This requires the memory reporter to apply properly"

I'm not sure what you mean by that. As you say, it does not apply as given. What do I have to do to get it to apply?

I've occasionally whined about big bugs with little motivation listed, so I'll try to preclude my fellow whiners by giving a motivation for this, which I only understood after reading the patch. Please correct me if I am incorrect:

PLDHashTable is a low-level implementation that is not the preferred hash table for common uses where standard types are stored. This bug changes the PLDHashTables uses in nsMsgDatabase to type-specific variants, resulting in cleaner, easier to read code.
(Assignee)

Comment 3

4 years ago
(In reply to Kent James (:rkent) from comment #2)
> "This requires the memory reporter to apply properly"
> 
> I'm not sure what you mean by that. As you say, it does not apply as given.
> What do I have to do to get it to apply?

Bug 480843. There's no real dependency, just the fact that two patches in my queue touch the same code, and I only decided to change this after getting sick of manipulating PLDHashTables in that bug.

> PLDHashTable is a low-level implementation that is not the preferred hash
> table for common uses where standard types are stored. This bug changes the
> PLDHashTables uses in nsMsgDatabase to type-specific variants, resulting in
> cleaner, easier to read code.

I also take the time to fix up some of the ownership: nsMsgHdr now explicitly uses an nsCOMPtr for its nsIMdbRow--it always held a strong reference to it, now we use nsCOMPtr to explicitly manage it.

Comment 4

4 years ago
Comment on attachment 728244 [details] [diff] [review]
Kill it dead

Review of attachment 728244 [details] [diff] [review]:
-----------------------------------------------------------------

Overall the patch is fine, but has bit rotted a bit since my review was delayed. I'm going to r- just so we discuss my few questions, but I probably won't give the next version much scrutiny before r+

I am suggesting that we cleanup at least one of memory management oddities, namely doing an addref in "new nsMsgHdr"

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +462,5 @@
> +    hdr->GetMessageKey(&key);
> +  // TODO: Implement LRU replacement.
> +  if (m_cachedHeaders.Count() > m_cacheSize)
> +    ClearHdrCache();
> +  nsIMsgDBHdr *bar = hdr;

Why do you need this temporary instead of just using hdr directly?

@@ +563,5 @@
>  }
>  
> +void nsMsgDatabase::ClearHdrCache()
> +{
> +  if (m_cachedHeaders.IsInitialized())

Is there any reason why we can't just initialize m_cachedHeaders to the default value in the initializer (like you do for the inuse cache) and get rid of all of these .IsInitialized() checks?

@@ +597,5 @@
> +  m_headersInUse.Put(key, hdr);
> +
> +  // the hash table won't add ref, we'll do it ourselves
> +  // stand for the addref that CreateMsgHdr normally does.
> +  NS_ADDREF(hdr);

This is really odd, non-standard behavior. Why do this here instead of the more conventional way of doing the addref in CreateMsgHdr?

Also note that the comment in CreateMsgHdr now says:
// don't need to addref here; GetHdrFromUseCache addrefs.

which is wrong, if the odd behavior is kept, the addref is in AddHdrToUseCache.

@@ +627,5 @@
>    nsMsgHdr *msgHdr = new nsMsgHdr(this, hdrRow);
>    if(!msgHdr)
>      return NS_ERROR_OUT_OF_MEMORY;
>    msgHdr->SetMessageKey(key);
>    // don't need to addref here; GetHdrFromUseCache addrefs.

As I said earlier, this comment is incorrect, and I would prefer if the addref was done here, in the conventional place. I don't expect *msgHdr = new ... to return an addrefed value.

@@ +4029,3 @@
>    {
>      nsresult rv = InitRefHash();
>      if (NS_FAILED(rv))

Unlike the normal NS_ERROR_FAILURE from this method, a failure at this point is something that the devs should know about. Could we at least make this NS_ENSURE_SUCCESS(rv, rv) so that we see a warning if this is failing? Otherwise this is a completely silent failure.

@@ +4037,5 @@
> +}
> +
> +void nsMsgDatabase::AddRefToHash(nsCString &reference, nsMsgKey threadId)
> +{
> +  if (m_msgReferences.IsInitialized())

Is there any good reason why we are not initializing here if needed? Previously this was a complex pain, but now it is relatively easy to add. It was not clear to me from looking a couple of levels deep that we could guarantee initialization.

@@ +4052,5 @@
>  
>    for (int32_t i = 0; i < numReferences; i++)
>    {
>      nsAutoCString reference;
>      

Inserted line of just spaces? Please trim.

@@ +4105,2 @@
>  
>    // Populate table with references of existing messages

Is there some reason that you are removing the null check here?
In my own code, I would use NS_ENSURE_STATE(enumerator) here, as that is the shortest available version. That has not caught on in mailnews, but I am happy for you to start a trend :)

@@ +4487,5 @@
>    return NS_OK; // it's not an error not to find a msg hdr.
>  }
>  
>  nsIMsgDBHdr *nsMsgDatabase::GetMsgHdrForSubject(nsCString &subject)
>  {

This method AFAICT has no users. Perhaps it makes more sense to just remove it instead of fixing it.
Attachment #728244 - Flags: review?(kent) → review-
(Assignee)

Comment 5

4 years ago
(In reply to Kent James (:rkent) from comment #4)
> I am suggesting that we cleanup at least one of memory management oddities,
> namely doing an addref in "new nsMsgHdr"

I originally avoided doing that to minimize regressions and avoid making the patch even larger, but I'm not against doing that in this patch.
> 
> ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
> @@ +462,5 @@
> > +    hdr->GetMessageKey(&key);
> > +  // TODO: Implement LRU replacement.
> > +  if (m_cachedHeaders.Count() > m_cacheSize)
> > +    ClearHdrCache();
> > +  nsIMsgDBHdr *bar = hdr;
> 
> Why do you need this temporary instead of just using hdr directly?

I honestly have no idea what I was doing here.

> @@ +563,5 @@
> >  }
> >  
> > +void nsMsgDatabase::ClearHdrCache()
> > +{
> > +  if (m_cachedHeaders.IsInitialized())
> 
> Is there any reason why we can't just initialize m_cachedHeaders to the
> default value in the initializer (like you do for the inuse cache) and get
> rid of all of these .IsInitialized() checks?

The original reason, IIRC, was to honor the cache size if a non-custom value is set later. Looking in more detail in the code, this isn't an issue.

> @@ +4037,5 @@
> > +}
> > +
> > +void nsMsgDatabase::AddRefToHash(nsCString &reference, nsMsgKey threadId)
> > +{
> > +  if (m_msgReferences.IsInitialized())
> 
> Is there any good reason why we are not initializing here if needed?
> Previously this was a complex pain, but now it is relatively easy to add. It
> was not clear to me from looking a couple of levels deep that we could
> guarantee initialization.

InitRefHash is a fairly expensive method. Looking at the code, this may be a performance enhancement: we only fill the table if we need to grab a reference to it. If we're trying to add an entry and it doesn't exist, then it would be filled properly when we lazily create the table at use.

> @@ +4105,2 @@
> >  
> >    // Populate table with references of existing messages
> 
> Is there some reason that you are removing the null check here?
> In my own code, I would use NS_ENSURE_STATE(enumerator) here, as that is the
> shortest available version. That has not caught on in mailnews, but I am
> happy for you to start a trend :)

It is my understanding that ::operator new is now infallible in mozilla code, so it would abort instead of return NULL.
Joshua, I know you're quite busy, so just touching base before this possibly bitrots horribly ...

Is there any reason we might want this for TB31?

(This probably should bake well and ride the train if we go out for TB31)
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 7

3 years ago
(In reply to Wayne Mery (:wsmwk) from comment #6)
> Joshua, I know you're quite busy, so just touching base before this possibly
> bitrots horribly ...
> 
> Is there any reason we might want this for TB31?
> 
> (This probably should bake well and ride the train if we go out for TB31)

The main benefit of this is cleaning up some old code, so it's not at all a priority.
Flags: needinfo?(Pidgeot18)

Comment 8

a month ago
Jorg, would these pointer games be something for you? There was a review done, the patch just needs polishing.
Flags: needinfo?(jorgk)
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 9

a month ago
Hey, Aceman, you went into clean-up mode, but I'm still trying to fix bugs our users will benefit from.
Flags: needinfo?(jorgk)
If this reduces failure points or helps the memory reporters be more accurate (which they currently do not) - and I don't know that it will - then it will be a win for users, just not user facing.
Status: ASSIGNED → NEW
Whiteboard: [patchlove]

Comment 11

a month ago
Created attachment 8839599 [details] [diff] [review]
853881-kill-pldhash.patch - refreshed, some (not all!) compile problems fixed.

Refreshed to latest trunk. Sadly there are a few things that didn't compile here.
Most importantly classes nsDataHashtable and nsInterfaceHashtable don't have methods 'Init' and 'IsInitialised' any more. Looks like the need to be initialised in the constructor now.

The worst error is in nsMsgDatabase::CreateMsgHdr():
nsMsgDatabase.cpp(685): error C2664: 'nsMsgHdr::nsMsgHdr(const nsMsgHdr &)': cannot convert argument 2 from 'nsIMdbRow *' to 'already_AddRefed<nsIMdbRow>'

One option to fix this was to revert these changes:
-    nsMsgHdr(nsMsgDatabase *db, nsIMdbRow *dbRow);
+    nsMsgHdr(nsMsgDatabase *db, already_AddRefed<nsIMdbRow> dbRow);

-    nsIMdbRow     *m_mdbRow;
+    nsCOMPtr<nsIMdbRow> m_mdbRow;

-nsMsgHdr::nsMsgHdr(nsMsgDatabase *db, nsIMdbRow *dbRow)
+nsMsgHdr::nsMsgHdr(nsMsgDatabase *db, already_AddRefed<nsIMdbRow> dbRow)

The second option was to use a static cast. No idea whether that's correct or not.

Surprisingly TB even runs with this patch ;-)

(In reply to :aceman from comment #8)
> Jorg, would these pointer games be something for you? There was a review
> done, the patch just needs polishing.
That's extreme wishful thinking. Apart from being a pain to refresh, the underlying technology has changed and some stuff doesn't compile any more, most likely since M-C tightened some screws. Aceman, would you like to lend a hand here before we ask Kent for help?
Flags: needinfo?(acelists)

Comment 12

a month ago
Yes, I can look at the new version.

Comment 13

a month ago
Created attachment 8839731 [details] [diff] [review]
853881-kill-pldhash.patch (v2).

Hi Kent, I've refreshed the patch and solved all the compile problems.

However, I inserted a static cast as marked with XXX. I have addressed some of the issues from your comment #4 (removed 'bar' and 'IsInitialised' had to be removed anyway since the base class doesn't have such a method (any more?)).

There are a few comments left re. refcounting and error handling.

Since this abandoned code fell into my lap and I spent a few hours getting it into shape, could I ask you for your collaboration here to make the adjustments you require yourself. Sorry for the unusual and/or outrageous request, but the idea here is to save Joshua's work, and instead of going through a few loops your direct input would be much appreciated.

Aceman, I solved the compile problems myself, and I'm asking Kent to finish it off.
Attachment #8839599 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8839731 - Flags: review?(rkent)

Comment 14

a month ago
OK, then. But I played with the patch and saw some compile warnigs:

1. reorder variables in the constructor (according to the order in nsMsgDatabase.h):

In file included from tbird-bin/dist/include/nsMailDatabase.h:10:0,
                 from mailnews/db/msgdb/src/nsMsgDatabase.cpp:10:
tbird-bin/dist/include/nsMsgDatabase.h: In constructor ‘nsMsgDatabase::nsMsgDatabase()’:
tbird-bin/dist/include/nsMsgDatabase.h:438:48: warning: ‘nsMsgDatabase::m_headersInUse’ will be initialized after [-Wreorder]
   nsDataHashtable<nsMsgKeyHashKey, nsMsgHdr *> m_headersInUse;
                                                ^
tbird-bin/dist/include/nsMsgDatabase.h:434:12: warning:   ‘uint32_t nsMsgDatabase::m_cacheSize’ [-Wreorder]
   uint32_t m_cacheSize;
            ^
mailnews/db/msgdb/src/nsMsgDatabase.cpp:924:1: warning:   when initialized here [-Wreorder]
 nsMsgDatabase::nsMsgDatabase()
 ^

2. not sure yet what to do with this one. Do we have a problem if hdr is not added to cache? Can we just ignore the return value?

mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘void nsMsgDatabase::AddHdrToCache(nsIMsgDBHdr*, nsMsgKey)’:
mailnews/db/msgdb/src/nsMsgDatabase.cpp:505:55: warning: ignoring return value of ‘bool nsBaseHashtable<KeyClass, DataType, UserDataType>::Put(nsBaseHashtable<KeyClass, DataType, UserDataType>::KeyType, const UserDataType&, const fallible_t&) [with KeyClass = mozilla::mailnews::MsgKeyHashKey; DataType = nsCOMPtr<nsIMsgDBHdr>; UserDataType = nsIMsgDBHdr*; nsBaseHashtable<KeyClass, DataType, UserDataType>::KeyType = const unsigned int&; nsBaseHashtable<KeyClass, DataType, UserDataType>::fallible_t = mozilla::fallible_t]’, declared with attribute warn_unused_result [-Wunused-result]
   m_cachedHeaders.Put(key, hdr, mozilla::fallible_t());
                                                       ^
3. looks like this function just needs removing:

mailnews/db/msgdb/src/nsMsgDatabase.cpp: At global scope:
mailnews/db/msgdb/src/nsMsgDatabase.cpp:3950:1: warning: ‘void msg_DHashFreeStringKey(PLDHashTable*, PLDHashEntryHdr*)’ defined but not used [-Wunused-function]
 msg_DHashFreeStringKey(PLDHashTable* aTable, PLDHashEntryHdr* aEntry)

Comment 15

a month ago
Aceman, can you be a charm and fix those warnings for me and then r?rkent again.

Re. item 2:
We have two options: Either put a MOZ_ASSEERT(???, "Logic error: Entry already in cache");
or explicitly discard the return value. This is the code that got removed:

-nsresult nsMsgDatabase::AddHdrToCache(nsIMsgDBHdr *hdr, nsMsgKey key) // do we want key? We could get it from hdr
-{
-  if (m_bCacheHeaders)
-  {
-    if (!m_cachedHeaders)
-      m_cachedHeaders = PL_NewDHashTable(&gMsgDBHashTableOps, (void *) nullptr, sizeof(struct MsgHdrHashElement), m_cacheSize );
-    if (m_cachedHeaders)
-    {
-      if (key == nsMsgKey_None)
-        hdr->GetMessageKey(&key);
-      if (m_cachedHeaders->entryCount > m_cacheSize)
-        ClearHdrCache(true);
-      PLDHashEntryHdr *entry = PL_DHashTableOperate(m_cachedHeaders, (void *) key, PL_DHASH_ADD);
-      if (!entry)
-        return NS_ERROR_OUT_OF_MEMORY; // XXX out of memory
-
-      MsgHdrHashElement* element = reinterpret_cast<MsgHdrHashElement*>(entry);
-      element->mHdr = hdr;
-      element->mKey = key;
-      NS_ADDREF(hdr);     // make the cache hold onto the header
-      return NS_OK;
-    }
-  }
-  return NS_ERROR_FAILURE;

So as far as I can see, the entry is just added, duplicate or not, hmm. What do you read here?
Whiteboard: [patchlove]

Comment 16

a month ago
If it is a duplicate and it can't be added, we could silently ignore it.
I rather asked if it can't be added to cache due to out of memory or something. 
The original code returns with NS_ERROR_OUT_OF_MEMORY.

But it seems we freely empty the whole cache if it is full so it seems it's not critical if an item isn't in the cache (can't be added). I would just ignore the return value of the m_cachedHeaders.Put(). In the new code AddHdrToCache is void so we never check if it succeeded. It probably doesn't matter.

Comment 17

a month ago
Created attachment 8839830 [details] [diff] [review]
853881-kill-pldhash.patch (v2.1)

Solved the 3 compile warnings.
Attachment #8839731 - Attachment is obsolete: true
Attachment #8839731 - Flags: review?(rkent)
Attachment #8839830 - Flags: review?(rkent)
You need to log in before you can comment on or make changes to this bug.