739467 - db batching not useful anymore

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[David :Bienvenu]

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.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 2

[Wayne Mery (:wsmwk)]

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

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 4

[David :Bienvenu]

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

Comment 5

[David :Bienvenu]

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.

Comment 6

[Wayne Mery (:wsmwk)]

(don't know why I added perf for obsolete code)

Comment 7

[Dean Zhu]

Hi, 
I'd like try and fix this bug.

Comment 8

[:aceman]

Yes you can.
Get Thunderbird building:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Thunderbird_build

Create the patch:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

If you will be making multiple patches for multiple bugs or will edit the one patch in steps:
https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues

Comment 9

[Dean Zhu]

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?

Comment 10

[:aceman]

Yes, that is what was said.

Comment 11

[Dean Zhu]

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?

Comment 12

[:aceman]

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

Comment 13

[Dean Zhu]

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!

Comment 14

[:aceman]

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

Comment 15

[Dean Zhu]

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

Comment 16

[:aceman]

It is mailnews/db/msgdb/public/nsIMsgDatabase.idl .

Comment 17

[Dean Zhu]

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

Comment 18

[:aceman]

Use the "Attach file" link on the top of the page. And in the attachment properties, set the "review ?" flag to my email address.

Comment 19

[Dean Zhu]

Attachment: Bug739467.patch

Comment 20

[:aceman]

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.

Comment 21

[Dean Zhu]

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?

Comment 22

[:aceman]

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.

Comment 23

[Dean Zhu]

Attachment: Bug739467.patch

Fixed commented code.

Comment 24

[Dean Zhu]

Attachment: Bug739467.patch

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.

Comment 25

[Jorg K (CEST = GMT+2)]

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.

Comment 26

[Dean Zhu]

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

Comment 27

[Dean Zhu]

Attachment: Bug739467.patch

I added a -w option so it seems to be fixed. 
If there is anything I should change please say so.

Comment 28

[Jorg K (CEST = GMT+2)]

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

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 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: Bug739467.patch

OK, I've done it for you. This is what we need.

Comment 31

[Dean Zhu]

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 32

[:aceman]

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

Comment 33

[Jorg K (CEST = GMT+2)]

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

Comment 34

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c30484eedecc8d09f2e6158fa4b85c957cee84da
(removed spaces a per comment #32)

Dean, thanks for your contribution!

Comment 35

[Dean Zhu]

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?

Comment 36

[Jorg K (CEST = GMT+2)]

It's $(HOME)/mercurial.ini and in each repo .hg/hgrc. In the latter one you set the user as shown in comment #28.