Move BuildInfo from sources into resources, defer use of resources during compilation #2425

lihaoyi · 2023-04-07T01:23:13Z

Instead of compiling the buildinfo values into the source code, we only compile the buildinfo keys into the source code and store the buildinfo values in the resources, to be loaded at runtime. This greatly reduces the amount of time spent compiling things when buildinfo changes. In this repo, some rough tests indicate that it makes the second -i installLocal after a one-line change in the LICENSE file drop from 50s to 14s of runtime

mill-profile.json Before:

mill-profile.txt

mill-profile.json After:

mill-profile.txt

Notes:

To really benefit from this, I had to make compileClasspath depend only on upstream module's compileClasspath()/compile().classes, so it doesn't end up depending on their runtime resources() and forcing unnecessary recompiles when the buildinfo resources change. This is probably a change we want to do anyway, given the direction set by Don't include resources into compileClasspath. #1811
I consolidated the BuildInfo implementation with contrib/buildinfo/.
Extended contrib/buildinfo/ to support Java modules, which we dogfood in main.client.
The old behavior of statically compiling the values into the binary is still available under def buildInfoStaticCompiled = true, for anyone who needs it. That includes anyone using BuildInfo in Scala.js, which doesn't support JVM resources by default
For now, I copy-pasted the implementation into the root build.sc file, and updated mill-bootstrap.patch to remove it and make use of the mill-contrib version. When we re-bootstrap Mill on latest main, we can remove the copy-pasta
contrib/buildinfo/ should be mostly source compatible with the previous implementation, except that I made buildInfoPackageName a required field to encourage the best practice of not putting code in the empty package

lefou

Interesting approach. It swaps compiletime work with runtime work. Not really my taste.

I think this approach complicates things. We now do some work at runtime which can fail. To be performant, we also need to cache things a bit, to avoid loading the resources again and again. Also, in the current draft, we loose Scaladoc, but that could be changed, and in general the flexibility to provide more than just key-value pairs.

What I like about this approach. It is still better than using system properties for things, which are not meant to be changed.

lefou · 2023-04-07T08:09:27Z

What I wonder. How great is the impact of using generated code that changes with every git change? As we use zinc, which tracks binary changes and our generated classes does not change its API (only the values, which are not part of the ABI), downstream compilation targets should not really be affected. Only other targets like jar or assembly.

lihaoyi · 2023-04-07T09:40:52Z

@lefou I updated the PR description with some rough benchmarks running installLocal on this repo. It's a pretty dramatic improvement, from 50s to 14s. You can look at the profiles, all those no-op Zinc compiles take around 100-1000ms each to run and decide to do nothing, all the docJars take a while to re-generate since those are not incremental, and when you have a bunch of modules it adds up. After this PR, the only things it needs to re-build is the final assembly taking ~10s.

My own specific motivation comes when running the .fork/.server example/integration tests on this repo, which needs to publishLocal all the modules before running. Waiting ~1min every time I iterate on an example/integration test is a big annoyance. 14s isn't great either, but it's better, and maybe I can drive that number down further in future

The new mechanism is definitely more complicated, but I'd argue it better reflects the true dependencies of the system:

When the buildInfo keys change, then we need to re-compile the world, since we're changing the set of fields we make statically-available on the BuildInfo object
When the buildInfo values change, we do not need to re-compile anything, but we do need to re-package the new values for use at runtime

lihaoyi · 2023-04-07T10:01:29Z

Yeah being limited to primitive key-values is a bit annoying, but I don't think it's the end of the world. We can always add back support for more complex data structures with the appropriate code-gen. But there's only one place in com-lihaoyi/mill which uses a Seq[String] rather than a plain String, and contrib/buildinfo/ doesn't support data structures at all, so I think we're ok with dropping that functionality for now

lefou

Currently, you put each property in a single resource file. Have you considered using java.util.Properties for loading from a single file? This would also reduce complexity.

lefou · 2023-04-07T13:08:26Z

contrib/buildinfo/src/mill/contrib/buildinfo/BuildInfo.scala

