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.

We might consider trying mozglue's NativeZip here.

(In reply to Richard Newman [:rnewman] from comment #2) > We might consider trying mozglue's NativeZip here. +1

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%.

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.

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?

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?

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?

Listing the files of the archive.

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.

You're still not saying what you're trying to do. What do you need modification time for?

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.

So, you are not so much interested in modification times, you want file names. Add a function that iterates through them?

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?

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.

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?

(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.

All of these are private. Only Zip::GetStream is public.

(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.

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.

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?

Over to someone who still works on this full-time :)

(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

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?

(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.

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()

(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?

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?

Yeah, setting NI to myself to remember.

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.

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?

(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.