986683 - Represent the Google logo on the about:home page more efficiently

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Andrew McCreight [:mccr8]]

Right now, due in part to the JS engine not parsing strings very well, we're ending up with a huge number of copies of this logo.  Plus data URIs are not a very efficient way to store things.  It would be nice if it could be stored more efficiently, say in a binary format.

Comment 1

[Marco Bonardo [:mak]]

How do we read a binary file on the filesystem from a content page?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Assuming we fix the JS engine to parse strings better, how much of a memory-use win would fixing this be? I'm skeptical that it's worth the trouble.

Comment 3

[Nicholas Nethercote [inactive]]

base64 encoding is an inefficient way to store images.

Comment 4

[Andrew McCreight [:mccr8]]

The full string is 9258 characters long, and each character takes a couple of bytes to represent in the JS engine, IIRC, so that's around 18kb, which isn't too terrible. We also should figure out why there are two copies of the full string, as it would be bad if the number of copies of it is unbounded.

Anyways, I agree that it may not be worth the hassle of encoding it in a binary format, which is why I left this fairly open-ended.  The "right" thing to do in the short term might even just be to make this giant string into a single line in aboutHome.js.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Inefficiency isn't enough to outweigh convenience - needs to be inefficient and significant :)

Comment 6

[Andrew McCreight [:mccr8]]

I'm seeing where we end up if I convert this into a single multiline string.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Use a single multiline string for the search engine logo to avoid startup memory bloat

I used njn's suggestion in bug 986628 comment 13 and turned this into a single multiline string.  If my logging can be believed, this gets rid of every copy of the string from memory (?!?).

Comment 8

[Marco Bonardo [:mak]]

why not also on the "data:image/png;base64," line?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8395062 [details] [diff] [review]
Use a single multiline string for the search engine logo to avoid startup memory bloat

this is a no-brainer if it helps (probably even if it doesn't!)

(as mak says, first line can use the same treatment)

Comment 10

[Andrew McCreight [:mccr8]]

Oops, sorry, there are zero copies of this because I bungled this somehow and it produces an error and doesn't show the image. :(
  JavaScript error: chrome://browser/content/abouthome/aboutHome.js, line 10: illegal character

Comment 11

[Nicholas Nethercote [inactive]]

In Gaia there was a period where seemingly every binary asset (pictures, ringtones, etc.) was stored as a base64 data URI, and it was terrible. So forgive me if I'm leery about the practice! Hopefully it's not being used for larger assets elsewhere.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8395062 [details] [diff] [review]
Use a single multiline string for the search engine logo to avoid startup memory bloat

*ahem*

Comment 13

[Marco Bonardo [:mak]]

what if we make an about:defaultsearchenginelogo redirect pointing to a binary image? with URI_SAFE_FOR_UNTRUSTED_CONTENT may work

Comment 14

[:Gavin Sharp [email: [e-mail]]]

That'd have to be web-exposed, which isn't ideal (though maybe not a big deal in practice).

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Use a single multiline string for the search engine logo to avoid startup memory bloat

Fixed version.  I guess that explains why the highlighting in Emacs was all weird!

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Load chrome image

So, here's an alternate approach.  It turns out we can load chrome URI images from content (!?!?), so we could actually just load this as a file.  Here's a proof of concept that uses a chrome URI image I found somewhere, and I can confirm that it works.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Should depend on whether the package is contentaccessible, but yeah that should work.

Comment 18

[Marco Bonardo [:mak]]

I think at a certain point we also had an about:home mockup that was just using the search engine logo (the same we use in the search bar) just with higher DPIs... so we could also go further, pass the icon from the search service along with the engine, so we have icons for any engine.
Worth involving UX and see if we can stop having a large logo just for google? (actually, that may also require marketing, I think there was an agreement with google about the use of the current logo).

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: Use a single multiline string for the search engine logo to avoid startup memory bloat.

After some minor hijinx where I set the image source to the string "Google", I tested this and it properly displays the Google logo.  In addition, we end up with exactly one copy of this string in memory, in a GC log taken before the first startup GC, as expected.

I added a little note explaining the oddness, so nobody decides it is ugly and undoes it.

Comment 20

[Nicholas Nethercote [inactive]]

I instrumented the ConcatString() call in frontend/FoldConstants.cpp, printing out the size of the resulting string. If I sum all the lengths before applying this patch, I get 1,727,384 bytes (and that's just doing length()*sizeof(jschar) -- I haven't accounted for any slop bytes). After applying the patch, it drops to 266,702 bytes. Good!

The output shows me that there are still some other strings like this one, albeit smaller. E.g. there's one that spans 40 lines. I'll try to hunt them down.

Comment 21

[Nicholas Nethercote [inactive]]

> The output shows me that there are still some other strings like this one,
> albeit smaller. E.g. there's one that spans 40 lines. I'll try to hunt them
> down.

I added some instrumentation to give me JS stacks where these are occurring, and a lot of these seem to be coming from within Cu.import(), CC[].getService() and Cu.evalInSandbox() calls. gavin, how are they implemented?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I'm not sure what you're asking, or what you mean exactly by "from within Cu.import() calls".

Comment 23

[Nicholas Nethercote [inactive]]

I added some hacky instrumentation to SpiderMonkey to get the JS stack each time the concatenation occurs. In most cases it pointed to a line containing a Cu.import() call. For example, line 226 of XPCOMUtils.jsm was mentioned frequently, and it looks like this:

      Cu.import(aResource, temp);

I'm not sure how to interpret this -- maybe the instrumentation is bogus, or maybe the implementation of Cu.import() is triggering the concatenations somehow. So I'd like to look "inside" Cu.import(), if that makes sense.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

It sounds like maybe the instrumentation is telling you that a script that is being import()ed is what contains the long string?

Cu.import is implemented in mostly mozJSComponentLoader::ImportInto (via mozJSComponentLoader::Import).

Comment 25

[Nicholas Nethercote [inactive]]

> Cu.import is implemented in mostly mozJSComponentLoader::ImportInto (via
> mozJSComponentLoader::Import).

Mmm, that appears to be what's happening. Thanks.

Comment 26

[Andrew McCreight [:mccr8]]

This is probably one of the causes of the increase in pre-settled startup memory in the last year or so.  Bug 818940, which landed in 23, increased the size of this image from about 60 lines to about 120 lines.  I could calculate how much of a memory increase it is using a sum or something but I am lazy.

try run: https://tbpl.mozilla.org/?tree=Try&rev=16d6e497ede6

Comment 27

[Andrew McCreight [:mccr8]]

This is kind of initially a regression from bug 627301, which landed in Firefox 4.  Before that, the data URI was represented as a single string.

Comment 28

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3d8044e709

I guess this should technically land on the Fx inbound or whatever but this file seems to rarely change so hopefully it will be okay...

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f3d8044e709