+      .map {
+        case (k, v) =>
+          if (isScala) s"""val $k = this.readMillBuildInfo("$k")"""
+          else s"""public static java.lang.String $k = readMillBuildInfo("$k");"""


This needs to be a method, not a field, otherwise the compiler is allowed to inline the value at the call side, which we need to avoid.

This needs to be a method, not a field, otherwise the compiler is allowed to inline the value at the call side, which we need to avoid.

I don't think that happens unless the RHS of the final field is a literal constant or (constant-folded). So in this case, a method call loading a JVM resource, it won't get inlined

In the buildInfoStaticCompiled = true case, we need to make it a method, as the constants are candidates for inlining (https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.28), but that means we also need to make it a method in the other case, otherwise the generated code is not interchangeable.

lihaoyi · 2023-04-07T13:15:37Z

Currently, you put each property in a single resource file. Have you considered using java.util.Properties for loading from a single file? This would also reduce complexity.

I considered it, but some things I liked about separate files:

You don't need to worry about data formats, escaping newlines, separators, etc. With one-file-per-entry, the file contains the literal string, that's it
If multiple people define buildinfo keys in the same package having them in separate files means that they automatically get unioned together when classpaths are combined. This applies to whether they're in separate folders, separate jars, combined into a single jar, etc. all without needing any special configuration (e.g. Assembly.Rule.Append) to make it happen. (This should be mostly avoided by forcing people to provide a packagename, but nonetheless...),
General buildinfo keys are pretty few, like O(10), certainly smaller than the number of classfiles that are in the jar anyway. So having them in separate files won't be too bad w.r.t. perf

lihaoyi · 2023-04-08T00:51:25Z

One thing of note is that I had to add ./mill contrib.buildinfo.publishLocal to ci/patch-mill-bootstrap.sh, now renamed to ci/prepare-mill-bootstrap.sh, since Mill's own build.sc now for the first time is using a contrib plugin. millProjectModule isn't wired up to support contrib plugins, which I think might take a bit more thought to do nicely.

I think that's probably a good thing that we've started dogfooding our own plugins, even if it takes a bit more setup to bootstrap.

lihaoyi · 2023-04-08T10:58:10Z

all the feedback so far has been addressed and CI is green(ish)

lefou · 2023-04-08T11:36:14Z

One thing of note is that I had to add ./mill contrib.buildinfo.publishLocal to ci/patch-mill-bootstrap.sh, now renamed to ci/prepare-mill-bootstrap.sh, since Mill's own build.sc now for the first time is using a contrib plugin. millProjectModule isn't wired up to support contrib plugins, which I think might take a bit more thought to do nicely.

I think that's probably a good thing that we've started dogfooding our own plugins, even if it takes a bit more setup to bootstrap.

One thought, not a request: Isn't it now possible to directly include the contrib/buildinfo/src/mill/contrib/buildinfo/BuildInfo.scala into build.sc via import $file?

lihaoyi · 2023-04-08T11:41:51Z

It isn't, because import $file assumes a script, which means .sc extension and disallows package declarations. That's unchangeable while we're still using Ammonite

Once we tag a milestone and rebootstrap, we'll be able to configure the mill-build/build.sc to add the .scala to the sources to be compiled without the script code wrapper. So in future we should be able to avoid this copy-paste-patch messiness and just directly share the .scala file between application code and the root build.sc using the meta-build (similar to what a lot of SBT projects do)

import $file still wouldnt work, but we could conceivably add a shortcut import $scalafile or something to add non-script Scala files to the sources without needing to define a meta-build

lefou · 2023-04-08T11:43:43Z

I was referring to the bootstrap tests, where we need to publishLocal the plugin, but instead could load it directly as source file.

lefou · 2023-04-08T11:45:46Z

In another project we kept .scala symlinks to .sc files, to use plugin code in Mill directly and to publish it as plugin.

lihaoyi · 2023-04-08T11:47:19Z

Oh, for the bootstrap tests, we might be able to. I don't see how import $file would help there, for the same reasons. My understanding is that we'd need to wire them up the same way we do millProjectModules

lihaoyi · 2023-04-08T11:49:56Z

How does the symlinks work with the package declaration st the top of the file? My understanding is that it's a blocker, since it's required to have one for .scala files (assuming we avoid the empty package) and required not to have one for .sc files (since they get wrapped, and package declarations in Scala can only be at the top of files)

lefou

Some files needs formatting. Otherwise, this looks good to go. And I'm happy about the outcome. The buildinfo plugin is now in a usable state (Java support and Java/ScalaDoc). Nice work!

lihaoyi · 2023-04-08T11:54:58Z

Is this how you're meant to auto-format the files? It's the command you gave inhttps://github.com/com-lihaoyi/os-lib/pull/160, but seems to give me in an error

lihaoyi mill$ ./mill __.reformat
Cannot resolve __.reformat. Try `mill resolve _` to see what's available.

lefou · 2023-04-10T09:02:04Z

https://com-lihaoyi.github.io/mill/mill/Configuration.html#_reformatting_your_code

…2435) This should allow us to do debugging or iteration on example/integration in `.local` mode, which is a lot faster than `.fork` or `.server` Even after #2425 landed, the `.local` integration tests are still much faster to run than `.fork` and `.server`, with publishing taking ~20s down from ~60s before which is still a very annoying wait. Better if we can avoid that, using `.local` in the common development workflow and leaving `.fork` and `.server` for CI to catch rare edge-case bugs We basically needed to be a bit more robust in spawning subprocesses, allowing the `InputPumper` machinery to work for any redirected streams rather than being hardcoded to work with the `PipedInputStream` that `MillServerMain` uses. I had to fix an issue in `ScalaJSModule#run` to make `example.web[3-scalajs-module].local.test` pass with this additional enforcement. It appears we were not properly pumping the JSEnv subprocess stdout/stderr. This would cause the outputs to disappear when run repeatedly in client-server mode, since the output would be inherited by first MillClient process that spawned the server, and not subsequent clients that simply connect to it --------- Co-authored-by: Tobias Roeser <le.petit.fou@web.de>

lolgab · 2023-06-01T11:27:07Z

This broke my Scala.js build.
Can we set the default so ScalaJSModules and ScalaNativeModules always gets def buildInfoStaticCompiled = true?

This adds support for Mill API 0.11. In Mill 0.11.0-M8, some internals have changed (com-lihaoyi/mill#2425), so the consumption of transitive module dependencies had to be changed to continue to generate proper OSGi Manifests. This feels a bit fragile in the sense, that if the customized targets and their interactions change upstream, we might need to follow these changes in this plugin. Pull request: #118

lihaoyi added 5 commits April 7, 2023 07:36

.

504d6ff

build.sc compiles

1075d7e

move to resource based buildinfo

482b52f

.

18a54a4

cleanup

c9382e5

lefou reviewed Apr 7, 2023

View reviewed changes

lihaoyi added 3 commits April 7, 2023 16:17

fix

9eef9c3

.

028c779

transitiveCompileClasspath should include compile.classes

cddc254

lihaoyi changed the title ~~[WIP] Move BuildInfo dependencies into forkArgs to avoid unnecessary re-compilations~~ [WIP] Move BuildInfo from sources into resources, defer use of resources during compilation Apr 7, 2023

lihaoyi added 13 commits April 7, 2023 18:19

wip replacing contribu/buildinfo with new implementation

0dc7664

contrib.buildinfo.test passes

80b8d0b

fix build.sc

1a5bb75

.

ac0e98e

.

f6e96b0

.

5050886

update patch bootstrap

f490919

add mill-contrib-buildinfo import to patch

196c320

scaladoc

e2625ce

fixcompile

e28d97f

fix-compile

fe6aef7

.

a622dca

fixbsp

6600186

lihaoyi marked this pull request as ready for review April 7, 2023 12:45

add import to patch

f351a10

lefou reviewed Apr 7, 2023

View reviewed changes

update

e229fc9

lihaoyi added 3 commits April 8, 2023 10:21

remove-println

235fc7f

fix-patch

d50c0ed

.

1ae58e4

lihaoyi force-pushed the dynamic-mill-version branch from 822f690 to 1ae58e4 Compare April 8, 2023 02:24

lihaoyi added 4 commits April 8, 2023 16:01

debug

f725a38

.

8ea492f

add publishLocal to make test-mill-dev pass

8d7f669

bump timeout for flaky test

a6e2084

lihaoyi changed the title ~~[WIP] Move BuildInfo from sources into resources, defer use of resources during compilation~~ Move BuildInfo from sources into resources, defer use of resources during compilation Apr 8, 2023

lefou approved these changes Apr 8, 2023

View reviewed changes

scalafmt

f36c0b8

lihaoyi merged commit d9cf409 into com-lihaoyi:main Apr 8, 2023

lefou added this to the 0.11.0-M8 milestone Apr 10, 2023

lihaoyi mentioned this pull request Apr 14, 2023

enforce stdout/stderr contents in .local example/integration tests #2435

Merged

lefou mentioned this pull request Jun 24, 2023

Update Mill API to 0.11 and some tweaks lefou/mill-osgi#118

Merged

lefou mentioned this pull request Sep 11, 2023

Mill doesn't evict older versions of transitive dependencies from moduleDeps #2732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move BuildInfo from sources into resources, defer use of resources during compilation #2425

Move BuildInfo from sources into resources, defer use of resources during compilation #2425

lihaoyi commented Apr 7, 2023 •

edited

Loading

lefou left a comment •

edited

Loading

lefou commented Apr 7, 2023

lihaoyi commented Apr 7, 2023 •

edited

Loading

lihaoyi commented Apr 7, 2023

lefou left a comment

lefou Apr 7, 2023

lihaoyi Apr 7, 2023

lefou Apr 7, 2023

lihaoyi commented Apr 7, 2023

lihaoyi commented Apr 8, 2023 •

edited

Loading

lihaoyi commented Apr 8, 2023 •

edited

Loading

lefou commented Apr 8, 2023

lihaoyi commented Apr 8, 2023 •

edited

Loading

lefou commented Apr 8, 2023

lefou commented Apr 8, 2023

lihaoyi commented Apr 8, 2023

lihaoyi commented Apr 8, 2023 •

edited

Loading

lefou left a comment

lihaoyi commented Apr 8, 2023

lefou commented Apr 10, 2023

lolgab commented Jun 1, 2023

Move BuildInfo from sources into resources, defer use of resources during compilation #2425

Move BuildInfo from sources into resources, defer use of resources during compilation #2425

Conversation

lihaoyi commented Apr 7, 2023 • edited Loading

lefou left a comment • edited Loading

Choose a reason for hiding this comment

lefou commented Apr 7, 2023

lihaoyi commented Apr 7, 2023 • edited Loading

lihaoyi commented Apr 7, 2023

lefou left a comment

Choose a reason for hiding this comment

lefou Apr 7, 2023

Choose a reason for hiding this comment

lihaoyi Apr 7, 2023

Choose a reason for hiding this comment

lefou Apr 7, 2023

Choose a reason for hiding this comment

lihaoyi commented Apr 7, 2023

lihaoyi commented Apr 8, 2023 • edited Loading

lihaoyi commented Apr 8, 2023 • edited Loading

lefou commented Apr 8, 2023

lihaoyi commented Apr 8, 2023 • edited Loading

lefou commented Apr 8, 2023

lefou commented Apr 8, 2023

lihaoyi commented Apr 8, 2023

lihaoyi commented Apr 8, 2023 • edited Loading

lefou left a comment

Choose a reason for hiding this comment

lihaoyi commented Apr 8, 2023

lefou commented Apr 10, 2023

lolgab commented Jun 1, 2023

lihaoyi commented Apr 7, 2023 •

edited

Loading

lefou left a comment •

edited

Loading

lihaoyi commented Apr 7, 2023 •

edited

Loading

lihaoyi commented Apr 8, 2023 •

edited

Loading

lihaoyi commented Apr 8, 2023 •

edited

Loading

lihaoyi commented Apr 8, 2023 •

edited

Loading

lihaoyi commented Apr 8, 2023 •

edited

Loading