nsMsgLocalMailFolder::GetSizeOnDisk seems to return wrong value for maildir store

ASSIGNED
Assigned to

Status

MailNews Core
Database
ASSIGNED
3 years ago
8 months ago

People

(Reporter: aceman, Assigned: rkent)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
nsMsgLocalMailFolder::GetSizeOnDisk should return the size of the folder, but it reads it from the size of the file. This can't work on maildir, as there are multiple files in the filesystem folder. This function should probably call into the msgStore the get the size on disk and the stores (mbox and maildir) should get a GetSizeOnDisk method each.
Created attachment 8448715 [details] [diff] [review]
Patch v1

Possible fix.
Attachment #8448715 - Flags: feedback?(acelists)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8448715 [details] [diff] [review]
Patch v1

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

I didn't test it on a real maildir account as I haven't yet delved into that. But I tried that it doesn't break mbox :)
I deleted .msf of a folder and it was recreated fine and the size was picked up fine (shown in folder -> properties).

The bug was found by Neil so he could take credit by looking at the fix :)

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +90,5 @@
> +   * Returns the size of the folder on the disk.
> +   *
> +   * @param aFolder folder we want to get the size of.
> +   */
> +  int64_t getFolderSizeOnDisk(in nsIMsgFolder aFolder);

I think we use 'long long' in .idl files. And thanks for preparing for 64bit!

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1121,2 @@
>      int64_t folderSize;
> +    msgStore->GetFolderSizeOnDisk(this, &folderSize);

Yes, this is what I had in mind. Please check return value (rv) of msgStore->GetFolderSizeOnDisk().

Can you please check whether we call GetFolderSizeOnDisk often? As the maildir variant will be very expensive, getting the metadata of all the files.

Is mFolderSize stored in the database (folder caches) and updated automatically when new messages are put into the folder?

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +981,5 @@
> +    if (fileName.IsEmpty())
> +      continue;
> +    folderPath->Clone(getter_AddRefs(filePath));
> +    filePath->AppendNative(fileName);
> +    rv = filePath->GetFileSize(&msgSize);

So this iterates over the message database (index) and gets the files of the messages and gets size of each of them.

Another approach is to just use folderPath.directoryEntries directly. Depends on whether the message db is authoritative on what is in the folder or the on disk files are authoritative (any newly found files should be automatically picked up as in case of mbox).

@@ +985,5 @@
> +    rv = filePath->GetFileSize(&msgSize);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    // 200 is a per-message overhead to account for any extra data added
> +    // to the message.
> +    *aFolderSize += msgSize + 200;

Where does this number come from? Isn't the file size final?
Attachment #8448715 - Flags: feedback?(neil)

Comment 3

