Closed Bug 1695312 Opened 3 years ago Closed 5 months ago

Make mach command modules be loaded selectively

Categories

(Firefox Build System :: Mach Core, enhancement, P3)

Product:
Firefox Build System
Component:
Mach Core
Type:
enhancement

Tracking

(firefox117 fixed)

Status:
RESOLVED FIXED
Milestone:
117 Branch
Tracking Flags:
Tracking Status
firefox117 --- fixed

People

(Reporter: mhentges, Assigned: ahochheiden)

References

Details

Attachments

(5 files, 6 obsolete files)

Bug 1695312 - Activate the virtualenv associated with a mach command much earlier r?#build
48 bytes, text/x-phabricator-request
Details | Review
Bug 1695312 - Selectively load only the mach command modules needed for the command about to be run r?#build
48 bytes, text/x-phabricator-request
Details | Review
Bug 1695312 - Sort the `MACH_COMMANDS` dict alphabetically r?#build
48 bytes, text/x-phabricator-request
Details | Review
Bug 1695312 - Add missing commands to the `MACH_COMMANDS` dict r?#build
48 bytes, text/x-phabricator-request
Details | Review
Bug 1695312 - Add the ability for `dispatch` to ad-hoc load command modules that aren't already loaded r?#build
48 bytes, text/x-phabricator-request
Details | Review

Currently, to register mach commands, we import all necessary mach_commands.py files, interpret them and their dependents, and run all @CommandProvider decorators.
This causes two downsides:

  • It slows the running of mach, because all modules are loaded, even those for commands who are unused. On my machine, mach takes 420ms to start up, and roughly half of that (~225ms!) is spent interpreting all these unnecessary modules
  • We attempt to import modules before activating a command's virtualenv.

Additionally, there's the general issue of mach commands being registered simply by having their file imported. There's some action-at-a-distance implicit work happening there which is tough to follow. By registering commands in a pythonic way, we improve tooling support and usability.

Priority: -- → P3
Depends on: 1696251
See Also: → 1388894
Depends on: 1388894
See Also: 1388894 →
No longer depends on: 1696251
Blocks: 1717104

Alex L has done some good work centralizing our MACH_COMMANDS, but there's still a fair amount of work to go to get lazy-loading working:

  1. First, refactor cases that depend on all mach modules being loaded at once:
    • conditions may effect whether or not commands are shown as disabled in ./mach --help. We should ensure that, instead, the conditions option only affects whether the current command to-be-run is enabled or not.
    • settings: move these to a global spot (probably to mach_initialize.py) so that they're always available for all Mach commands to use.
  2. Instead of importing all commands' modules, instead only import the one associated with the current command-to-be-run.
  3. Finally add some tests to verify correctness of the MACH_COMMANDS "command-to-module" reference list:
    • All commands must point to a file containing their associated @Command function
    • All modules (MACH_COMMANDS.values()) must have all @Command functions associated with an entry in MACH_COMMANDS.
Blocks: 1808732
Assignee: nobody → ahochheiden
Severity: -- → S3
Summary: Lazy-load mach commands → Make mach command modules be loaded selectively

This activated virtualenv for a command is managed
CommandSiteManager and it is passed down to where it was activated
before to prevent a second, redundant, activation.

Since this activation is happening before we can determine where the
objdir will be, we have to store the virtualenvs elsewhere. They will
now live in the same directory as the mach virtualenv has always
lived.

This makes loading almost all commands faster, since only one module
filed is loaded rather than all of them. There is one main exception,
dealing with 'help'. Running ./mach help (or -h or --help) requires
the description text for every command, so every module file is still
loaded.

We could expand this improvement here to consolidate all commands and
their parameters in this MACH_COMMANDS dict, but the only two benefits
are improving help, and not having two places where the commands are
specified (their file, and this dict).

There's a lot of extra work needed to do that, especially for handling
sub commands, and it did not seem worth the cost for the benefit at this
time.

Depends on D180499

This isn't strictly necessary for this work, but I know it's necessary
for upcoming work, so I may as well take care of it now, since it's
definitely related.

Depends on D180501

