Arksine / moonraker

Web API Server for Klipper
https://moonraker.readthedocs.io
GNU General Public License v3.0
1.02k stars 392 forks source link

git_deploy: intialize in parallel #825

Closed kdomanski closed 3 months ago

kdomanski commented 4 months ago

Massively speeds up startup on setups with many git repos.


I cannot figure out what takes so long to initialize() the GitDeploy instances and the pyinstrument lib doesn't work well with asyncio.

The fact is, running all initializers at once instead of waiting for one at a time speeds up my server_init() from ~6.5 seconds to ~3 seconds.

Relevant slice of my moonraker.conf:

[update_manager]
enable_auto_refresh = True
refresh_window = 19-7
refresh_interval = 12

[update_manager Klippain]
type = git_repo
path = ~/klippain_config
channel = beta
origin = https://github.com/Frix-x/klippain.git
primary_branch = main
managed_services = moonraker klipper
install_script = install.sh

[update_manager fluidd]
type = web
channel = stable
repo = fluidd-core/fluidd
path = ~/fluidd

[update_manager sonar]
type = git_repo
path = ~/sonar
origin = https://github.com/mainsail-crew/sonar.git
primary_branch = main
managed_services = sonar
install_script = tools/install.sh

[update_manager led_effect]
type = git_repo
primary_branch = master
path = ~/klipper-led_effect
origin = https://github.com/julianschill/klipper-led_effect.git
is_system_service = False

[update_manager happy-hare]
type = git_repo
path = ~/Happy-Hare
origin = https://github.com/moggieuk/Happy-Hare.git
primary_branch = main
install_script = install.sh
managed_services = klipper

[update_manager crowsnest]
type = git_repo
path = ~/crowsnest
origin = https://github.com/mainsail-crew/crowsnest.git
managed_services = crowsnest
install_script = tools/install.sh                                                                                                                                                                                                                                           

[update_manager z_calibration]
type = git_repo
path = ~/klipper_z_calibration
origin = https://github.com/protoloft/klipper_z_calibration.git
primary_branch = master
managed_services = klipper
Arksine commented 4 months ago

The delay is the result of spawning git subprocesses during initialization. Moonraker checks the git config for each repo to see if any other instances of Moonraker are managing the same repo, then adds its own instance ID to the git config. The goal is to warn if multiple instances of Moonraker are managing the same repo, as this is not desirable.

I have some concerns about spawning concurrent git processes. Historically I have found git to be finicky, so I have intentionally avoided re-entry with regard to init and refresh.

kdomanski commented 4 months ago

Hey, thanks for the explanation. Care to elaborate about the troubles you've encountered? TBH I don't see any additional re-entrant execution here, since the spawning is per GitDeploy instance. Whatever race condition this might create, already exist due to refresh timers, no?

Your response triggered another idea though. I changed the UpdateManager.component_init() method to run lazily (with event_loop.register_callback) and it shaved off almost another second (now 6.6s -> 2.2s). I'd argue that it's a good idea to already start the server while the update manager is still doing its initial checks in the background. On my printer, this represents about 15% of total startup time.

If you're open to more back-and-forth about this, we can take the conversation to Discord.

Arksine commented 3 months ago

I looked at this in a bit more detail and decided to make a change in commit 10dfb0d47729d4c8af3df30d9fd020c7b5e1ee0c. Rather than use the git config to store multi-instance info the update manager will use shared memory. This only occurs once for the entire UM, which avoids the slowdown introduced by calling git config.

FWIW, it is necessary to run the UM's component_init() prior to the server starting. Several attributes are set in initialize(), lazily setting them would potentially result in an exception if a frontend requests the status endpoint before they are set.

kdomanski commented 3 months ago

I looked at this in a bit more detail and decided to make a change in commit 10dfb0d. Rather than use the git config to store multi-instance info the update manager will use shared memory. This only occurs once for the entire UM, which avoids the slowdown introduced by calling git config.

I just measured, this is fantastic! Speedup from 6.6s to 2.3s. Thank you for being so open to tackling this!

FWIW, it is necessary to run the UM's component_init() prior to the server starting. Several attributes are set in initialize(), lazily setting them would potentially result in an exception if a frontend requests the status endpoint before they are set.

In my test with lazy init, it was sufficient to add self.last_refresh_time = 0.0 in the constructor. But hey, with your new changes it doesn't matter anymore.