3 years ago
(In reply to aceman from comment #2)
> Can you please check whether we call GetFolderSizeOnDisk often? As the
> maildir variant will be very expensive, getting the metadata of all the
> files.
> 
> Is mFolderSize stored in the database (folder caches) and updated
> automatically when new messages are put into the folder?
It's not cached in the folder database, only in the folder object. It's refreshed
* When the size is first queried
* After a copy completes
* After a compact completes
* When a folder is updated

> Another approach is to just use folderPath.directoryEntries directly.
> Depends on whether the message db is authoritative on what is in the folder
> or the on disk files are authoritative (any newly found files should be
> automatically picked up as in case of mbox).
I'd have to read the maildir store code to work out which is right.

> > +    // 200 is a per-message overhead to account for any extra data added
> Where does this number come from? Isn't the file size final?
Sounds bogus to me too.

Comment 4

3 years ago
Comment on attachment 8448715 [details] [diff] [review]
Patch v1

Ask me for feedback again if you want me to check whether the maildir store should just enumerate all the files.

>+  int64_t size;
>+  rv = file->GetFileSize(&size);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  *aFolderSize = size;
>+
>+  return rv;
return file->GetFileSize(aFolderSize);

>+  rv = db->EnumerateMessages(getter_AddRefs(messages));
>+  nsCOMPtr<nsISupports> support;
>+  nsCOMPtr<nsIFile> folderPath;
>+  nsCOMPtr<nsIFile> filePath;
>+  rv = aFolder->GetFilePath(getter_AddRefs(folderPath));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  folderPath->Append(NS_LITERAL_STRING("cur"));
>+  nsAutoCString fileName;
>+
>+  if (!messages) {
Check messages is non-null first, before fiddling around with the path.

>+    *aFolderSize = 0;
You already set it to zero.
Attachment #8448715 - Flags: feedback?(neil)
Created attachment 8449324 [details] [diff] [review]
Patch v1.6

Made the suggested changes.
Also, I tried to manually place a few message files in maildir folder.
They weren't picked up automatically on TB restart. On repairing the folder
they were picked up and the size was updated. So,I think iterating over the messages from the db entries should be fine. Please let me know if this needs to be changed.

Thanks.
Attachment #8448715 - Attachment is obsolete: true
Attachment #8448715 - Flags: feedback?(acelists)
Attachment #8449324 - Flags: review?(kent)
Attachment #8449324 - Flags: feedback?(acelists)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8449324 [details] [diff] [review]
Patch v1.6

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

Thanks, looks fine to me now. I haven't run tested this but I trust you have done so as you have a maildir account set up :)
Attachment #8449324 - Flags: feedback?(acelists) → feedback+
(Assignee)

Comment 7

3 years ago
(In reply to neil@parkwaycc.co.uk from comment #3)
> (In reply to aceman from comment #2)
> > Can you please check whether we call GetFolderSizeOnDisk often? As the
> > maildir variant will be very expensive, getting the metadata of all the
> > files.
> > 
> > Is mFolderSize stored in the database (folder caches) and updated
> > automatically when new messages are put into the folder?
> It's not cached in the folder database, only in the folder object. It's
> refreshed
> * When the size is first queried
> * After a copy completes
> * After a compact completes
> * When a folder is updated
> 

folderSize IS an attribute of the folderCache, see http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1305

This exists mainly to support the display of folder size in the folder pane, which was moved to an extension (under protest by yours truly) and now is a poorly maintained extension with almost 200,000 users.
(Assignee)

Comment 8

3 years ago
It's hard to know how much to demand of this patch, since there are lots of issues in the existing implementation of folderSize. Let me just comment on various issues at least:

1) Code should not use mFolderSize = 0 as a flag to force a refresh, as that is needed to give the correct OnIntPropertyChanged notification. The actual refresh work should be in RefreshSizeOnDisk, which should probably be an nsIMsgFolder method. If that is fixed, you should call nsMsgDBFolder::SetSizeOnDisk instead of setting mFolderSize directly, so that we have a single place where the NotifyIntPropertyChange is done.

2) Note that News declares NS_IMETHODIMP nsMsgNewsFolder::RefreshSizeOnDisk() but refreshSizeOnDisk is not a method of nsIMsgNewsFolder, only nsIMsgLocalFolder.

3) folderSize needs to be 64 bit.
(Assignee)

Comment 9

3 years ago
Comment on attachment 8449324 [details] [diff] [review]
Patch v1.6

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

I don't see any unit tests that exercise this. Could you either write a new test, or modify an existing test to read and check the sizeOnDisk property?

Overall, I don't think that calling the OS to get fileSize for potentially thousands of files on the UI thread is going to be acceptable performance. Please change to just som over the fileSize in the msgdatabase instead.

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +86,5 @@
>    boolean hasSpaceAvailable(in nsIMsgFolder aFolder,
>                              in long long aSpaceRequested);
>  
>    /**
> +   * Returns the size of the folder on the disk.

This description merely repeats the name of the method. What do you mean by "the folder"? What units are size? What all is included? Please try to anticipate how people might be confused, and give a better description. I don't want to prescribe to you exactly what is needed, think it through and propose for yourself a better description.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1108,5 @@
>      NotifyIntPropertyChanged(kFolderSizeAtom, oldFolderSize, mFolderSize);
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP nsMsgLocalMailFolder::GetSizeOnDisk(uint32_t* aSize)

This exact same code also exists in news folders. Please make the same change there.

@@ +1124,1 @@
>      mFolderSize = (uint32_t) folderSize;

This cast will certainly overflow and give an incorrect result if folderSize is larger than 4GB, which we are trying to allow. If you are not going to fix that (which probably should be in a separate bug) then perhaps we could agree on a compromise?

Here is a suggestion. Use (uint32_t)(-1) as the flag for "not initialized" instead of zero, and set this to 0 when it overflows. Or perhaps set it to 1 when it overflows, as 1 byte size would look obviously incorrect? Any other ideas (short of reworking correctly to make this 64 bit)?

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +949,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aFolder);
> +  NS_ENSURE_ARG_POINTER(aFolderSize);
> +
> +  *aFolderSize = 0;

Why are you resetting this here? I would normally expect that this value is unmodified if there is an early return from an error, not zero.

@@ +981,5 @@
> +    folderPath->Clone(getter_AddRefs(filePath));
> +    filePath->AppendNative(fileName);
> +    rv = filePath->GetFileSize(&msgSize);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    *aFolderSize += msgSize;

Sorry, but my sense of this is that this is unacceptably slow. You are iterating over potentially thousands of files to collect their size, all on the UI thread. It seems to me that this will cause a jank of the user interface on large folders every time it is called. This is not a particularly important property, and is not worth this jank.

The message size is already stored in the database for each message. Yes the folder size from the OS is a more reliable indicator of size, but  I would much prefer if we just used the existing memory-resident message size from the db and summed over it instead of calling the OS to get a file size, at least for the maildir case.
Attachment #8449324 - Flags: review?(kent) → review-
Created attachment 8455468 [details] [diff] [review]
Patch v1.8

This patch fixes the core issue, trying to address rkent's comment.
TODO: fix the idl comment (will do it soon).
Attachment #8449324 - Attachment is obsolete: true
Attachment #8455468 - Flags: review?(kent)
Created attachment 8455470 [details] [diff] [review]
Test v1

Test for the same.
But, the test prints entirely different values for OS filesize and dbfoldersize methods.

Thanks.
Attachment #8455470 - Flags: feedback?(kent)
(Assignee)

Comment 12

3 years ago
I'd like to point out that IMAP folder has code like this:

3056   uint32_t messageSize;
3057   if (NS_SUCCEEDED(newMsgHdr->GetMessageSize(&messageSize)))
3058     mFolderSize += messageSize;

and mFolderSize is what is read when you do GetSizeOnDisk, so it is not without precedent to use the db size instead of reading the size on the disk.
(Reporter)

Comment 13

3 years ago
I'm OK with that as I understand checking all the thousands of files will be slow.
As long as if the .msf is lost the properties (including message sizes) are fetched from the individual files.
(Assignee)

Comment 14

3 years ago
The code should probably also be changed, matching IMAP, to increment the folder size by the message size when the message is first added to the folder, rather than recalculating the folder size each time a message is added.
(Assignee)

Comment 15

3 years ago
Comment on attachment 8455468 [details] [diff] [review]
Patch v1.8

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

As I look at this, I'm afraid that there is some fundamental design work that needs to be redone for this to work.

In the previous design for local folders, we assumed that it was cheap to get the folder size, hence we regularly called RefreshSizeOnDisk which zeroed out the size and recalculated it. It turns out that this cheapness is related to the mbox store, and does not apply to maildir. So we need to change the way that we keep track of folder size. This patch does not go far enough in solving the problem.

You really need to get rid of the regular calls to RefreshSizeOnDisk, and instead move the logic to keep track of folder size from the folder to the store. For mbox, you can continue to just calculate it freshly when needed. But for maildir, you'll need to treat it differently by storing a previous folderSizeOnDisk, and updating it when anything in the store changes.

If maildir is going to keep track of folder size by adding or subtracting folder changes, then there is no reason why that could not be the actual file size instead of the db size. That is, whenever a file is added or removed from the maildir, you would have logic to get its size and use that to correct the locally stored size.

Because you will need to store that size, then you will need a saved folder property that contains the folder size on disk. Perhaps the existing db variable will do that, I have not checked carefully.

So I hate to do this, but I really think that this needs redoing with a different approach.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1123,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // FIXME This may result in overflow.

There is an ongoing effort to resolve the issues with directory sizes larger than 4 GB, which has been a long standing weakness. Given that, I don't think we should allow a new "gotcha" for large folders. We are getting rid of them, not adding them.

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +944,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsMsgMaildirStore::GetFolderSizeOnDisk(nsIMsgFolder *aFolder, int64_t *aFolderSize)

I would add some comments here that we are not, in fact, getting the "folder size on disk" but instead are getting the total size of the messages. That means that we are ignoring slack space in the files.

@@ +949,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aFolder);
> +  NS_ENSURE_ARG_POINTER(aFolderSize);
> +
> +  uint32_t msgSize = 0, folderSize = 0;

folderSize needs to be 64 bit

::: mailnews/news/src/nsNewsFolder.cpp
@@ +772,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    int64_t folderSize = (uint32_t)(-1); // uninitialized
> +    rv = msgStore->GetFolderSizeOnDisk(this, &folderSize);    

Nit: trailing space.

@@ +773,5 @@
>  
> +    int64_t folderSize = (uint32_t)(-1); // uninitialized
> +    rv = msgStore->GetFolderSizeOnDisk(this, &folderSize);    
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    // FIXME This may result in overflow.

Same issue, I think we need to fix this now and not later.
Attachment #8455468 - Flags: review?(kent) → review-
(Assignee)

Comment 16

3 years ago
Comment on attachment 8455470 [details] [diff] [review]
Test v1

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

In the main bug, I am now suggesting that the store accumulate the value of the disk storage used each time that a maildir file is added to the storage. That would allow you to then use the os file functions to test the value of the GetSizeOnDisk from the folder using file system values.

I suppose that the feedback request was for comments on the differences between the file system values and the message hdr values, which would be moot if we end up doing the changes to the overall design that I suggested. Otherwise we need a test, and this one looks fine.
Attachment #8455470 - Flags: feedback?(kent) → feedback+
Created attachment 8497749 [details] [diff] [review]
approach 2 (WIP)

Okay so this is another approach.
Here I am trying that every message addition/removal
makes the change to the folder size.
Please let me know if this is the right way to proceed.

TODO:
We need to also make sure that the first run of folder->GetSizeOnDisk()
has an invalid value and it calls msgStore->GetFolderSizeOnDisk() as shown
by approach 1. And, once this (folder->GetSizeOnDisk()) is a valid value, 
the msgStore methods should handle it accordingly.

Thanks.
Attachment #8455468 - Attachment is obsolete: true
Attachment #8497749 - Flags: feedback?(kent)
(Reporter)

Comment 18

3 years ago
(In reply to Suyash Agarwal (:sshagarwal) from comment #17)
> Created attachment 8497749 [details] [diff] [review]
> approach 2 (WIP)
> 
> Okay so this is another approach.
> Here I am trying that every message addition/removal
> makes the change to the folder size.
> Please let me know if this is the right way to proceed.
Looks like it, if we want to remove the perf hit of iterating all the files in the filesystem folder.
GetSizeOnDisk may be called any number of times, e.g. for each folder and should return the proper value after any change of messages (users may have the Extra folders column extension enabled).
It is not a one time query, e.g. when user opens folder properties.

One thing I do not understand int he patch, is in DeleteFolder or AddSubfolder, you seem to be manipulating the size of the parent folder. Is that right and is that really needed? I think folders do not include the size of their subfolders in their own size.

> TODO:
> We need to also make sure that the first run of folder->GetSizeOnDisk()
> has an invalid value and it calls msgStore->GetFolderSizeOnDisk() as shown
> by approach 1. And, once this (folder->GetSizeOnDisk()) is a valid value, 
> the msgStore methods should handle it accordingly.

Yes, it seems to me for the initialization of mFolderSize at first run (or when .msf file is wiped) you need to resurrect patch v1.
On the other hand, isn't there a method to get the size of a directory? Do we really need to iterate all files in it? I know on some filesystems the size of directory may return only the size of the metadata (size of file names), not the total sum of the sizes of files in it. It is not sure from nsIFile::GetFileSize, whether is has solved this or just returns whatever the filesystem returns.
(Reporter)

Comment 19

3 years ago
Also, does the WIP patch actually compile/work? When I tried to make the sizes 64bit, it needed some more changes. See https://bug789679.bugzilla.mozilla.org/attachment.cgi?id=738666 .
We should somehow coordinate the effort.
(Reporter)

Updated

3 years ago
Blocks: 1078367
(In reply to :aceman from comment #19)
> Also, does the WIP patch actually compile/work? When I tried to make the
> sizes 64bit, it needed some more changes. See
> https://bug789679.bugzilla.mozilla.org/attachment.cgi?id=738666 .
> We should somehow coordinate the effort.

Ya, it needs more changes. I made those extra needed changes and now it crashes for me.
I need to tweak it a bit more. Moreover, I was planning to shift this redundant code everywhere to a single helper function and call it. Or, we can coordinate the efforts as you said :)
Or, maybe an entirely different approach is needed.

(In reply to :aceman from comment #18)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #17)
> Looks like it, if we want to remove the perf hit of iterating all the files
> in the filesystem folder.
> GetSizeOnDisk may be called any number of times, e.g. for each folder and
> should return the proper value after any change of messages (users may have
> the Extra folders column extension enabled).
> It is not a one time query, e.g. when user opens folder properties.

Ya, so I was planning, once mFolderSize is set, we can simply return it whenever
queried. And update it only when addition/deletion of files/folders takes place.
 
> One thing I do not understand int he patch, is in DeleteFolder or
> AddSubfolder, you seem to be manipulating the size of the parent folder. Is
> that right and is that really needed? I think folders do not include the
> size of their subfolders in their own size.
>
Not sure, I thought the folder size should comprise the size of everything
that's inside it. And, I do think that's logically correct as well.
 
> > TODO:
> > We need to also make sure that the first run of folder->GetSizeOnDisk()
> > has an invalid value and it calls msgStore->GetFolderSizeOnDisk() as shown
> > by approach 1. And, once this (folder->GetSizeOnDisk()) is a valid value, 
> > the msgStore methods should handle it accordingly.
> 
> Yes, it seems to me for the initialization of mFolderSize at first run (or
> when .msf file is wiped) you need to resurrect patch v1.
Ya, I also found we need something from it.

> On the other hand, isn't there a method to get the size of a directory? Do
> we really need to iterate all files in it? I know on some filesystems the
> size of directory may return only the size of the metadata (size of file
> names), not the total sum of the sizes of files in it. It is not sure from
> nsIFile::GetFileSize, whether is has solved this or just returns whatever
> the filesystem returns.

Not much idea what
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileUnix.cpp#1227
means.
(Reporter)

Comment 21

3 years ago
(In reply to Suyash Agarwal (:sshagarwal) from comment #20)
> Ya, it needs more changes. I made those extra needed changes and now it
> crashes for me.
> I need to tweak it a bit more. Moreover, I was planning to shift this
> redundant code everywhere to a single helper function and call it. Or, we
> can coordinate the efforts as you said :)
> Or, maybe an entirely different approach is needed.
OK, let's first wait if rkent confirms the current direction and then we can finish it.

> > One thing I do not understand int he patch, is in DeleteFolder or
> > AddSubfolder, you seem to be manipulating the size of the parent folder. Is
> > that right and is that really needed? I think folders do not include the
> > size of their subfolders in their own size.
> >
> Not sure, I thought the folder size should comprise the size of everything
> that's inside it. And, I do think that's logically correct as well.
No, I don't think it works this way. The folder.getsizeondisk returns only the size of messages the folder contains directly. In the mbox case you see it only gets the size of the one file. No files of its subfolders in folder.sbd or anything like that.
(Assignee)

Comment 22

3 years ago
Comment on attachment 8497749 [details] [diff] [review]
approach 2 (WIP)

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

This patch seems to be mostly concerned with propagating the message folder size to parent folders. I agree with aceman that we don't want to do that. You should be able to observe the folder size of just the direct contents of a folder. The user interface that displays folder size (which as I said earlier, was incorrectly removed several years ago and moved to an extension; we should reverse that) should accumulate the folder size when a folder is elided to hide its subfolders, but show the actual non-summed usage when the folder is expanded. This requires that the backend report the unsummed usage for each folder.

As to other feedback on the patch, the key issue really is persistence of the folder size between Thunderbird sessions. I don't see that you have addressed that at all.

We should really split out the changes to update folderSize to 64 bit, from the changes to fix it for maildir. Can you do a simple patch to update folderSize to 64 bit?
Attachment #8497749 - Flags: feedback?(kent) → feedback+
(Reporter)

Comment 23

3 years ago
(In reply to Kent James (:rkent) from comment #22)
> Comment on attachment 8497749 [details] [diff] [review]
> approach 2 (WIP)
> 
> Review of attachment 8497749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems to be mostly concerned with propagating the message folder
> size to parent folders. I agree with aceman that we don't want to do that.
> You should be able to observe the folder size of just the direct contents of
> a folder. The user interface that displays folder size (which as I said
> earlier, was incorrectly removed several years ago and moved to an
> extension; we should reverse that) should accumulate the folder size when a
> folder is elided to hide its subfolders, but show the actual non-summed
> usage when the folder is expanded. This requires that the backend report the
> unsummed usage for each folder.
I agree with you here! I even have that accumulation in the UI implemented in the Extra folder columns extension and use it for some years, the patch just waits somewhere at standard8:)

> As to other feedback on the patch, the key issue really is persistence of
> the folder size between Thunderbird sessions. I don't see that you have
> addressed that at all.
Isn't mFolderSize stored automatically in nsMsgDBFolder::WriteToFolderCacheElem ?

> We should really split out the changes to update folderSize to 64 bit, from
> the changes to fix it for maildir. Can you do a simple patch to update
> folderSize to 64 bit?
That patch is not that simple. If we split that part out, we can then just use
https://bug789679.bugzilla.mozilla.org/attachment.cgi?id=738666 .

I would also be OK with stripping the 64bit part out of this patch. Just use 64bit sizes internally where needed, but keep the interfaces 32bit for now. We can then layer attachment 738666 [details] [diff] [review] on top of it.
(Assignee)

Comment 24

3 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=464973#c60 where I reopen a bug to merge Extra Folder Columns into core. If we can get magnus to buy off on that, that would be a good project for you, aceman.
(In reply to :aceman from comment #0)
> nsMsgLocalMailFolder::GetSizeOnDisk should return the size of the folder, but it reads it from the size of the file.
> This can't work on maildir, as there are multiple files in the filesystem folder.

When MaildirStore is used for a type=pop3 account, sizeOnDisk = 0 was shown.

If MaildirStore is used, mal data is held under following directory. 
    .../<TopLevelFolderName.sbd/ ... /<ParentFolderName>.sbd/<FolderName>/cur
In BerkleyStore, SizeOnDisk is corresponds to file size of .../ ...sbd/<FolderName> file.
In MaildirStore, if sizeOnDisk is supported, sizeOnDisk should retun "total file size under .../ ...sbd/<FolderName>/cur directory".
IIUC, "API for file size of a file" won't return value if the FILE is directory, under almost all OS. This is perhaps a reason of sizeOnDisk = 0.
So, "directory scan for file names under a .../cur directory" and "getting property of each file" is needed if MaildirStore.

As seen in "Directory Property" display in Windows Explorer, it usually takes long if very many files are held in a directory.
Is performance of "directory scan" cared?
FYI. According to following document, "Windows API FindFirstFile... and FindNextFile..." seems a fastest method on Win to scan a directory and get property of each file(file size in this case).
> http://stackoverflow.com/questions/2979432/directory-file-size-calculation-how-to-make-it-faster#3050354
If "directory scan for sizeOnDisk upon each sizeOnDisk query" is too expensive, following can be a solution of performance issue.
    1. MsgDatabase.sizeOnDiskOfMaildir property. (stored in .msf file).
    2. MsgDatabase.sizeOnDiskOfMaildir is read only property, except for;
        - Mail copy/move to MaildirStore(add cur/nnnnnnnn file), or Mail delete from MaildirStore(remove cur/nnnnnnnn file)
           Increment or reduce sizeOnDiskOfMaildir value upon each mail add or delete.
        - Repair Folder/Compact of folder(recalculate file size under cur directory and refresh sizeOnDiskOfMaildir value)
Because Repair Folder/Compact of folder is batch type job on entire mail folder, "cost of directory scan" is not so expensive.

Is such enhancement possible?

If IMAP folder, sizeOnDisk looks to return "total of mail size in all MsgDBHdr in IMAP Mbox which is returned from server" regardless of Offline-Use setting/auto-sync option.
If Offline-Use=On IMAP folder, both of next are natural expectation.
    (A) analogy from local mail folder for type=pop3 or none.
           sizeOnDisk = file size of Offline-Store file if BerkleyStore.
                                total file size under <FolderName>/cur directory which is used as Offline-Store.
    (B) analogy from imap mbox witth Offline-Use=Off folder for type=imap. (this is current implementation)
           sizeOnDisk = "total of mail size in all MsgDBHdr", regardless of Berkley/MaildirStore, Offline-UseOn/Off.
                                 This depends on IMAP Delete model, because MsgDBHdr of "flagged as \Deleted" is removed immediately
                                 if "Move to trash" or "remove immediately", but MsgDBHdr is not removed of "mark it s deleted".

Which is definition of sizeOnDisk in IMAP

If "sizeOnDisk in MaildirStore of type=pop3 or type=none" is calculated from MsgDBHdr as done in IMAP folder, I think "performance problem due to directory scan if MaildirStore(cur directory)" can be avoided.

Is it possible?
Created attachment 8506286 [details] [diff] [review]
Another approach WIP v1

So, this patch *breaks* mbox as of now.
But it works on maildir. We don't need timely
unnecessary computations now, its just once when
done and *not* every time whenever needed.
There are very few calculation calls now.
Please let me know if this approach is approved and
I'll port it to mbox as well.

Thanks.
Attachment #8497749 - Attachment is obsolete: true
Attachment #8506286 - Flags: feedback?(kent)
Attachment #8506286 - Flags: feedback?(acelists)
(Reporter)

Comment 28

3 years ago
Comment on attachment 8506286 [details] [diff] [review]
Another approach WIP v1

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

Why does it break mbox? Due to the removed RefreshSizeOnDisk calls?
So what is the idea behind this implementation? I see you update size of maildir at each adding/removing a message.
What do you plan to change in mbox?
Attachment #8506286 - Flags: feedback?(acelists)
(In reply to :aceman from comment #28)
> Another approach WIP v1
> Why does it break mbox? Due to the removed RefreshSizeOnDisk calls?
Ya.
> So what is the idea behind this implementation? I see you update size of
> maildir at each adding/removing a message.
> What do you plan to change in mbox?
The same. I'll include size change on every operation in mbox like done in maildir.
(Reporter)

Comment 30

3 years ago
(In reply to Suyash Agarwal (:sshagarwal) from comment #29)
> > So what is the idea behind this implementation? I see you update size of
> > maildir at each adding/removing a message.
> > What do you plan to change in mbox?
> The same. I'll include size change on every operation in mbox like done in
> maildir.

Why should that be easier that the current method?
(In reply to :aceman from comment #30)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #29)
> > > So what is the idea behind this implementation? I see you update size of
> > > maildir at each adding/removing a message.
> > > What do you plan to change in mbox?
> > The same. I'll include size change on every operation in mbox like done in
> > maildir.
> 
> Why should that be easier that the current method?

I don't think that'll be easier but at least, it centralizes the control (as suggested by rkent
that we remove the scattered RefreshOnDisk() calls) and moreover, the control is moved from
folder to store.
And, consistency is observed between implementations in maildir and mbox.
(Reporter)

Comment 32

3 years ago
Yes, I agree now the store should track the disk size. But the method that does it may be specific to each store so you do not need to complicate the mbox one just because the maildir needs a specific way (tracking each operation).
Ya but, I want to remove RefreshSizeOnDisk() and so we need to update the size somehow.
So, if not refreshsizeondisk()  (update when needed), do it as in maildir (update when done)?
(In reply to Suyash Agarwal (:sshagarwal) from comment #33)
> Ya but, I want to remove RefreshSizeOnDisk() and so we need to update the size somehow.
> So, if not refreshsizeondisk()  (update when needed), do it as in maildir (update when done)?

Some reasons why current RefreshSizeOnDisk() like one is needed in local mail folder of BerkleyStore  :
   - Compaact folder uses SizeOnDisk in order to check actual Berkley LocalStore file size after Compct operation.
     If "Total data size required to write by Compact"  != "Actual file size after Compact", it raises error.
   - Upon MsgDB close, (a) "Actual file size of Berkley LocalStore file" and its (b) "last modified timestamp" is written to .msf fille.
      And upon MsgDB open, "(a) & (b) held in .msf file" is compared with actual "(a) & (b) upon MsgDB open".
      This is done for "outdated msf condition" detection in LocalStore file wth BerkleyStore.
In both cases, "actual file size on disk when size is requested" is needed.
"Sum of msg size in MsgDBHdr like one in IMAP" can't be used in these cases of LocalStore file wth BerkleyStore.

(In reply to :aceman from comment #32)
> But the method that does it may be specific to each store so you do not need to complicate the mbox one
> just because the maildir needs a specific way (tracking each operation).

I believe  that "Another approach WIP v1" is an approch which is to avoid performance issue in "Directory scan for MaildirStore when many files are held in a directory" with keeping current "actual file size when size is requested for BerkleyStore LocalStore file".

If imap, GetSizeOnDisk() simply returns mFolderSize.
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#1770
And mFolderSize is incremented upon each NormalEndHeaderParseStream for an UID, because msg size is always obtained from server.
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#3058
Size of Offline-store file is irrelevant in imap.
I believe that this is done "just because the IMAP needs a specific way".

If MaildirStore LocalStore file, one of next is needed.
   - hook each mail add/each mail delete, and update total size in mFolderSize as done in imap, or save in FolderCache etc.
     (imap like approach, but "uid fetch 1:* rfc822size body.peek[headrs(...])" like one doesn't exist.)
   - get sum of "mail size in all MsDBHdr" upon each SizeOnDisk request (mix of imap like one and berkley like one)
     I think this is faster than /cur directory scan. But this may be slower as seen in "unread mail count of all subfolders" feature.
   - scan .../cur directory upon each SizeOnDisk request (berkley like approach)
   - other methods

:aceman, what do you think about "what should be in MaildirStore LocalStore file"?
(Reporter)

Comment 35

3 years ago
I thikn we already agreet what is the prefered method for maildir. I just try to understand why we need to rewrite mbox too.
(Assignee)

Comment 36

3 years ago
Comment on attachment 8506286 [details] [diff] [review]
Another approach WIP v1

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

I spent a few hours (with interruptions) trying to understand the underlying issues, and never got so that I was fully comfortable with the complex notifications and updates required. Overall though, I think that you are on the right track. We need to think through very carefully how the folder cache and message database are used, so that you don't open the message database but use the cache instead when possible. It's hard for me to make sure all this is done with a partial fix.

But in general I think it is fine to 1) update the folder size in the cache and db when the folder size changes, using some measure of the file size.

I'm still not sure if the correct measure of the file size is the one in the db, or whether you should just go to the file directly. In theory iterating all of the files to get the folder size should only happen when the folder is first created and empty, so the file size itself might be better. But if it ever runs, and there are tens of thousands of messages in the folder, it would seriously mess up the UI. Thoughts?

But the tough issue is where to update the folder size, and making sure that the folder size is properly initialed before that, and saved correctly when done, since you are relying on the counts in the dbFolderInfo as the main value for the folder size. To really review that, I need a completed, tested patch.

::: mailnews/news/src/nsNewsFolder.cpp
@@ +773,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    uint32_t folderSize = (uint32_t)(-1); // uninitialized
> +    rv = msgStore->GetFolderSizeOnDisk(this, &folderSize);    

Nit: extra space at end.
Attachment #8506286 - Flags: feedback?(kent) → feedback+
Created attachment 8507554 [details] [diff] [review]
Patch v2

Okay so this patch works fine for both maildir and mbox but separately.
That is, I am deciding whether we need to call RefreshSizeOnDisk() based
on the contract store ID. Since, I am unable to get the store type via
the folder or nsIMsgPluggableStore instance, this decision remains global
irrespective of the individual folder store.
Once we are able to get that, I think we can improve this to work efficiently
and correctly with different servers using different stores.

Thanks.
Attachment #8506286 - Attachment is obsolete: true
Attachment #8507554 - Flags: review?(kent)
Attachment #8507554 - Flags: feedback?(acelists)
(Reporter)

Comment 38

3 years ago
Can you rename RefreshSizeOnDisk to UpdateSizeOnDisk, make it a method of the store, and in mbox it ignores any argument and just does reads the size as today (what RefreshSizeOnDisk does). For maildir, it receives one argument containing the value by which the size should be changed (whether negative or positive).
Internally (for maildir) it would do the dance you are open-coding at several places.
UpdateSizeOnDisk(argument) {
rv = folder->GetSizeOnDisk(&folderSize);
NS_ENSURE_SUCCESS(rv, rv);
rv = folder->SetSizeOnDisk(folderSize + argument);
}

Would that fit your concept?
Can you please provide the results of test v1 attached here? for berkeley mailbox?
Since, the output on my system doesn't seem satisfactory even without any patches except this patch.

Thanks.
Flags: needinfo?(acelists)
(Assignee)

Comment 40

3 years ago
I spent all day today trying to understand the underlying issues here. I made some notes on folder data persistence since we are planning to persist the folder size, which has not been an important issue in the past. See https://wiki.mozilla.org/User:Rkent/Folder_Data_Persistence

I'm confident now that I understand how the data persistence works now that we could get it reliably done, but there has to be changes to the expectation of how the folder size is handled in the msgFolder objects.

Also, the calculation of the folder size in the fallback where the persistence could be quite slow, yet the result is usually not particularly critical. This is crying out for an async solution, which is now done with OS.File in Gecko.

Rather than doing a solution that we know will freeze the UI in some cases, I think that the approach that is needed is to write an XPCOM object that will use OS.File to calculate the folder size, calling back to nsIMsgFolder.sizeOnDisk when complete. It seems overkill for this case, but we probably need an XPCOM infrastructure in mailnews for OS.File anyway, so here is the chance.

So let me experiment with that approach a little.
(In reply to Suyash Agarwal (:sshagarwal) from comment #37)
> Since, I am unable to get the store type via the folder or nsIMsgPluggableStore instance, 
> this decision remains global irrespective of the individual folder store.
> Once we are able to get that, I think we can improve this to work efficiently
> and correctly with different servers using different stores.

Currently,  available property of nsIMsgPluggableStore is supportsCompaction only.
> msgFolder.msgStore : [xpconnect wrapped nsIMsgPluggableStore]
>    msgFolder.msgStore / Property
>      supportsCompaction = false
supportsCompaction = false is currently always set to false if MaildirStore, and true is set if BerkleyStore.
And, type of message store is completely hidden pretty well.

Because of following,
   http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIncomingServer.cpp#929
   nsMsgIncomingServer::GetMsgStore(nsIMsgPluggableStore **aMsgStore)
   - "Multiple MsgStoreType per server(account)" is not supported yet.
      Only one MsgStoreType is currently usable for a server(account) .
   - MsgStoreType is currently per server(account) property. 
     MsgStoreType is currently not per folder property. 
  - To keep "default of mail.server.server#.storeContractID = @mozilla.org/msgstore/maildirstore;1", 
     mail.server.server#.storeContractID is currently always written if mail.server.server#.storeContractID is not defined.
  - mail.serverDefaultStoreContractID is not used in nsMsgIncomingServer::GetMsgStore.
    mail.serverDefaultStoreContractID is perhaps referred upon account creation only.
and because I also couldn't find any MsgStoreType related property in Mail&News related objects,
I believe "use prefs.js entry" part of your decision is correct, valid, proper.
However, "using mail.serverDefaultStoreContractID" is wrong.
It should be per server mail.server.server#.storeContractID, as done in nsMsgIncomingServer::GetMsgStore.

From perspective of efficiency in program coding, property like IncomingServer.msgStoreType / msgFolder.msgStoreType(getter to  IncomingServer.msgStoreType), or property like msgFolder.msgStore.msgStoretype, is better implemented.
(Assignee)

Comment 42

3 years ago
I don't feel like we can make progress on this until the 64bit support is added from bug 813459.

But here is the direction I think that we need to go.

1) Make sure that folderSize is correctly saved in both the folder cache and dbFolderInfo. So we have to make sure that we are calling the correct read and write functions when those are changed.

2) Make the pluggable store getSize function an async callback that sets the sizeOnDisk of msgFolder. This changes some how that was used, but the uses of getSize are all (AFAICT) UI functions that respond to listeners anyway, so a delayed setting would be OK. This is to prevent a jank if we need to go to the disk directly to get the value.

I tried to put together some sample demo code for some of these things, but the issues with the 64bit made it ugly. It is senseless to land code with 32 bit folder, only to change it to 64 bit right away. So let's get the 64 bit code issues for folderSize landed before we do anything more here.
Depends on: 813459
(Assignee)

Comment 43

3 years ago
Comment on attachment 8507554 [details] [diff] [review]
Patch v2

Sorry to do the r-, but I want to propose some alternate approaches, and it is hard to do until we get the 64 bit code landed from bug 813459
Attachment #8507554 - Flags: review?(kent) → review-
(Reporter)

Comment 44

3 years ago
(In reply to Suyash Agarwal (:sshagarwal) from comment #39)
> Can you please provide the results of test v1 attached here? for berkeley
> mailbox?
> Since, the output on my system doesn't seem satisfactory even without any
> patches except this patch.
> 
> Thanks.

I do not understand. Why should I run the test and with what patches applied?
Flags: needinfo?(acelists)
(Assignee)

Updated

3 years ago
Depends on: 1093217
(In reply to Kent James (:rkent) from comment #43)
> Comment on attachment 8507554 [details] [diff] [review]
> Patch v2
> 
> Sorry to do the r-, but I want to propose some alternate approaches, and it
> is hard to do until we get the 64 bit code landed from bug 813459

Oh okay. So, I'm waiting till then (on this).
(In reply to :aceman from comment #44)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #39)
> > Can you please provide the results of test v1 attached here? for berkeley
> > mailbox?
> > Since, the output on my system doesn't seem satisfactory even without any
> > patches except this patch.
> > 
> > Thanks.
> 
> I do not understand. Why should I run the test and with what patches applied?

I wanted you to test the test attached on this bug. Just the test without any additional
patches as it was still producing unexpected results for me on MBox. If that happens
for you, either I am making some mistake in the test or mbox is broken as well.
But now, since this still needs to wait, I think we can leave it for now.

Thanks.
(Assignee)

Comment 46

3 years ago
This turns out to be difficult for so many reasons. Because sizeOnDisk has been effectively hidden from users since the fileSize column was moved to the Extra Folder Columns extension, various brokenness in that has not been noticed. But in doing these maildir tests, it becomes very difficult to make sure that maildir is working, when there are underlying issues in the handling of mbox itself. Because we are proposing to merge Extra Folder Columns back into core, these issues will be much more visible in the future.

sizeOnDisk means three different things, which complicates its use to store the maildir size:

1) sizeOnDisk is used as a method of stamping the mork file as in sync with the underlying store. As a result, many of the mechanisms to persist sizeOnDisk occur during mbox validation, rather than during changes in the underlying values.

2) sizeOnDisk for local folders means the disk space usage for the message files, which is the interpretation we expect to use here.

3) sizeOnDisk for IMAP folders is the sum of reported message sizes from the server, which is completely different than the meaning here. You need to take steps to prevent the maildir folderSize from redefining IMAP's sizeOnDisk when the offline message store is maildir.

I have been working on a patch that solves some of these underlying issues, and also works with maildir. I'll try to separate the two issues, and put the non-maildir changes into bug 1093217. The resulting maildir patch will mostly be the existing patch in this bug, but will depend on the patches in bug 1093217.
Just an idea.

There are 3 kinds of "File Size On Disk" ;
> (A) Call sizeOnDisk
>       msgFolder.sizeOnDisk for total mail size display.
> (B) Call LastFileSizeUponMsfUpdate
>       Actual file size of BerkleyStore file of local mail folder(with modified timestamp) for "outdated msf codition" check
> (C) Call ActuaFileSizeByCompact
>       Actual file size of BerkleyStore file of local mail folder for healthy check of Compact.
Call "recent file size" "ActualFileSize".

Before MaildirStore suppoort :
>                                                   Local mail folder    IMAP Offline-Use=On   IMAP Offline-Use=Off             
> (A) sizeOnDisk                            ActualFileSize       Counted by parser       Counted by parser
> (B) LastFileSizeUponMsfUpdate  ActualFileSize       N/A                              N/A
> (C) ActuaFileSizeByCompact      ActualFileSize       ActualFileSize              N/A
Fortunately, sizeOnDisk==ActualFileSize always if local mail folder.

If MaildirStore, following is needed.
>                                                     Local mail folder      IMAP Offline-Use=On   IMAP Offline-Use=Off             
> (A) sizeOnDisk                            Total size of /cur       Counted by parser       Counted by parser
> (B) LastFileSizeUponMsfUpdate  Size of newest mail  N/A                              N/A
> (C) ActuaFileSizeByCompact      N/A                          N/A                              N/A
For "outdated msf condition" check of MorkDB(msf) for local mail folder,
LastFileSizeUponMsfUpdate is better "file size(and timestamp) of last added /cur/nnnnnnnn file".
Because there is no need of Compact of MailStore file in MaildirStore, ActuaFileSizeByCompact is irrelevant.

I believe main purpose of msgFolder.sizeOnDisk is "total mail size display".
So, "separation of (B)/(C) from (A)" is needed first.
If (B)/(C) is separated from (A), we can continue to use current GetSizeOnDisk definition.
For (B) LastFileSizeUponMsfUpdate, following is a solution.
- Introduce /cur/LastAddedFile.dat file(or <FolderName>\LastAddedFile.dat file) :
    last_added_file_name
    file_size_of_the_last_added_file
    timestamp_of_the_last_added_file
    total_file_size_in_cur_directory
    update_sequence_number
- Upon adding /cur/nnnnnnnn file,
   Open /cur/LastAddedFile.dat file with Write/Replace/Exclusive mode
   Write /cur/nnnnnnnn file and close
   Write /cur/LastAddedFile.dat
   Close /cur/LastAddedFile.dat
- Upon .msf file update, 
   use FileSize=total_file_size_in_cur_directory, Timestamp=update_sequence_number
  
For clean definition and common definition in BerkleyStore and MaildirStore, following may be better.
  - Introduce msgDBHdr.messageFile in addition to msssageKey/messageOffset.
  - MaildirStore :
    move msgDBHdr->StringProperty->storeToken to msgDBHdr.messageFile
  - BerkleyStore :
    add msgDBHdr.messageFile(same as msgFolder.URI value for independence from OS, or Null) 
By this, mail data of a messageKey can be accessed via messageFile+messageOffset always.
(Reporter)

Comment 48

3 years ago
The 2 blocking patches (including 64-bitness in bug 789679) were landed so you could try to continue here. Unless the solution here is also dependent on bug 1093217.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
(Reporter)

Comment 49

3 years ago
Comment on attachment 8507554 [details] [diff] [review]
Patch v2

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

Please de-bitrot the patch and update it for 64-bit sizeOnDisk now. Then I could finally test it :)

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +89,5 @@
>    /**
> +   * Returns the size of the folder on the disk.
> +   * @param aFolder folder we want to get the size of.
> +   */
> +  unsigned long getFolderSizeOnDisk(in nsIMsgFolder aFolder);

long long (and signed) now.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +421,5 @@
> +    pPrefBranch->GetCharPref("mail.serverDefaultStoreContractID",
> +                             getter_Copies(defaultStore));
> +    if (defaultStore.EqualsLiteral("@mozilla.org/msgstore/berkeleystore;1"))
> +      (void) RefreshSizeOnDisk();
> +  }

