Closed
Bug 1065484
Opened 10 years ago
Closed 8 years ago
Distribution.copyFiles takes 500msec on Nexus S first run
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: rnewman, Unassigned, Mentored)
References
Details
(Keywords: perf, Whiteboard: [good next bug][lang=java])
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/octet-stream
|
Details |
Reporter | ||
Comment 1•10 years ago
|
||
This can basically be summed up as "java.util.zip.ZipFile is really inefficient".
Almost all of this CPU time is in ZipFile.readCentralDir.
We can't get away without doing this work, so we should look into a more efficient way to read the APK.
Reporter | ||
Comment 2•10 years ago
|
||
We might consider trying mozglue's NativeZip here.
Mentor: rnewman
Whiteboard: [good second bug][lang=java]
Comment 3•10 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #2)
> We might consider trying mozglue's NativeZip here.
+1
Comment 4•10 years ago
|
||
Whiteboard: [good second bug][lang=java] → [good next bug][lang=java]
Comment 5•9 years ago
|
||
I've added an extract method to NativeZip and the native Zip implementation, returning the number of files extracted.
In my tests (not on a Nexus S though) the time consumption dropped to about 6%.
Attachment #8669090 -
Flags: feedback?(rnewman)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → tynn.dev
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8669090 [details] [diff] [review]
bug1065484.patch
Review of attachment 8669090 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not qualified to review the Zip changes, which is where the meat lies. Back to me or mcomella when that lower-level stuff is considered good.
Attachment #8669090 -
Flags: feedback?(rnewman) → feedback?(mh+mozilla)
Comment 7•9 years ago
|
||
Comment on attachment 8669090 [details] [diff] [review]
bug1065484.patch
Review of attachment 8669090 [details] [diff] [review]:
-----------------------------------------------------------------
Considering you're not streaming the decompression, is there a reason you don't do it without adding C code at all, through getInputStream?
Updated•9 years ago
|
Attachment #8669090 -
Flags: feedback?(mh+mozilla)
Comment 8•9 years ago
|
||
NativeZip doesn't offer anything else than getInputStream. So adding C code seems necessary to me.
To extract files from the zip file we need to read the central dir on the C level. My implementation was just one idea I had for doing this. But also the one least intrusive.
If we don't extract the files directly, we might need to expose the pointers to entries and methods for their handling to the Java level. At least we'd need to have access to the names of the entries to use getInputStream. But Distibution.copyFiles also uses the modified time.
What would you suggest to do?
Flags: needinfo?(mh+mozilla)
Comment 9•9 years ago
|
||
I'm not following. What are you trying to do that can't be achieved with getInputStream and making java code write out the content?
Flags: needinfo?(mh+mozilla)
Comment 10•9 years ago
|
||
Listing the files of the archive.
Comment 11•9 years ago
|
||
Hi Mike,
I had a look into the Zip class again for extracting a directory structure from a zip file.
I found the only public method relating to data extraction is
Zip::GetStream(const char *, Stream *) const
populating a Stream object with methods
Zip::Stream::GetBuffer()
Zip::Stream::GetSize()
Zip::Stream::GetUncompressedSize()
Zip::Stream::GetType()
Zip::Stream::GetZStream(void *)
This only is useful if all names are known and doesn't give us the modification time. But to read the central directory we would need access to
Zip::DirectoryEntry
which is a private struct.
I could think of two possible ways to implement the given task:
1. creating Zip::Extract to handle everything in C
2. exposing Zip::DirectoryEntry or at least parts of it and using NativeZip.getInputStream
I couldn't think of any method without adding C code. Please tell me if I'm missing something here.
Flags: needinfo?(mh+mozilla)
Comment 12•9 years ago
|
||
You're still not saying what you're trying to do. What do you need modification time for?
Flags: needinfo?(mh+mozilla)
Comment 13•9 years ago
|
||
The method Distribution.copyFiles copies the distribution/ directory of the APK zip file to the device.
For this it uses the ZipFile and ZipEntry classes, which I want to replaced with NativeZip.
The given patch just replaces the whole Distribution.copyFiles method with a NativeZip.extract method implemented in Zip::Extract. This was the easiest and least intrusive implementation, mainly to see if it's more efficient; the time consumption dropped to about 6%.
To reuse the current implementation NativeZip.getInputStream is a good replacement for ZipFile.getInputStream, while there is no replacement for the ZipEntry class and the ZipFile.entries, ZipEntry.getName and ZipEntry.getTime methods within NativeZip or the C part of mozglue.
So the part of getting the stream contents itself by using NativeZip.getInputStream is not the issue here. But I need to know the filename to use this method. Since the contents of the distribution/ directory vary, I need to get at least all entry names from the zipfile. But this is not possible at the moment and I would like to implement this part in the context of this bug or a new one if that's better.
Flags: needinfo?(mh+mozilla)
Comment 14•9 years ago
|
||
So, you are not so much interested in modification times, you want file names. Add a function that iterates through them?
Flags: needinfo?(mh+mozilla)
Comment 15•9 years ago
|
||
Yes, the file names are the essential information we need to get. And I think we don't really need the modification times, even though these are used currently. How should I tackle this task? What am I allowed to change?
Flags: needinfo?(mh+mozilla)
Comment 16•9 years ago
|
||
You should be able to do what you want with some minimal JNI, and without touching Zip.cpp/.h, and leave the string manipulations to java code.
Flags: needinfo?(mh+mozilla)
Comment 17•9 years ago
|
||
Zip only provides an interface to load named entries.
So I'm not able to do anything here without touching Zip.cpp/.h.
How should I proceed?
Flags: needinfo?(rnewman)
Comment 18•9 years ago
|
||
(In reply to Christian Schmitz (:tynn) from comment #17)
> Zip only provides an interface to load named entries.
> So I'm not able to do anything here without touching Zip.cpp/.h.
> How should I proceed?
Zip::GetFirstEntry will get you a DirectoryEntry and DirectoryEntry::GetNext allows to enumerate them all. I don't see what's missing to do what you want to do.
Comment 19•9 years ago
|
||
All of these are private. Only Zip::GetStream is public.
Comment 20•9 years ago
|
||
(In reply to Christian Schmitz (:tynn) from comment #19)
> All of these are private. Only Zip::GetStream is public.
And because a method is private you want to add other methods? I'm not following. Just add a "public:" where you need it.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(rnewman)
Comment 21•9 years ago
|
||
I haven't found a good implementation to do this. It would end up having the same issues like ZipFile. I'd still like to have an extraction method implemented on C level. This would make a difference.
Updated•9 years ago
|
Assignee: tynn.dev → nobody
Status: ASSIGNED → NEW
Comment 22•8 years ago
|
||
If this is still relevant, I'd like to work on it.
Reading the discussion between Christian and Mike has left me uncertain if Christian had the right idea and I might as well continue his work? Would the NativeZip solution even still be preferred or have things changed since then?
Flags: needinfo?(rnewman)
Reporter | ||
Comment 23•8 years ago
|
||
Over to someone who still works on this full-time :)
Mentor: rnewman → s.kaspari
Flags: needinfo?(rnewman) → needinfo?(s.kaspari)
Comment 24•8 years ago
|
||
(In reply to B. Dahse from comment #22)
> If this is still relevant, I'd like to work on it.
>
> Reading the discussion between Christian and Mike has left me uncertain if
> Christian had the right idea and I might as well continue his work? Would
> the NativeZip solution even still be preferred or have things changed since
> then?
Good question. Considering that this bug is already 2 years old and a Nexus S is something you only see rarely nowadays: How do we perform on recent devices and/or Android versions? The 6% performance gain mentioned in comment 6 doesn't sound much either. Would you be interested in tracing this to see whether this is even an issue worth looking into? If there is a bottleneck and we can significantly improve this by using NativeZip (because we already ship it) then this is something worth doing.
You might need to create a distribution[1] or download/modify the sample[2] in order to run this code. An "APK distribution" is probably the easiest way to test this:
https://wiki.mozilla.org/Mobile/Distribution_Files#Testing_a_distribution_locally
[1] https://wiki.mozilla.org/Mobile/Distribution_Files
[2] https://github.com/mozilla/fennec-distribution-sample
Flags: needinfo?(s.kaspari)
Comment 25•8 years ago
|
||
Alright thanks, I'll look into profiling a distribution. I guess you meant to link to comment 5 instead, but what's important is: the way I read it, the performance gain was actually about 94% ?
Still, my Android device is kind of old. It's a Sony Z3C (released almost exactly two years ago) currently with Cyanogenmod and Android 5.1.1
Will it be okay to test with it?
Flags: needinfo?(s.kaspari)
Comment 26•8 years ago
|
||
(In reply to B. Dahse from comment #25)
> Alright thanks, I'll look into profiling a distribution. I guess you meant
> to link to comment 5 instead, but what's important is: the way I read it,
> the performance gain was actually about 94% ?
Oh, you are right, sorry, I scanned the comments in this bug too quickly.
> Still, my Android device is kind of old. It's a Sony Z3C (released almost
> exactly two years ago) currently with Cyanogenmod and Android 5.1.1
> Will it be okay to test with it?
In general this sounds good. I assume that this part of the code was slow in general and this bug wasn't only about the Nexus S.
Flags: needinfo?(s.kaspari)
Comment 27•8 years ago
|
||
I've tested this sample distribution[1] with the device mentioned in comment 25. The attachment is a trace of copyFilesFromPackagedAssets[2] and it shows that 74.8% of the time in the thread that unpacks the distribution files is spent in ZipFile.readCentralDir or methods called by ZipFile.readCentralDir. I also measured the time difference between entering and leaving [2] with elapsedRealtime[3] and that came out to around 33 milliseconds.
Sebastian, do you think we need to test further with heavier distributions or is the sample distribution a realistic size?
[1]: https://github.com/mozilla/fennec-distribution-sample
[2]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java#738
[3]: https://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtime()
Flags: needinfo?(s.kaspari)
Comment 28•8 years ago
|
||
(In reply to B. Dahse from comment #27)
> Sebastian, do you think we need to test further with heavier distributions
> or is the sample distribution a realistic size?
The size is realistic. It contains some preferences, bookmarks, suggested sites and an extension. Some distributions might contain translations for additional locales though.
What device did you use when measuring the 33ms?
Flags: needinfo?(s.kaspari)
Comment 29•8 years ago
|
||
A Sony Z3C with Cyanogenmod and Android 5.1.1. The trace file indicates more than 33ms inclusive time but of course the tracing slows it down, so I don't know which numbers can be trusted. Do you have the chance to test as well?
Comment 31•8 years ago
|
||
I created a build with the sample distribution and did 5 fresh starts on a bunch of devices, measuring copyFilesFromPackagedAssets():
* Google Pixel XL: 85.4ms [69ms, 83ms, 109ms, 85ms, 81ms]
* Motorola Moto E2: 53.4ms [71ms, 70ms, 47ms, 52ms, 27ms]
* Sony Z3C: 38ms [25ms, 67ms, 26ms, 32ms, 40ms]
* Nexus 6P: 65.8ms [65ms, 74ms, 65ms, 65ms, 60ms]
* Nexus 9: 141.2ms [132ms, 153ms, 180ms, 125ms, 116ms]
* Nexus 7: 51.4ms [40ms, 56ms, 60ms, 58ms, 43ms]
All of them significantly faster than 500ms. It's surprising that the newer devices (Pixel XL and 6P) are not necessarily the faster ones.
Flags: needinfo?(s.kaspari)
Comment 32•8 years ago
|
||
Thanks for doing more extensive testing, Sebastian.
It is surprising that the Z3C seems to be the fastest of the pack because as you say, it's definitely not the newest!
Given these results, do you think there is much room for improvement? I would say it's not worth it, if the prospect is adding more native code. What's your take on it?
Comment 33•8 years ago
|
||
(In reply to B. Dahse from comment #32)
> Given these results, do you think there is much room for improvement? I
> would say it's not worth it, if the prospect is adding more native code.
> What's your take on it?
I agree. Ignoring the Nexus 9 all devices have been faster than 100ms and, even though the results are not directly saying that, it is likely that newer devices will be even faster going forward. In addition to that future distributions are going to be (pre-)installed on newer devices and are less often distributed to old, existing devices.
(In reply to B. Dahse from comment #32)
> It is surprising that the Z3C seems to be the fastest of the pack because as
> you say, it's definitely not the newest!
This is indeed surprising. The only difference is that I recently made a factory reset and there's not much installed on the device.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•