Closed Bug 739467 Opened 9 years ago Closed 4 years ago

db batching not useful anymore

Categories

(MailNews Core :: Database, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: Bienvenu, Assigned: deanzhu1, Mentored)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 4 obsolete files)

nsIMsgDatabase has Start/EndBatch methods - these don't do anything since the pluggable store landing and should be removed, along with the db batching parameter to nsIMsgFolder::EnableNotifications.
Is there a way for batching to be inferred by the pluggable store implementation?  For example, I think a SQLite back-end would really want to have or be able to infer batching so it could know if it should defer its COMMIT (since those can be rather expensive because there are fsync's).
(In reply to Andrew Sutherland (:asuth) from comment #1)
> Is there a way for batching to be inferred by the pluggable store
> implementation?  For example, I think a SQLite back-end would really want to
> have or be able to infer batching so it could know if it should defer its
> COMMIT (since those can be rather expensive because there are fsync's).

are you saying we've already taken a performance hit?
(In reply to Wayne Mery (:wsmwk) from comment #2)
> are you saying we've already taken a performance hit?

No.  There are no SQLite implementations anywhere right now; but if there were, they would benefit from higher level batch signaling.  Specifically, if we are moving 100 messages across folders, we would want 1 COMMIT for all of those, not 100 of them.  My question was a naive prompt that should be answered before removing batch signaling just to make sure there are known better ways to get the batch signalling in cases where it would be beneficial.
(In reply to Wayne Mery (:wsmwk) from comment #2)
> (In reply to Andrew Sutherland (:asuth) from comment #1)
> > Is there a way for batching to be inferred by the pluggable store
> > implementation?  For example, I think a SQLite back-end would really want to
> > have or be able to infer batching so it could know if it should defer its
> > COMMIT (since those can be rather expensive because there are fsync's).
> 
> are you saying we've already taken a performance hit?

No, the opposite. Before pluggable stores, the lowest level of the nsMsgDatabase methods rewrote the message flags in the x-mozilla-status header, one message at at a time, and the batching was used to tell the nsMsgDatabase to keep an open file handle to the mailbox across calls to change the flags. The pluggable store methods take an array of messages to operate on, so they can do their own batching.
The answer to Andrew's question is the move/copy commands take an array of messages, so they can do a single commit if they want, w/o any higher level batching.
Keywords: perf
(don't know why I added perf for obsolete code)
Assignee: mozilla → nobody
Keywords: perf
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][lang=c++]
Mentor: acelists
Hi, 
I'd like try and fix this bug.
So I have been trying to understand the problem and the fixes that have been proposed, could you check if my understanding is correct?

1) Since thunderbird changed its store landing somecode has become obsolete.
FIX
a) Removing the obsolete code, i.e. the startbatch and endbatch methods
b) Modifying the nsIMsgFolder::EnableNotifications method to take one parameter less which is the batching argument, that means that we should change the method header and every call to the method.

Is this correct?
Yes, that is what was said.
So I did everything I mentioned above, and recompiled the project. 
As it wasnt really a bug fix but a cleaning old code I do not know how to test if it was done correctly, although I dont see any problem with it.
Should I submit a patch now?
Out of curiosity, is tnere any reason mozilla uses mercurial instead of Git?
(In reply to Dean Zhu from comment #11)
> So I did everything I mentioned above, and recompiled the project. 
> As it wasnt really a bug fix but a cleaning old code I do not know how to
> test if it was done correctly, although I dont see any problem with it.

Probably if it compiled it should be correct. But I'll see.

> Should I submit a patch now?

Yes please.

> Out of curiosity, is tnere any reason mozilla uses mercurial instead of Git?

I don't know, you could look in google search, maybe it is mentioned in some old articles.
I was rechecking before I submitted the patch and I have ran into a bug.
The OP talked about the start/end batch method in the nsIMsgDatabase, but I can't manage to find its source file and mistakenly modified another file.

In addition I have a doubt about the workflow. My approach right now is simply going to the dxr webpage and looking for all instances of enablenotifications. And then modifying them one by one, is it ok? Or I should follow another method?

Thank you!
(In reply to Dean Zhu from comment #13)
> I was rechecking before I submitted the patch and I have ran into a bug.
> The OP talked about the start/end batch method in the nsIMsgDatabase, but I
> can't manage to find its source file and mistakenly modified another file.

Ok, so what is the problem? Can you discard that change/patch and start anew?

> In addition I have a doubt about the workflow. My approach right now is
> simply going to the dxr webpage and looking for all instances of
> enablenotifications. And then modifying them one by one, is it ok? Or I
> should follow another method?

So you can also use something like "grep -r -i enablenotifications" on the local files. dxr can be sometimes outdated by several days.
(In reply to :aceman from comment #14)
> Ok, so what is the problem? Can you discard that change/patch and start anew?

I didnt submit the patch so there is no problem with that.

> So you can also use something like "grep -r -i enablenotifications" on the
> local files. dxr can be sometimes outdated by several days.
I tried using grep but for some reason I could not find the source file for nsIMsgDatabase. It only appeared for include headers.
It is mailnews/db/msgdb/public/nsIMsgDatabase.idl .
I finally got around compiling and everything and generating a patch file using hg diff -r tip > BUG739467.patch. How should I submit my patch so someone can review it? I am not really used to mercurial
Use the "Attach file" link on the top of the page. And in the attachment properties, set the "review ?" flag to my email address.
Attached patch Bug739467.patch (obsolete) — Splinter Review
Attachment #8851288 - Flags: review?(acelists)
Comment on attachment 8851288 [details] [diff] [review]
Bug739467.patch

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

Thanks for the patch! It does compile for me fine.

Why are you removing all the trailing spaces? Is your editor doing that automatically? You should turn it off.

Also the patch is missing the required mercurial headers, like:
# HG changeset patch
# User xxx
# Parent  edaca1486019d810ac88f19e2c683d3483650027
Commit message...

You probably need to fix something in the mercurial setup.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +5085,2 @@
>      // we're probably doing something that should be batched.
>      nsCOMPtr <nsIMsgDatabase> database;

Do we still need this comment and variable?

::: mailnews/db/msgdb/public/nsImapMailDatabase.h
@@ +17,5 @@
>    nsImapMailDatabase();
>    virtual ~nsImapMailDatabase();
>    
> +  //NS_IMETHOD    StartBatch() override;
> +  //NS_IMETHOD    EndBatch() override;

Remove the lines.

::: mailnews/db/msgdb/public/nsMailDatabase.h
@@ +26,5 @@
>    NS_IMETHOD DeleteMessages(uint32_t aNumKeys, nsMsgKey* nsMsgKeys,
>                              nsIDBChangeListener *instigator) override;
>  
> +  //NS_IMETHOD StartBatch() override;
> +  //NS_IMETHOD EndBatch() override;

Remove the lines.

::: mailnews/db/msgdb/src/nsMailDatabase.cpp
@@ +73,1 @@
>  NS_IMETHODIMP nsMailDatabase::StartBatch()

Remove the functions.

::: mailnews/local/src/nsLocalUndoTxn.cpp
@@ +267,4 @@
>            }
>          }
>        }
> +    

Do not add the spaces, just an empty line.
Attachment #8851288 - Flags: review?(acelists) → feedback+
I have looked into the issues you've mentioned and fixed those, I'm not sure about the trailing spaces issue I will look into that later.
I tried following the guide you posted about submitting the patch but couldnt follow it, when I try to use "hg bzexport"
it tells me to refresh first. Should I resubmit a patch with the fixes you mentioned?
Yes you can try to refresh and use the hg bzexport extension (but I do not use it so I can't guide you, I use the mq extension).

Yes, please resubmit or attach the new patch.
Attached patch Bug739467.patch (obsolete) — Splinter Review
Fixed commented code.
Attachment #8851288 - Attachment is obsolete: true
Attachment #8851396 - Flags: review?
Attached patch Bug739467.patch (obsolete) — Splinter Review
Forgot to specify reviewer, the thing with spaces is still going on, I'll try and see what can I do for the next time.
Attachment #8851397 - Flags: review?(acelists)
Nice that you remove a lot of trailing spaces, but can you please attach a patch without those changes. It's very hard to see what you've changed. Also see comment #20:
> Why are you removing all the trailing spaces? Is your editor doing that automatically?
> You should turn it off.

In one case, where you removed |srcDB->EndBatch();| you even added trailing spaces. Click on "Splinter Review" above next to the attachment to see it.

Please submit a patch that only shows the real changes, not white-space changes. Also, when you do, please supersede the previous versions.
Hi,
I didn't notice my diff files had so many trailing spaces, I thougth it was only on parts I had edited.
In any case I have been looking at my local files and I cannot find this spaces.
Could the problem be caused by the diff command?
I am currently using hg diff -g > Bug739467.patch
Attached patch Bug739467.patch (obsolete) — Splinter Review
I added a -w option so it seems to be fixed. 
If there is anything I should change please say so.
Attachment #8851396 - Attachment is obsolete: true
Attachment #8851397 - Attachment is obsolete: true
Attachment #8851396 - Flags: review?
Attachment #8851397 - Flags: review?(acelists)
Attachment #8851402 - Flags: review?(acelists)
We all use Mercurial queues, so |hg qnew -m "Bug xxx - fix yyy. r=aceman" xxx.patch|. This way you can handle multiple patches/bugs at the same time. I suggest you do that now, then |hg qpop| that patch, edit it to remove all the unnecessary hunks, then |hg qpush| and |hg qref|

Also, set up your mercurial.ini to contain:
[diff]
git = 1
showfunc = 1
unified = 8

Also set up .hg/hgrc with
[ui]
username = name <ee@domain.com>

We need real patch fines with a header, not diffs.

Also see: https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues

You don't want "-w" (ignore white-sapce). You somehow changed all the files removing white-space.
Comment on attachment 8851402 [details] [diff] [review]
Bug739467.patch

That diff file is fine in principle. I can apply it. Generally you want to follow what I said in the previous comment. Submit patches, not diff files.

Also, please no "-w". There is no need, if your editor leaves the white space intact.

So my suggestion: |hg revert --all| to undo everything. Then apply the diff file (patch < diff.file), then work on getting a proper patch.
Attached patch Bug739467.patchSplinter Review
OK, I've done it for you. This is what we need.
Attachment #8851402 - Attachment is obsolete: true
Attachment #8851402 - Flags: review?(acelists)
Attachment #8851404 - Flags: review?(acelists)
I have tried using 2 editors (emacs and atom) and both aren't showing any trailing whitespaces, so it's quite weird.
I am not really familiar with mercurial yet, so I submitted a diff so at least someone could review it. I will try and learn in the future. 
Thank You!
Comment on attachment 8851404 [details] [diff] [review]
Bug739467.patch

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

Ok, this looks good now. Thanks for the patches guys.

::: mailnews/db/msgdb/public/nsImapMailDatabase.h
@@ +15,5 @@
>    // for the folder. This is mainly because we're deriving from nsMailDatabase;
>    // Perhaps we shouldn't...
>    nsImapMailDatabase();
>    virtual ~nsImapMailDatabase();
>    

Now, these spaces could go :)
Attachment #8851404 - Flags: review?(acelists) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
(In reply to :aceman from comment #32)
> Now, these spaces could go :)
I'll see whether I'll remove them at check-in. However, I once tried to remove trailing spaces in the vicinity of changes in another bug and it was a never ending spiral since the larger the patch became, the more trailing spaced moved into view.
https://hg.mozilla.org/comm-central/rev/c30484eedecc8d09f2e6158fa4b85c957cee84da
(removed spaces a per comment #32)

Dean, thanks for your contribution!
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Thank you for all the help, just one last question.
I'm trying to use the mq extention, but the patch files dont show the line with user:
I have username set up in $(HOME)/.hgrc 
Is there anything else I should do?
It's $(HOME)/mercurial.ini and in each repo .hg/hgrc. In the latter one you set the user as shown in comment #28.
You need to log in before you can comment on or make changes to this bug.