Last Comment Bug 684152 - Don't even bother trying to unpackComponents if the APK hasn't changed
: Don't even bother trying to unpackComponents if the APK hasn't changed
: mobile, perf
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla8
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
: Jim Chen [:jchen] [:darchons]
Depends on:
  Show dependency treegraph
Reported: 2011-09-01 20:36 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2011-09-09 15:49 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (1.57 KB, patch)
2011-09-01 20:36 PDT, Mark Finkle (:mfinkle) (use needinfo?)
doug.turner: review-
Details | Diff | Splinter Review
patch 2 (2.33 KB, patch)
2011-09-01 21:34 PDT, Mark Finkle (:mfinkle) (use needinfo?)
doug.turner: review+
christian: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-09-01 20:36:18 PDT
Created attachment 557759 [details] [diff] [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.
Comment 1 Doug Turner (:dougt) 2011-09-01 21:18:54 PDT
Comment on attachment 557759 [details] [diff] [review]

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

looks good, small tweaks needed.

::: embedding/android/
@@ +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)
Comment 2 Doug Turner (:dougt) 2011-09-01 21:31:21 PDT
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.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-01 21:34:17 PDT
Created attachment 557766 [details] [diff] [review]
patch 2

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.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-02 06:15:40 PDT
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-06 14:27:28 PDT
Comment on attachment 557766 [details] [diff] [review]
patch 2

Too much for Beta. Will try for Aurora.
Comment 6 christian 2011-09-08 14:39:33 PDT
Comment on attachment 557766 [details] [diff] [review]
patch 2

Approved for releases/mozilla-aurora. Please land asap.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-09 12:34:37 PDT

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