Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #748 +/- ##
==========================================
+ Coverage 87.25% 87.35% +0.09%
==========================================
Files 75 75
Lines 4692 4808 +116
Branches 817 844 +27
==========================================
+ Hits 4094 4200 +106
- Misses 468 476 +8
- Partials 130 132 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3190a73 to
7099200
Compare
|
Thanks for the work here. In addition to promoting Given the impact of spawning subprocesses in Windows, I'm skeptical that this aggressive caching is even worth the risk of invalid caches causing cryptic and difficult to reproduce bugs. Please try simplifying the change to focus only on the high-impact DSV capabilities and priority changes so we have the numbers behind that specific part of the change. If we decide that there is an opportunity for some package-level or hook-level caching, I'd rather see that explored in a way that would provide the benefit to all shells, not just It's probably worth noting here that the type of workspace plays a huge role in describing the impact of |
|
Thanks for the early look, wasn't quite ready for another review yet, but this is good feedback, I will incorporate. |
The .dsv descriptors are colcon's own representation of an environment change, and a pure-Python evaluator for them is generated into every install prefix. generate_command_environment is the one consumer that never moved onto them: it writes a script which dot-sources each dependency's per-shell rendering plus all of its hooks and spawns a shell to dump the result. Implement it on DsvShell instead, at a priority above the shell extensions, and fall back to them whenever a descriptor references a script which has no descriptor of its own, or a dependency provides no package.dsv. Measured on a 363 package ROS 2 workspace on Windows, where the shell path costs 438 + 60.5 * n_deps ms per package and the median package has 95 dependencies: 390 ms per package instead of 6.2 s, with the resulting environment matching what the bat extension produced for every package.
7099200 to
537b783
Compare
|
Okay, I dropped the caching bits. The bugs are too subtle to be worth it. Also, agreed on workspace type. My profiling is done with merge-install, which means all the scripts should be effectively no-op, right? An isolated workspace would have the same number of scripts, but they would actually be doing things in each script. |
cottsay
left a comment
There was a problem hiding this comment.
Thanks for slimming down the change.
Looking pretty good, just a couple of comments and some manual testing to do.
|
Followed up on the workspace-type question with actual data instead of just reasoning about it. Built a full rolling workspace from
No measurable difference on Linux. |
|
'Lotta em-dash in there... |
|
I'm surprised it didn't move the needle at all for you. On Nix, I did something similar with building the environment using the dsvs, in part because sourcing hundreds of https://github.com/clearpathrobotics/nix-ros-base/tree/devel/lib/setup#colcon-env-setup I think the big time eater was the envvar dedup, as each new local_setup would iterate longer and longer path lists, checking each entry to make sure it wasn't adding something already present. That's an N^2 operation so it really adds up past a certain point. |
Yeah, moved to a new computer so 👷 has gotten a little trigger happy. I'm going to retest this on Windows though, because I think it will be more obvious there. |
|
Win11, colcon-core 0.17.1 vs this branch (537b783)
stock ps1 10.8 s (merge) / 7.4 s (isolated) |
|
…_prefix Per cottsay's review: include original exception context when raising _MissingDescriptorError, and apply the altsep normalization to the joined path rather than just the value, so a wrongly-separated prefix is also caught.
The
.dsvdescriptors are colcon's own representation of an environment change. They arrived in 2019 (cbee991) together with a pure-Python evaluator generated into every install prefix (bd55f3f);shandbatwere moved onto that evaluator for their prefix scripts within days (bccec1d, 8fc78e3), and #244 made the descriptor authoritative even where no shell-specific script exists. The per-shell hooks are renderings of the same data.generate_command_environmentis the one consumer that never moved. It still writes a script which dot-sources each dependency's per-shell rendering plus all of its hooks and spawns a shell to dump the result. This implements it onDsvShellinstead, at a priority above the shell extensions, and falls back to them whenever a descriptor references a script that has no descriptor of its own, or a dependency provides nopackage.dsv.Not spawning a shell is also considerably cheaper. On a 363 package ROS 2 workspace on Windows the shell path costs roughly
438 + 60.5 * n_depsms per package and the median package has 95 dependencies; evaluating the descriptors instead costs about 390 ms per package, measured over all 32664 dependency pairs of that workspace.rcutilsrviz_default_plugins