I do not like this. You copy this block to many places and it specialcases one store. If we need RefreshSizeOnDisk then it should be pushed down into the stores as GetFolderSizeOnDisk is.

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +1170,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = srcFolder->SetSizeOnDisk(srcFolderSize - messagesSize);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    srcFolder->NotifyIntPropertyChanged(MsgNewAtom("FolderSize").take(), srcFolderSize,
> +                                        srcFolderSize - messagesSize);

I don't like this duplication. I proposed making RefreshSizeOnDisk do exactly this. Was that bad/unworkable?

::: mailnews/news/src/nsNewsFolder.cpp
@@ +774,5 @@
>  
> +    uint32_t folderSize = (uint32_t)(-1); // uninitialized
> +    rv = msgStore->GetFolderSizeOnDisk(this, &folderSize);    
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    mFolderSize = folderSize;

I think I have updated this function in the 64bit sizeondisk patch, please adapt this part.
Attachment #8507554 - Flags: feedback?(acelists) → feedback-
(Assignee)

Comment 50

3 years ago
Created attachment 8529101 [details] [diff] [review]
b1032360-3.patch

This is a work-in-progress of my last attempt to look at this. There is still a lot that I am uncertain of, including the removal of RefreshDisk(), and the need for a Runnable to calculate the folderSize when it is lost.

