699620 - Unpack places.sqlite from omni.jar when profile is created

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect)

Description

[Vladan Djeric (:vladan)]

Attachment: Code for unpacking the DB when profile is created

To improve first run performance, we will be unpacking pre-created SQLite databases from omni.jar on profile initialization (see bug 691268).

This change extracts places.sqlite from omni.jar when the database doesn't exist. The extracted DB contains the DB schema but no rows. If extraction fails for whatever reason, an error is logged and the DB is created from scratch as before.

Comment 1

[Marco Bonardo [:mak]]

Comment on attachment 571813 [details] [diff] [review]
Code for unpacking the DB when profile is created

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

there's nothing really interesting requiring a deep review here. What should be clarified instead is:
- do we have a tangible win to pay back the additional effort required to pack an up-to-date db at each release?
- do we have evidence the time of the first startup of a profile is something that drives out users?

Regarding places.sqlite, it should have the right user_schema, so we skip all initialization, and ideally it should have bookmarks roots already in place (moz_bookmarks_roots should be populated and moz_bookmarks should have the corresponding entries), since that's an expensive task on first startup. To clarify, I'm really hoping to remove the dumb bookmarks roots concept in future.

::: toolkit/components/places/Database.cpp
@@ +415,5 @@
>      // Be sure to clear the pref to avoid handling it more than once.
>      (void)Preferences::ClearUser(PREF_FORCE_DATABASE_REPLACEMENT);
>  
>      return NS_ERROR_FILE_CORRUPTED;
>    }

at this point is probably safer to do
if (forceInit) {
  clearUser
  if (databaseFileExists)
    return NS_ERROR...
}

if (!databaseFileExists)
  your_omnijar_code

@@ +422,5 @@
> +  if (!databaseFileExists && !forceInit) {
> +    nsZipArchive *omnijar =
> +      mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
> +    if (omnijar)
> +      rv = omnijar->Extract(NS_LITERAL_CSTRING(PLACES_JAR_PATH), databaseFile);

you are not checking rv, and it may even be fine since if we fail to extract the db will be created the usual way...
then I suggest to put a MOZ_ASSERT(NS_SUCCEEDED(rv)); here so we care only in debug mode.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Vladan, it is my understanding that we decided to pick another strategy. Should this bug be closed?

Comment 3

[Vladan Djeric (:vladan)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=699615#c15