Closed Bug 684152 Opened 13 years ago Closed 13 years ago

Don't even bother trying to unpackComponents if the APK hasn't changed

Categories

(Core Graveyard :: Widget: Android, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox8 fixed, firefox9 fixed)

RESOLVED FIXED
mozilla8
Tracking Status
firefox8 --- fixed
firefox9 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

(Keywords: mobile, perf, Whiteboard: [mobilestartupshrink][ts][qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
During the Android startup, we attempt to unpack files from the APK. The current code does tests on the files (date and size) to make sure we only write out files when they are new.

Still, all the checks add up and on devices with slow file systems, all the .exists() and .lastModified() and .length() checks can take up a non-trivial amount of time.

From my tests on the nexus one, galaxy tab, and a Try server run on the Tegras, it's around 200ms.

This patch sets the last modified timestamp to the APK last modified timestamp when creating the "components" folder. If the folder exists and the timestamps are the same, we just leave.
Attachment #557759 - Flags: review?(doug.turner)
Comment on attachment 557759 [details] [diff] [review]
Patch

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

looks good, small tweaks needed.

::: embedding/android/GeckoApp.java
@@ +461,5 @@
>          throws IOException, FileNotFoundException
>      {
> +        File applicationPackage = new File(getApplication().getPackageResourcePath());
> +        File componentsDir = new File(sGREDir, "components");
> +        if (componentsDir.exists() && componentsDir.lastModified() == applicationPackage.lastModified())

You don't need the first call to .exists().

If componentsDir doesn't exists, then the call to lastModified() will return zero and that will not match the applicationPackage.lastModified().  so, just:

if (componentsDir.lastModified() == applicationPackage.lastModified())

@@ +468,2 @@
>          componentsDir.mkdir();
> +        componentsDir.setLastModified(applicationPackage.lastModified());

If this line fails, which could happen if utime fails, you won't unpack.  Please test for failure.  (it returns a bool.  false == failure)
Attachment #557759 - Flags: review?(doug.turner) → review-
ignore the comment about the change I wanted against setLastModified.  If the system doesn't support utime for some reason, worse case, we will unpack everytime.
Attached patch patch 2Splinter Review
Makes the first change, but skips the second. The worst thing that should happen from a failed setLastModified call is that we unpack the next time around.
Assignee: nobody → mark.finkle
Attachment #557759 - Attachment is obsolete: true
Attachment #557766 - Flags: review?(doug.turner)
Keywords: mobile, perf
OS: Linux → Android
Hardware: x86 → ARM
Whiteboard: [mobilestartupshrink][ts]
Version: unspecified → Trunk
Attachment #557766 - Flags: review?(doug.turner) → review+
Attachment #557766 - Flags: approval-mozilla-beta?
Attachment #557766 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/ad61ee019ba8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 557766 [details] [diff] [review]
patch 2

Too much for Beta. Will try for Aurora.
Attachment #557766 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 557766 [details] [diff] [review]
patch 2

Approved for releases/mozilla-aurora. Please land asap.
Attachment #557766 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [mobilestartupshrink][ts] → [mobilestartupshrink][ts][qa-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: