dub: Cache PackageManager across dub{Package,Dependant} targets

[kinke]

No description provided.

[kinke]

@Geod24: Any suggestions? Maybe constructing directly here and feeding it NativePath.init as root package path?

[kinke]

@Geod24: Looking at https://github.com/dlang/dub/blob/0d4858eb473a31a058132c8692d499effdb1cb28/source/dub/packagemanager.d#L116-L124, it doesn't look like that special <root pkg path>/.dub/packages/ repo can be disabled. Is there a way?

[kinke]

And AFAICT, I cannot use the reggae project or build directory here either (for a somewhat consistent local .dub/packages repo, not depending on the order of dubPackage targets), as each dub root project/config gets a reggae Options struct with projectPath and workingDir both overridden to the dub root project directory. And the original projectPath is a bunch of calls up:

reggae/payload/reggae/rules/dub/external.d

Lines 123 to 132 in cc9ec99

	projectPath = reggaeOptions.projectPath;
	const path = buildPath(projectPath, dubPath.value);
	subOptions = reggaeOptions.dup;
	subOptions.projectPath = path;
	subOptions.workingDir = path;
	subOptions.dubConfig = dubPath.config.value;
	// dubInfos in this case returns an associative array but there's
	// only really one key.
	auto output = () @trusted { return stdout; }();
	dubInfo = dubInfos(output, subOptions)[dubPath.config.value];

[Geod24]

The PackageManager is not designed to handle changing Location, and was originally designed with 3 hardcoded Locations, making any kind of extensibility hard (e.g. the PlacementLocation enum).
Caching the PackageManager is an option but seems the most hacky one. Another option is to only instantiate one Dub instance (might require some changes to Dub), or to make package loading lazy. Per and myself have made some progress in the lazy loading, I don't know the Reggae codebase well enough to say how much Dub needs to change to allow for what it does. So ATM I don't have a satisfying answer, sorry.

[kinke]

Okay nevermind, I found a way, and am using the original reggae project directory now.

[kinke]

After some quick tests, this seems to work. On my box, a buildgen runtime (for a reggaefile.d with ~23 dub root projects) of previously 6.2 seconds was reduced to 5.7 secs; with an almost empty ~/.dub/packages cache. I hope it's more noticeable with a populated cache (on Windows, the original buildgen runtime was 23 seconds, and after killing %LOCALAPPDATA%\dub\packages, it was cut in half to 11.5 seconds - but that was before this PR).

[kinke]

I hope it's more noticeable with a populated cache

Seems like it, nice. I've repopulated my dub packages cache on Windows, fetching 111 packages. Then the buildgen runtime of master is ~13.5 secs, and with this PR down to ~10 secs (best of 3 each). That's quite a lot, as 111 dub packages in the cache isn't a lot, and a good portion of that runtime is caused by preGenerateCommands (edit: which are unaffected).

Edit: I've also diffed the produced build.ninja files (1.4 MB!) - identical.

[PetarKirov]

LGTM, but I defer to @Geod24 for the final decision.

[Geod24]

Why does Reggae need to instantiate more than one Dub instance though ?

[kinke]

Why does Reggae need to instantiate more than one Dub instance though ?

As said, a reggaefile.d might represent an orchestrated build of dozens of dub root projects/configs. The Dub class ctors AFAICT don't really look as if a single instance could be used for all of them, switching from one root project to the next. So currently, if reggaefile.d contains N dubPackage targets, it creates N Dub instances (and used to create N PackageManager instances along with it).

[Geod24]

Then I think caching the PackageManager makes sense. I've been looking into removing the scanning behavior. It was reintroduced at some point because of a corner case that I didn't know how to handle at the time, but can now. But even without the scanning, you'd end up loading the same packages multiple times, which is wasteful.

In the long run, I would like to look into why it needs multiple root packages. The only thing that root package has on a regular package is the dependency resolution / config. Dub can obviously build dependencies without them being root packages so I would expect Reggae to be able to do so as well (conceptually, the API for it is probably terrible).

[kinke]

One unittest fails because it changes a dub.sdl that it previously built, and rightfully expects reggae to re-generate its configuration after the .sdl change. But caching the PackageManager (for the whole unittest process - per thread) leads to the .sdl not being reloaded.

[atilaneves]

This means the test needs to know waaay too much about the implementation.

[kinke]

Well, the test needs to reparse a changed .sdl, something a real reggae process never does, and I thought this is the least invasive method to make that test still work.

[atilaneves]

I don't know enough about dub to review this. @Geod24 ?

[kinke]

Caching the PackageManager is an option but seems the most hacky one. Another option is to only instantiate one Dub instance (might require some changes to Dub), or to make package loading lazy. Per and myself have made some progress in the lazy loading, I don't know the Reggae codebase well enough to say how much Dub needs to change to allow for what it does.

I'm more than happy to revise this after according dub API changes; this performance problem is the only real blocker for switching our SIL build to lots of dubPackage targets in a central reggaefile.d, so I have to make do with what dub currently offers. What takes 10 secs in my Windows VM with this cache here currently takes 40 secs on our Windows CI runners (with an older reggae version missing some other existing optimizations, and a way more populated dub packages cache).

Wrt. the Dub class - what stands in reggae's way for using a single instance is that it's tightly coupled to a single root project: https://github.com/dlang/dub/blob/12c78d972680924446f205480c61167b82318286/source/dub/dub.d#L112-L129. With reggae, we might have a pretty large number of dub targets, specified by a pair of <path to dub pkg>, <dub config name>. So e.g. something like 3 configs for one package (executable, unittest runner, integration test runner), and one config (unittest runner only) for a library-only other package.

Lazy loading would be super-nice of course, so that the dub packages cache size hopefully wouldn't affect dub performance anymore.

[Geod24]

With reggae, we might have a pretty large number of dub targets, specified by a pair of , . So e.g. something like 3 configs for one package (executable, unittest runner, integration test runner), and one config (unittest runner only) for a library-only other package.

The one-config-per-root-project approach also doesn't work if we want to solve dlang/dub#1706 and dlang/dub#1217 .

[kinke]

Well reggae's dubPackage targets can come with custom options (incl. dub build type, and/or extra dflags applied to the entire dub tree for that specific root-pkg+config target etc. etc.). E.g., the integration tests config is usually coupled with the unittest dub build type (not the default debug one), which is the implicit one for the special unittest config. So I doubt there's much to gain if we tried to bundle all configs of a specific root project and process them as a bundle.