Last Comment Bug 763854 - Check file references (cleanup stored files) only when needed
: Check file references (cleanup stored files) only when needed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jan Varga [:janv]
:
: Hsin-Yi Tsai [:hsinyi]
Mentors:
Depends on:
Blocks: 785884
  Show dependency treegraph
 
Reported: 2012-06-12 01:57 PDT by Jan Varga [:janv]
Modified: 2013-06-12 01:00 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0.1 (39.93 KB, patch)
2012-06-14 10:15 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.2 (40.66 KB, patch)
2012-06-27 04:36 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.3 (42.16 KB, patch)
2012-07-03 03:52 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.4 (42.35 KB, patch)
2012-07-31 05:00 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
interdiff 0.4 - 0.5 (796 bytes, patch)
2012-08-07 06:50 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch 0.6 (43.16 KB, patch)
2012-08-07 22:51 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch 0.6 (43.21 KB, patch)
2012-08-20 08:39 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch 0.6 (43.21 KB, patch)
2012-08-23 12:11 PDT, Jan Varga [:janv]
bent.mozilla: review+
Details | Diff | Splinter Review
patch v0.7 (45.65 KB, patch)
2012-08-24 09:49 PDT, Jan Varga [:janv]
bent.mozilla: review+
Details | Diff | Splinter Review
interdiff v0.6 - v0.7 (15.58 KB, patch)
2012-08-24 09:50 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review

Description Jan Varga [:janv] 2012-06-12 01:57:38 PDT
Currently, during origin initialization, we have to traverse all stored files on disk and check db references for every database, even if we need to open just one of them.

I think I found out how to minimize opening of all databases at least (and if opening is needed, don't check all stored files, just files that were not finished, for example unfinished copy or file that was referenced only from JS and we didn't have a chance to delete it after a GC)
Traversing is still needed to initialize quota.
Comment 1 Jan Varga [:janv] 2012-06-13 11:24:59 PDT
Possible causes of orphaned files:
- a crash during file copy or when a file is only referenced from JS
- a file only referenced from JS won't be removed even during clean shutdown, since release builds won't run GC in near future

Unified quota will likely need to initialize (or at least count file sizes) all origins, that's why we need to improve origin initialization code.
Current implementation also doesn't scale well, imagine 10 000 stored files.

Solution:
Create a journal file (kind of a mark) for stored files that are not (yet) referenced from database in a separate directory. For example, in a subdirectory called "journals".

Existence of a journal file (during origin initialization) in the journals subdirectory means, that we have to check if there are any database references for the stored file.
If there are references -> just delete the journal
If there are no references -> remove stored file and then the journal

We can't optimize this by integrating with SQLite journal, because even when the transaction (in which all db references are removed) is successfully committed we can't remove the stored file if it has references from JS.
Comment 2 Jan Varga [:janv] 2012-06-14 10:15:16 PDT
Created attachment 633175 [details] [diff] [review]
patch v0.1
Comment 3 Jan Varga [:janv] 2012-06-27 04:36:54 PDT
Created attachment 637076 [details] [diff] [review]
patch v0.2

rebased patch
Comment 4 Jan Varga [:janv] 2012-07-03 03:52:43 PDT
Created attachment 638640 [details] [diff] [review]
patch v0.3

Forgot to create journals for newly created file handles.
Comment 5 Jan Varga [:janv] 2012-07-31 05:00:43 PDT
Created attachment 647497 [details] [diff] [review]
patch v0.4

- rebased (nsnull -> nullptr changes)
- removed unused local variable "fileManagers"
Comment 6 Jan Varga [:janv] 2012-08-07 06:50:26 PDT
Created attachment 649631 [details] [diff] [review]
interdiff 0.4 - 0.5
Comment 7 Jan Varga [:janv] 2012-08-07 22:51:44 PDT
Created attachment 649970 [details] [diff] [review]
patch 0.6

Rebased patch (patch for bug 780625 caused many conflicts)
Comment 8 Jan Varga [:janv] 2012-08-20 08:39:26 PDT
Created attachment 653395 [details] [diff] [review]
patch 0.6

rebased patch
Comment 9 Jan Varga [:janv] 2012-08-22 07:18:12 PDT
latest try results:
https://tbpl.mozilla.org/?tree=Try&rev=a266b4b3cdb3
Comment 10 Jan Varga [:janv] 2012-08-23 12:11:38 PDT
Created attachment 654726 [details] [diff] [review]
patch 0.6

- rebase (PR types conversion)
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-23 21:23:53 PDT
Comment on attachment 654726 [details] [diff] [review]
patch 0.6

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

This looks good! Just a few nits and a small change to two methods. r=me with these fixed.

::: dom/indexedDB/FileManager.cpp
@@ +54,5 @@
>  } // anonymous namespace
>  
>  nsresult
>  FileManager::Init(nsIFile* aDirectory,
> +                  mozIStorageConnection* aConnection)

Nit: Assert both params.

@@ +82,5 @@
> +  nsCOMPtr<nsIFile> journalDirectory;
> +  rv = aDirectory->Clone(getter_AddRefs(journalDirectory));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = journalDirectory->Append(NS_LITERAL_STRING("journals"));

