943082 - Follow-up: more efficient loading of favicons from JAR

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Richard Newman [:rnewman]]

See TODO in Part 2.

Comment 1

[Ed Morley [:emorley]]

(By request in #developers :-))

Comment 2

[Richard Newman [:rnewman]]

Neil, do you have what you need to start with this?

Comment 3

[Neil Bleasdale [:nbleasdale]]

I believe so yup. I assume it'll just be a case of comparing the bit after the 'about:' in the for loop for each uri rather than the whole lot.

Comment 4

[Richard Newman [:rnewman]]

You'll need to extend GeckoJarReader to support fetching multiple bitmaps at once, with the goal being to avoid rebuilding streams and rewalking the nested jars. Then the favicon code can hit the jar once, rather than having each of these calls:

        // Load and cache the built-in favicon in each of its sizes.
        // TODO: don't open the zip twice!
        ArrayList<Bitmap> toInsert = new ArrayList<Bitmap>(2);
        toInsert.add(loadBrandingBitmap(context, "favicon64.png"));
        toInsert.add(loadBrandingBitmap(context, "favicon32.png"));

The goal is that it could say something like this instead:

        ArrayList<Bitmap> toInsert = loadBrandingBitmaps("favicon64.png", "favicon32.png");

Comment 5

[Neil Bleasdale [:nbleasdale]]

Hah, then I probably did misunderstand the original intent slightly. No problem, from what you've said above that'll give me enough to get started putting something together. Thanks.

Comment 6

[Neil Bleasdale [:nbleasdale]]

Hi. Thought I should probably give a quick update as it's been a while. After a busy couple of weeks at work I've finally had time to sit and crack on with this. 

I've followed your suggestions and am making progress. The only thing slowing me down significantly is my inability to see the value of local variables when stepping through the code in a debugger (using Intellij at present). 

Following a suggestion by someone in #mobile I tried changing the JAVAC_FLAGS to be '-g:source, lines, vars \' in android-common.mk, but this breaks the build for me with it complaining about some failing tests IIRC (I'll double check that and post it here). 

So without proper debugging I'm relying on liberal use of 'log.d's everywhere. I'm getting there.

Comment 7

[Neil Bleasdale [:nbleasdale]]

Sorry, it doesn't fail tests building with that edited android-common.mk it has the error: Class names, 'vars', are only accepted if annotation processing is explicitly requested.

So not sure what I need to add where to get this up and running with this changed build file...

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

For debug symbols, have you tried adding `ac_add_options --enable-debug` to your mozconfig [1]?

I've gotten debugging working before via jimdb [2] using jdb and the flag above, however, jdb is terrible so I haven't stress tested it and generally rely on println debugging. :\

[1]: https://developer.mozilla.org/en-US/docs/Configuring_Build_Options#Optimization
[2]: https://wiki.mozilla.org/Mobile/Fennec/Android/GDB

Comment 9

[Mike Hoye [:mhoye]]

Hi, Neil - Are you still working on this?

Comment 10

[Neil Bleasdale [:nbleasdale]]

Hi, sorry for the complete silence on this. I've been struggling to find the time to sit down and work on this on top of work/uni at the mo so realistically it's probably best that I make it available for someone else to take hold of.

Comment 11

[Mike Hoye [:mhoye]]

Thanks, Niel - if it's still in the pool when you're out from under work and school, we'd love to see you pick it up again.

Comment 12

[Jakub Miara]

Attachment: brandingLoad.patch

A proposal of patch for this bug. This is work in progress, needs some polishing and added robocop tests. I just wanted to consult the general direction.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8436552 [details] [diff] [review]
brandingLoad.patch

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

Before doing all of this work, I think it's wise to get a rudimentary measurement of (a) whether this is worth fixing, and (b) whether the fix is any faster! How long does it take to load each icon from the JAR? If we're talking about 1msec for each on a mid-range device, then our maximum saving is probably 1msec. If it's 10msec (or 50!), then maybe that's worth doing, if it doesn't add complexity.

The point about complexity is very real: if you add a new class and three new methods, we make our application larger, use more memory at runtime, etc. etc., and that can actually outweigh the benefits.

Anyway, assuming that there's something to gain here...

Your approach seems to be:

* Introduce an explicit representation of nested JARs.
* Cut out the zip handling code to get the stream for the innermost JAR.
* Seek inside the innermost JAR for each filename we want, collecting into an array that we return.
* Wrap this in the usual boilerplate for passing in lists of filenames so we know what to seek for.


Areas in which you might fall down:

* Seeking inside the zip is probably expensive. It's quite possible that seeking in this way will produce an outcome that is no faster (or actually slower!) than what we have now -- the only thing it saves is to not open the outermost zip twice, and we don't have any evidence that that's the big cost.

* You've introduced a bunch of arglist work, where variadic args are turned into an array, passed to another variadic method, then turned into an ArrayList. No point in creating that garbage and doing that work -- let's just take an ArrayList up front! We aren't building a generic zip accessor here.

* You might be able to do less work and generate less garbage by strategically leaking abstractions. For example: what if you assume that we're only loading multiple files from the same directory, or from the same innermost JAR? Both of those should always be true.

In that case, can we return something like the NativeZip from getInnermostZip, and simply have the caller drag bitmaps out of it and add them to the cache? In pseudocode:

  let jar = GeckoJarReader.getInnerJar("!!@#@#@#@@#some/jar/path");
  FaviconCache.addAll(jar.getBitmapsInDirectory("/icons/"))

or

  for (entry in jar.directory("/icons/")) {
    if (entry.name.startsWith("favicon")) {
      FaviconCache.addFromStream(entry.stream);
    }
  }

::: mobile/android/base/favicons/Favicons.java
@@ +415,4 @@
>       * "jar:jar:file:///data/app/org.mozilla.firefox-1.apk!/assets/omni.ja!/chrome/chrome/content/branding/favicon64.png"
>       */
> +    private static String getBrandingBitmapPath(String apkPath, String name) {        
> +        return brandingBitmapsPath + name;

You don't use apkPath in this method...

::: mobile/android/base/util/GeckoJarReader.java
@@ +44,5 @@
> +        }
> +        try{
> +            JarSourceStack sourceStack = JarSourceStack.fromUrl(sourceUrl);
> +            
> +            List<Bitmap> bitmaps = new ArrayList<Bitmap>(filenames.length);

The containers of length N are breeding!

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

This doesn't sound like a good first(!) bug.

Comment 15

[B. Dahse]

It's my understanding that we still handle favicons ourselves and will continue to do so? If that's correct and if this is still worth optimizing, I'd like to work on it :)

Comment 16

[B. Dahse]

Sorry, forgot to NI you in comment 15.

Comment 17

[B. Dahse]

I'm still interested in doing this, but I never NI'ed you so I can get a clear answer on if this is still needed. Assuming it is, is the goal still to be able to rewrite the code as suggested by Richard in comment 4?

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

I don't think this is applicable anymore. The icon code has gone through some refactorings and nowadays we do not load all the icons from the omnijar upfront - but instead load them lazily when needed and keep them in memory as long as needed.