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

RESOLVED WONTFIX

Status

()

Toolkit
Places
RESOLVED WONTFIX
6 years ago
7 months ago

People

(Reporter: vladan, Unassigned)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 571813 [details] [diff] [review]
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.
(Reporter)

Updated

6 years ago
Attachment #571813 - Attachment is patch: true
(Reporter)

Updated

6 years ago
Assignee: nobody → vdjeric
Blocks: 691268
Keywords: perf
(Reporter)

Updated

6 years ago
Attachment #571813 - Flags: review?(mak77)
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.
Attachment #571813 - Flags: review?(mak77) → review+
Vladan, it is my understanding that we decided to pick another strategy. Should this bug be closed?
Flags: needinfo?(vdjeric)
(Reporter)

Comment 3

4 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=699615#c15
Flags: needinfo?(vdjeric)

Updated

2 years ago
Assignee: vladan.bugzilla → nobody

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.