The build must work without having to download anything, as such,
everything needed by the build site (and by extension the mach site)
must be vendored. To simplify dependency conflicts between other sites,
everything else should not be vendored. If a new dependency is needed
for say lint, it should be specified as a pypi:<X>==<Y> in the
lint.txt file.

There may be vendored dependencies specified in there. Those can be
modified to PyPi dependencies if need be. They are only pointing to the
vendored versions because they are currently compatible, and not having
to download from PyPi is slightly faster on first-time site creation.

Depends on D180502

Attachment #9338380 - Attachment is obsolete: true

This is necessary for activating the site/command virtualenvs earlier,
since we can't determine what the objdir will be very early on in the
mach process intialization. We do know where the state dir is, and how
to get to the state dir for a specific topsrcdir, so we can use that
instead. This is already where the mach virtualenv lives anyway.

Also add the associated virtualenv names to each command that has one in
preparation for loading the virtualenvs earlier (next patch).

Depends on D181031

Attachment #9338379 - Attachment is obsolete: true
Depends on: 1838763

Comment on attachment 9339214 [details]
Bug 1695312 - Move the location of site/command virtualenvs out of the objdir and into the state dir r?#build

Revision D181029 was moved to bug 1838763. Setting attachment 9339214 [details] to obsolete.

Attachment #9339214 - Attachment is obsolete: true

This is needed so that we can remove the virtualenv_name parameter
from the @Command decorators because there's a subcommand for a
command that specifically needs this module. The easiest workaround
is to vendor this and use it in the common venv and remove the
subcommand's unique virtualenv alltogether.

Depends on D180501

Attachment #9339215 - Attachment is obsolete: true
Attachment #9341404 - Attachment description: WIP: Bug 1695312 - Remove the `virtualenv_name` parameter from all `@Command` decorators → Bug 1695312 - Remove the `virtualenv_name` parameter from all `@Command` decorators r?#build
Pushed by ahochheiden@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d2a2dd4790b
Sort the `MACH_COMMANDS` dict alphabetically r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/a35da3fce458
Add missing commands to the `MACH_COMMANDS` dict r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/eab5cdaf4bc9
Activate the virtualenv associated with a mach command much earlier r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/2839c2d35459
Selectively load only the mach command modules needed for the command about to be run r=firefox-build-system-reviewers,glandium
Attachment #9341403 - Attachment is obsolete: true
Attachment #9341404 - Attachment is obsolete: true
Pushed by ahochheiden@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56f3d3fc806f
Sort the `MACH_COMMANDS` dict alphabetically r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/06aefe3418e1
Add missing commands to the `MACH_COMMANDS` dict r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/6123c201f979
Activate the virtualenv associated with a mach command much earlier r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/e5687aa10124
Selectively load only the mach command modules needed for the command about to be run r=firefox-build-system-reviewers,glandium
Flags: needinfo?(ahochheiden)
See Also: → 1844306
Regressions: 1844349
Regressions: 1844350

Backed out as per request on Element, #sheriffs.

Status: RESOLVED → REOPENED
status-firefox117: fixed → ---
Resolution: FIXED → ---
Target Milestone: 117 Branch → ---
Blocks: 1844417

This is really just shuffling a bunch of things around. None of the
'load_*' member functions of the Mach class actually needed to be
member functions. They can all be static so that they can be used
anywhere. That combined with moving all the other 'mach_command' logic
to a different file, allows us to load the module for any command so
that we can successfully dispatch it.

Depends on D180500

Pushed by ahochheiden@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21f435391279
Sort the `MACH_COMMANDS` dict alphabetically r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/8b4be21eacef
Add missing commands to the `MACH_COMMANDS` dict r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/1932f8f581f7
Activate the virtualenv associated with a mach command much earlier r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/c36eec0834f4
Selectively load only the mach command modules needed for the command about to be run r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/ef16e8135fe1
Add the ability for `dispatch` to ad-hoc load command modules that aren't already loaded r=firefox-build-system-reviewers,glandium
Regressions: 1845306
Regressions: 1845365
Regressions: 1845426
Regressions: 1845444
Regressions: 1845272
Regressions: 1845833
Regressions: 1846065
Regressions: 1846068
Regressions: 1846281
Regressions: 1855312
Regressions: 1857279
You need to log in before you can comment on or make changes to this bug.