The tough issue for me is that, according to the overall philosophy of handling folder metadata, we should update the db value of folder size immediately when it is changed. But the conflation of folderSize as a UI element, with its use as a mark of validity for the mork file, means that in many cases folderSize is updated when it makes sense as a mark of db validity. My fear is that if we change it to match the overall philosophy of handling metadata, we may inadvertently introduce regressions that cause the db to be invalidated.

Sorry to leave this hanging like this, Suyash. But with Aceman's bugs finally getting landed we should be able to start working on the together in the next week or so.
(Reporter)

Comment 51

3 years ago
(In reply to Kent James (:rkent) from comment #50)
> Created attachment 8529101 [details] [diff] [review]
> b1032360-3.patch
> 
> This is a work-in-progress of my last attempt to look at this. There is
> still a lot that I am uncertain of, including the removal of RefreshDisk(),

Actually I just wanted to propose this removal. If RefreshSizeOnDisk is not needed in maildir, then remove it from the upper layer (LocalFolder) and make it internal to mboxStore how it invalidates the current size variable and determines when to refetch it from disk.
(Reporter)

Comment 52

3 years ago
Comment on attachment 8529101 [details] [diff] [review]
b1032360-3.patch

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

Interesting, you seem to run the collection of size on disk asynchronously now, also for mbox. Will it still be possible for the caller to get folder->GetSizeOnDisk() and receive the value synchronously (in output argument)? Or you mean he will receive the old value (possibly -1), the call just starts the background process of collecting the size. The size eventually gets updating internally in the folder but the caller is already doing something else. Do I understand it correctly?

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1113,2 @@
>    }
> +  mHaveSizeOnDisk = true;

