db batching not useful anymore

RESOLVED FIXED in Thunderbird 55.0

Status

RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Bienvenu, Assigned: deanzhu1, Mentored)

Tracking

Trunk
Thunderbird 55.0
x86_64
Windows 7

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Comment 2

6 years ago
(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.
(Reporter)

Comment 4

6 years ago
(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.
(Reporter)

Comment 5

6 years ago
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.

Updated

4 years ago
Keywords: perf

Comment 6

4 years ago
(don't know why I added perf for obsolete code)
Assignee: mozilla → nobody
Keywords: perf
Whiteboard: [good first bug]

Updated

2 years ago
Whiteboard: [good first bug] → [good first bug][lang=c++]

Updated

a year ago
Mentor: acelists
(Assignee)

Comment 7

a year ago
Hi, 
I'd like try and fix this bug.
(Assignee)

Comment 9

a year ago
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

a year ago
Yes, that is what was said.
(Assignee)

Comment 11

a year ago
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

a year ago
(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.
(Assignee)

Comment 13

a year ago
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

a year ago
(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.
(Assignee)

Comment 15

a year ago
(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

a year ago
It is mailnews/db/msgdb/public/nsIMsgDatabase.idl .
(Assignee)

Comment 17

a year ago
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

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

Comment 19

a year ago
Created attachment 8851288 [details] [diff] [review]
Bug739467.patch
Attachment #8851288 - Flags: review?(acelists)

Comment 20

a year ago
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+
(Assignee)

Comment 21

a year ago
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

a year ago
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.
(Assignee)

Comment 23

a year ago
Created attachment 8851396 [details] [diff] [review]
Bug739467.patch

Fixed commented code.
Attachment #8851288 - Attachment is obsolete: true
Attachment #8851396 - Flags: review?
(Assignee)

Comment 24

a year ago
Created attachment 8851397 [details] [diff] [review]
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.
Attachment #8851397 - Flags: review?(acelists)

Comment 25

a year ago
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.
(Assignee)

Comment 26

a year ago
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
(Assignee)

Comment 27

a year ago
Created attachment 8851402 [details] [diff] [review]
Bug739467.patch

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)

Comment 28

a year ago
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 29

a year ago
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

a year ago
Created attachment 8851404 [details] [diff] [review]
Bug739467.patch

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

Comment 31

a year ago
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

a year ago
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+

Updated

a year ago
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 33

a year ago
(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

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

Dean, thanks for your contribution!
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
(Assignee)

Comment 35

a year ago
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

a year ago
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.