907049 - Recursively reading moz.build files does utf-8 decoding of config.status content once per file.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/file/bb025b6949e8/python/mozbuild/mozbuild/frontend/reader.py#l171

This is executed for each moz.build file, so more than a thousand times.

With an awfully wrong patch, i can get from reading all moz.build files in 1.5s to 0.7s.

I don't know that code well enough to come up with an actual landable patch, though.

Comment 1

[(not currently active) Ted Mielczarek]

Given that CONFIG is read-only, it seems like it'd be fine to create one copy of it as a member of BuildReader and pass it in to each Sandbox.

Comment 2

[Gregory Szorc [:gps]]

I'm working on this.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Cache unicode representation of substs variables

I took a slightly different approach from caching the dict in the build
reader instance. Instead, we're caching things in the ConfigEnvironment
object itself. This has the unfortunate side-effect that changes now
need to be synchronized between both versions of the dict. Fortunately,
there are only a few instances in the tree and they are all in tests. To
help enforce this, I converted .substs and .substs_unicode to a
read-only dict.

This patches reduces my read time from ~2.0s to ~0.35s. Not too shabby!

$ time ./config.status
Reticulating splines...
Finished reading 1169 moz.build files into 3388 descriptors in 0.36s
Backend executed in 1.06s
2337 total backend files. 0 created; 0 updated; 2345 unchanged
Total wall time: 1.90s; CPU time: 1.90s; Efficiency: 100%

real0m2.007s
user0m1.894s
sys0m0.111s

https://tbpl.mozilla.org/?tree=Try&rev=314ae35293d0

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 793231 [details] [diff] [review]
Cache unicode representation of substs variables

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

::: python/mozbuild/mozbuild/backend/configenvironment.py
@@ +21,5 @@
>  
> +if sys.version_info.major == 2:
> +    text_type = unicode
> +else:
> +    text_type = str

For a followup, might be worth having in some util module.

@@ +148,5 @@
> +                    v = v.decode('utf-8', 'replace')
> +
> +            self.substs_unicode[k] = v
> +
> +        self.substs_unicode = ReadOnlyDict(self.substs_unicode)

is there actually a need for a non unicode substs?

::: python/mozbuild/mozbuild/test/common.py
@@ +30,5 @@
> +        })
> +
> +        self.substs.update(extra_substs)
> +
> +        self.substs_unicode = ReadOnlyDict({k.decode('utf-8', 'replace'): v.decode('utf-8',

i doubt you need 'replace' here.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #4)
> ::: python/mozbuild/mozbuild/backend/configenvironment.py
> @@ +21,5 @@
> >  
> > +if sys.version_info.major == 2:
> > +    text_type = unicode
> > +else:
> > +    text_type = str
> 
> For a followup, might be worth having in some util module.

Yeah, a "pycompat" module would be nice.

> 
> @@ +148,5 @@
> > +                    v = v.decode('utf-8', 'replace')
> > +
> > +            self.substs_unicode[k] = v
> > +
> > +        self.substs_unicode = ReadOnlyDict(self.substs_unicode)
> 
> is there actually a need for a non unicode substs?

Kinda/sorta. I have a feeling using Unicode everywhere will make things break. This has caused pain previously, including during development of bug 784841. I'd rather not tempt fate. This will all get cleaned up in bug 844509.

Comment 6

[Gregory Szorc [:gps]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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