So what is mHaveSizeOnDisk used for?
It looks like you do not have an up-to-date tree here. The current state of the function should be (notice we check for kSizeUnknown):

NS_IMETHODIMP nsMsgLocalMailFolder::GetSizeOnDisk(int64_t *aSize)
{
  NS_ENSURE_ARG_POINTER(aSize);
  nsresult rv = NS_OK;
  if (mFolderSize == kSizeUnknown)
  {
    nsCOMPtr<nsIFile> file;
    rv = GetFilePath(getter_AddRefs(file));
    NS_ENSURE_SUCCESS(rv, rv);
    // Use a temporary variable so that we keep mFolderSize on kSizeUnknown
    // if GetFileSize() fails.
    int64_t folderSize;
    rv = file->GetFileSize(&folderSize);
    NS_ENSURE_SUCCESS(rv, rv);

    mFolderSize = folderSize;
  }

::: mailnews/local/src/nsPop3Sink.cpp
@@ -310,5 @@
>    }
> -  // note that size on disk has possibly changed.
> -  nsCOMPtr<nsIMsgLocalMailFolder> localFolder = do_QueryInterface(m_folder);
> -  if (localFolder)
> -    (void) localFolder->RefreshSizeOnDisk();

How does the folder determine that the size has changed when we do not notify it here?
(Assignee)

Comment 53

2 years ago
I'm going to be the one working on this.
Assignee: syshagarwal → rkent
Blocks: 1288968
(Reporter)

Comment 54

8 months ago
Anindya, would you be able to use your expertise from bug 856087 to finish off this patch here? The comments mostly contain what needs to be done to the existing patch.
This is needed to be finished before we can seriously expose maildir in TB.

Comment 55

8 months ago
(In reply to :aceman from comment #54)
> Anindya, would you be able to use your expertise from bug 856087 to finish
> off this patch here? The comments mostly contain what needs to be done to
> the existing patch.
> This is needed to be finished before we can seriously expose maildir in TB.

Extremely sorry for the late reply. I was busy with my mid semester exams. I'll go through the comments and start working on it.
You need to log in before you can comment on or make changes to this bug.