Closed
Bug 699620
Opened 13 years ago
Closed 7 years ago
Unpack places.sqlite from omni.jar when profile is created
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: vladan, Unassigned)
References
Details
(Keywords: perf)
Attachments
(1 file)
3.04 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Attachment #571813 -
Attachment is patch: true
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Updated•13 years ago
|
Attachment #571813 -
Flags: review?(mak77)
Comment 1•13 years ago
|
||
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+
Comment 2•11 years ago
|
||
Vladan, it is my understanding that we decided to pick another strategy. Should this bug be closed?
Flags: needinfo?(vdjeric)
Reporter | ||
Comment 3•11 years ago
|
||
See https://bugzilla.mozilla.org/show_bug.cgi?id=699615#c15
Flags: needinfo?(vdjeric)
Updated•8 years ago
|
Assignee: vladan.bugzilla → nobody
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•