Nit: You use "journals" in several places, please make this a #define so that we will only ever have to change this in one spot in the future.

@@ +95,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ENSURE_TRUE(isDirectory, NS_ERROR_FAILURE);
> +  }
> +  else {
> +    rv = journalDirectory->Create(nsIFile::DIRECTORY_TYPE, 0755);

Hm. Why do you need to create this here? Can't you wait until you are actually making a journal entry? It will mean that you do extra work at the next startup (since you wouldn't try to enumerate the directory otherwise).

@@ +237,5 @@
> +// static
> +nsresult
> +FileManager::InitDirectory(mozIStorageServiceQuotaManagement* aService,
> +                           nsIFile* aDirectory, nsIFile* aDatabaseFile,
> +                           FactoryPrivilege aPrivilege)

Nit: One arg per line. And please assert them.

::: dom/indexedDB/IDBDatabase.cpp
@@ +904,5 @@
>  
>    mFileInfo = fileManager->GetNewFileInfo();
>    NS_ENSURE_TRUE(mFileInfo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
> +  int64_t fileId = mFileInfo->Id();

Nit: make this a const ref? No need for a stack var.

@@ +906,5 @@
>    NS_ENSURE_TRUE(mFileInfo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
> +  int64_t fileId = mFileInfo->Id();
> +
> +  nsCOMPtr<nsIFile> directory = fileManager->GetJournalDirectory();

FYI if you do lazy "journals" dir creation then you'll need to create the dir here if it doesn't exist.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +2664,5 @@
>        nsCOMPtr<nsIFile> nativeFile =
> +        fileManager->GetFileForId(journalDirectory, id);
> +      NS_ENSURE_TRUE(nativeFile, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +      rv = nativeFile->Create(nsIFile::NORMAL_FILE_TYPE, 0644);

You'll have to do the dir creation here too.

::: dom/indexedDB/IDBTransaction.cpp
@@ +1059,5 @@
> +  nsresult rv = function.ErrorCode();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = CreateJournals(mJournalsToCreateBeforeCommit,
> +                      mJournalsToRemoveAfterAbort);

So it seems to me that CreateJournals doesn't need to take any args at all. It's a method and has access to all these args.

@@ +1070,5 @@
> +UpdateRefcountFunction::DidCommit()
> +{
> +  mFileInfoEntries.EnumerateRead(FileInfoUpdateCallback, nullptr);
> +
> +  nsresult rv = RemoveJournals(mJournalsToRemoveAfterCommit);

Same here.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +1034,5 @@
>  
>  void
> +IndexedDatabaseManager::AddFileManager(const nsACString& aOrigin,
> +                                       const nsAString& aDatabaseName,
> +                                       FileManager* aFileManager)

Nit: Assert aFileManager

@@ +1040,5 @@
> +  nsTArray<nsRefPtr<FileManager> >* array;
> +  if (!mFileManagers.Get(aOrigin, &array)) {
> +    nsAutoPtr<nsTArray<nsRefPtr<FileManager> > > newArray(
> +      new nsTArray<nsRefPtr<FileManager> >());
> +    mFileManagers.Put(aOrigin, newArray);

Nit: Put() is infallible now so the autoptr is not necessary.

@@ +1885,5 @@
>  
>    return NS_OK;
>  }
>  
> +IndexedDatabaseManager::AsyncDeleteFileRunnable::AsyncDeleteFileRunnable(

Nit:

  IndexedDatabaseManager::
  AsyncDeleteFileRunnable::AsyncDeleteFileRunnable(...)
Comment 12 Jan Varga [:janv] 2012-08-24 09:49:44 PDT
Created attachment 655037 [details] [diff] [review]
patch v0.7

review comments addressed
Comment 13 Jan Varga [:janv] 2012-08-24 09:50:33 PDT
Created attachment 655038 [details] [diff] [review]
interdiff v0.6 - v0.7
Comment 14 Jan Varga [:janv] 2012-08-24 09:52:53 PDT
(In reply to ben turner [:bent] from comment #11)
> ::: dom/indexedDB/IDBTransaction.cpp
> @@ +1059,5 @@
> > +  nsresult rv = function.ErrorCode();
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  rv = CreateJournals(mJournalsToCreateBeforeCommit,
> > +                      mJournalsToRemoveAfterAbort);
> 
> So it seems to me that CreateJournals doesn't need to take any args at all.
> It's a method and has access to all these args.

ok, fixed

> 
> @@ +1070,5 @@
> > +UpdateRefcountFunction::DidCommit()
> > +{
> > +  mFileInfoEntries.EnumerateRead(FileInfoUpdateCallback, nullptr);
> > +
> > +  nsresult rv = RemoveJournals(mJournalsToRemoveAfterCommit);
> 
> Same here.
> 

can't do that here, since we need to go through different arrays in DidCommit() and DidAbort()
Comment 15 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-24 11:46:54 PDT
Comment on attachment 655037 [details] [diff] [review]
patch v0.7

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

Just add an assertion to EnsureJournal that you're not on the main thread. Looks good!
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:01:08 PDT
https://hg.mozilla.org/mozilla-central/rev/1c44596f22cf

Note You need to log in before you can comment on or make changes to this bug.