1008943 - mozinfo should be cheaper to import

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

On Windows, switching mozprocess to use platform instead of mozinfo reduced the build time by 2 minutes:
https://bugzilla.mozilla.org/show_bug.cgi?id=933558#c24

This is because we were spawning thousands of subprocesses per build, each of which are importing a lot of unnecessary libraries and doing unnecessary computation. A profile showed that importing mozfile (and subsequently urllib2) was taking up the majority of this time.

In general it is good to have imports at the top, but for small utility modules like this where performance is a real concern, I think we should do as little processing at import time as possible. Instead we should lazy load all mozinfo attributes. In order to accomplish this without breaking the API, I think we need to have a class instance disguised as a module. For example, see the accepted answer in:
http://stackoverflow.com/questions/1462986/lazy-module-variables-can-it-be-done

Comment 1

[Ahmed Kachkach [:akachkach]]

It seems like it's mozfile (imported in mozinfo) that does all the heavy imports (tarfile, zipfile, urllib2, ...), which is only used in mozinfo for its "load" function (that uses urllib2, and none of the other libs). So maybe we should make that lazy instead.

We could just put the import statements inside the functions that use them, that should do the trick.

Otherwise if you'd still like to make the other vars lazy, we'd have to do some refactoring because of the way those variables are generated ("dynamically" generated by adding values to globals()).

btw, what do you think about implementing it this way instead?

http://stackoverflow.com/a/880751/1334771

Comment 2

[Ahmed Kachkach [:akachkach]]

Attachment: lazy-mozfile.patch

Lazy imports in mozfile

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8421420 [details] [diff] [review]
lazy-mozfile.patch

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

Thanks! Let's just take this for now since we have it, but I think we should leave the bug open to do lazy loading for the rest of mozinfo.

Though please add a comment to the top of the module explaining why we don't import everything at the top with a reference to this bug number.

Comment 4

[Ahmed Kachkach [:akachkach]]

Attachment: lazy-mozfile.patch

carry forward r+

Comment 5

[Andrew Halberstadt [:ahal]]

We're going to take this for now because it will help, but I think it would still be good to do lazy loading in mozinfo in general. Probably not a very high priority though.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b58b65567d7f

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b58b65567d7f

Comment 8

[Julien Pagès (:parkouss)]

Attachment: mozinfo should be cheaper to import

I have moved some import statements to be lazier.

I ran mozinfo unit tests with success locally.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8507770 [details] [diff] [review]
mozinfo should be cheaper to import

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

Thanks, this looks reasonable to me! Though I'm not sure how much of an improvement lazy loading json, errno and ctypes is since those are system modules. But oh well, doesn't hurt I guess.

Comment 10

[Andrew Halberstadt [:ahal]]

Sigh, ignore my last comment, I'm tired :).. Unconditional r+.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/776061abb9be

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/776061abb9be

Comment 13

[Julien Pagès (:parkouss)]

I'm closing it because I believe there is no need to keep the [leave-open] whiteboard entry no that the work is done. Please tell me If I done something wrong here.