Closed Bug 1257049 Opened 8 years ago Closed 8 years ago

Stop spawning a separate process for config.status from configure.py

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1257049 - Stop spawning a separate process for config.status from configure.py.
58 bytes, text/x-review-board-request
gps
: review+
Details 
      No description provided.
Attachment #8731016 - Flags: review?(gps) → review+
Comment on attachment 8731016 [details]
Bug 1257049 - Stop spawning a separate process for config.status from configure.py.

https://reviewboard.mozilla.org/r/40313/#review36825
It turns out there are two issues:
- The one that broke automation, that is somehow caused by the os.environ assignment in configure.py, where replacing the unicode literals with str literals fixes it
- One that wouldn't show up on automation, but would break e.g. japanese builds because all the strings from config.status, when executing it, are str, while they are unicode when just calling config_status(), and that causes problems with the japanese CL_INCLUDES_PREFIX. Presumably, this would happen as well if we added "from __future__ import unicode_literals" in config.status.
Blocks: pyconfigure-infra
No longer blocks: pyconfigure
(In reply to Mike Hommey [:glandium] from comment #5)
> - One that wouldn't show up on automation, but would break e.g. japanese
> builds because all the strings from config.status, when executing it, are
> str, while they are unicode when just calling config_status(), and that
> causes problems with the japanese CL_INCLUDES_PREFIX. Presumably, this would
> happen as well if we added "from __future__ import unicode_literals" in
> config.status.

Could we do something smarter here and make values that originate in old-configure be byte literals (b'foo')? Values originating from moz.configure can be whatever type they're actually set as.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Could we do something smarter here and make values that originate in
> old-configure be byte literals (b'foo')? Values originating from
> moz.configure can be whatever type they're actually set as.

I looked at that today. That would work, but we can't dump those in config.status through json. We'd need our own serialization (it's nice to be able to _read_ config.status, pickle wouldn't allow that)
I fiddled with this a little bit today:
https://gist.github.com/a9bab41bd9102cb4055429c5c79a2a11

This code works in Python 2 and Python 3. It's actually slightly faster than `json.dump` in Python 2, but slightly slower in Python 3. I didn't have time to try hooking it up in the build system, we'd need to replace the use of `json.dump` in configure.py as well as ensure that values that originate from old-configure are stored as `bytes` and not `unicode`.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> I fiddled with this a little bit today:
> https://gist.github.com/a9bab41bd9102cb4055429c5c79a2a11
> 
> This code works in Python 2 and Python 3. It's actually slightly faster than
> `json.dump` in Python 2, but slightly slower in Python 3. I didn't have time
> to try hooking it up in the build system, we'd need to replace the use of
> `json.dump` in configure.py as well as ensure that values that originate
> from old-configure are stored as `bytes` and not `unicode`.

- I started from that code, and changed the API so that it's not passed a file object but returns a string instead. Guess what? That made it 3 times as fast.
- It turned out that the handling of unicode strings doesn't do any escaping so strings containing single quotes, backslashes, etc. were broken. Fixing that made it significantly slower, but with further optimization, it's still twice as fast as both your original code and json.dump, on the testcase you have in that code.
- With real-life data (substs from config.status), your code was ~10% slower than json.dump. My changes make it 33% faster than json.dump.

Interestingly, switching data from old-configure to byte literals actually works the same as keeping unicode literals (in practice we were already using unicode literals) (modulo some fixable serialization problem for ALLSUBSTS) (except for one thing that was already fishy, I'll file a separate bug for it).

OTOH, it's likely that the serialization problem for ALLSUBSTS actually hides a mountain of other such problems: the thing is there are only two variables in my build that contain non-ascii: NONASCII and CL_INCLUDES_PREFIX. And that's only printed out as part of ALLSUBSTS. I bet having non-ascii in more central things like the topsrcdir path would break badly.
Nice! When you refer to problems with non-ascii, you're thinking that make would choke on it? That seems entirely plausible, although if we write out Makefiles in the system encoding *maybe* it would work?
It would. The "problem" is that that's a lot to change. I'm leaning towards leaving that for later. Fix things in a minimalistic way to get the job done for *this* bug (which, in fact, probably won't involve replacing json.dump), and file new bugs for the remaining stuff.
Comment on attachment 8731016 [details]
Bug 1257049 - Stop spawning a separate process for config.status from configure.py.

There is enough difference to grant a new review.
Attachment #8731016 - Flags: review+ → review?(gps)
Blocks: 1296503
Comment on attachment 8731016 [details]
Bug 1257049 - Stop spawning a separate process for config.status from configure.py.

https://reviewboard.mozilla.org/r/40313/#review70698
Attachment #8731016 - Flags: review?(gps) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/ee9f4eeaebe8
Stop spawning a separate process for config.status from configure.py. r=gps
https://hg.mozilla.org/mozilla-central/rev/ee9f4eeaebe8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: