Alex Crichton (May 15 2020 at 18:31): Alex Crichton (May 15 2020 at 18:31):@fitzgen (he/him) currently we're building z3 and peepmatic on all the test builders, was that intentional?
Alex Crichton (May 15 2020 at 18:32):it looks like CI time for windows has spiked to 45m
fitzgen (he/him) (May 15 2020 at 18:33):yes, it is intentional
I don't know why windows is so slow to compile z3
we can't use system z3 because it is missing symbols (at least on ubuntu) :-/
fitzgen (he/him) (May 15 2020 at 18:35):I guess we could look into fetching from https://github.com/Z3Prover/z3/releases and using those prebuilt Z3s
Alex Crichton (May 15 2020 at 18:38):Hm why do we want to build peepmatic and such on all builders?
Alex Crichton (May 15 2020 at 18:38):It seems like architecturally it's so expensive we wouldn't really want to do that
Alex Crichton (May 15 2020 at 18:38):but instead we should focus it on one builder, but no need to test all channels on all platforms in all modes
Alex Crichton (May 15 2020 at 18:38):I'm not sure if it's z3 specifically or something else that's slow
fitzgen (he/him) (May 15 2020 at 18:39):well, since getting windows builds working was somewhat difficult, I wouldn't want to remove its testing
fitzgen (he/him) (May 15 2020 at 18:39):I'm not sure if it's z3 specifically or something else that's slow
it is building z3 that is slow here
Alex Crichton (May 15 2020 at 18:39):was that something we did?
Alex Crichton (May 15 2020 at 18:39):or was that upstream in the z3 crate?
Alex Crichton (May 15 2020 at 18:40):It just feels weird to blow 20 minutes testing a thing that's off-by-default for everyone
fitzgen (he/him) (May 15 2020 at 18:40):we statically link our own build of z3 because system packages are unreliable and missing symbols and such
Alex Crichton (May 15 2020 at 18:40):that's a long time for a PR to be sitting on CI
Alex Crichton (May 15 2020 at 18:40):but that logic is in z3, right?
Alex Crichton (May 15 2020 at 18:40):not in wasmtime?
Alex Crichton (May 15 2020 at 18:40):like it seems like z3's windows support shouldn't be tested by wastime
Alex Crichton (May 15 2020 at 18:40):wasmtime*
fitzgen (he/him) (May 15 2020 at 18:41):it is a feature of the z3 crate that I added and turned on
Alex Crichton (May 15 2020 at 18:41):but why would we be responsible for testing it?
Alex Crichton (May 15 2020 at 18:41):in the wasmtime ci that is
fitzgen (he/him) (May 15 2020 at 18:41):the z3 crate's ci does test it; we aren't responsible for testing that, its just that we test everything on all of our platforms
Alex Crichton (May 15 2020 at 18:42):but why would we test specifically on windows?
Alex Crichton (May 15 2020 at 18:42):like why isn't one builder sufficient?
Alex Crichton (May 15 2020 at 18:42):I thought we were just going to have one builder verify that the peepmatic blob is up-to-date
fitzgen (he/him) (May 15 2020 at 18:43):why do we test anything on windows?
Alex Crichton (May 15 2020 at 18:43):hm that feels like a disingenuous question
fitzgen (he/him) (May 15 2020 at 18:43):there is only one builder that tests that the generated peephole optimizations are up to date
Alex Crichton (May 15 2020 at 18:43):like we obviously have tons of windows-specific code in wasmtime
Alex Crichton (May 15 2020 at 18:43):but why would anything in peepmatic be windows-specific?
Alex Crichton (May 15 2020 at 18:43):right what I mean is that i thought we were only going to build peepmatic on that one builder
fitzgen (he/him) (May 15 2020 at 18:44):lets back up a bit
fitzgen (he/him) (May 15 2020 at 18:44):when peepmatic is on by default, do you want to still only test it on ubuntu?
Alex Crichton (May 15 2020 at 18:44):that's what I would expect, yeah
fitzgen (he/him) (May 15 2020 at 18:44):like, it seems we want to test everything we ship on all platforms we are shipping them on
Alex Crichton (May 15 2020 at 18:45):b/c it's development model is "we don't care how slow it is to build
Alex Crichton (May 15 2020 at 18:45):I think you're conflating this though
Alex Crichton (May 15 2020 at 18:45):we're not shipping peepmatic anywhere
Alex Crichton (May 15 2020 at 18:45):we ship wasmtime everywhere
Alex Crichton (May 15 2020 at 18:45):we only ship the peepmatic-generated-blob
Alex Crichton (May 15 2020 at 18:45):so I don't think there's much benefit to getting so much coverage
Alex Crichton (May 15 2020 at 18:45):like it's not like it's wrong but it is costly
fitzgen (he/him) (May 15 2020 at 18:45):much of the test coverage for the peepmatic-generated blob is in crates that depend transitively on z3
Alex Crichton (May 15 2020 at 18:45):and apparently very costly
Alex Crichton (May 15 2020 at 18:46):but we test the blob in all configurations if we enable it by default
fitzgen (he/him) (May 15 2020 at 18:46):I agree we need to make the cost lower, and I suggested a potential solution for investigation: fetch pre-built libz3.sos from the z3 github releases page
Alex Crichton (May 15 2020 at 18:46):z3 is only needed generating the blob, and I'm saying I don't think there's any need to test blob-generation on all platforms
fitzgen (he/him) (May 15 2020 at 18:47):we are not testing blob generation on all platforms
Alex Crichton (May 15 2020 at 18:47):I mean yeah we can make it faster but I still don't see the point in testing it on all platforms
Alex Crichton (May 15 2020 at 18:47):beyond making us feel good about ourselves
Alex Crichton (May 15 2020 at 18:47):aren't we?
Alex Crichton (May 15 2020 at 18:47):like we're running cargo test -p peepmatic basically
fitzgen (he/him) (May 15 2020 at 18:47):no, only the rebuild peephole optimizers builder is regenerating the blob
yes, we are running cargo test -p peepmatic
Alex Crichton (May 15 2020 at 18:48):something about cargo test -p peepmatic is bringing in z3?
Alex Crichton (May 15 2020 at 18:48):like something is building z3 on the "test everything" builders
fitzgen (he/him) (May 15 2020 at 18:48):peepmatic depends on z3
Alex Crichton (May 15 2020 at 18:48):and I thought we were not going to do that
fitzgen (he/him) (May 15 2020 at 18:48):we aren't building z3 on the dist builders
fitzgen (he/him) (May 15 2020 at 18:48):which is what we talked about before
Dan Gohman (May 15 2020 at 18:49):My understanding from reading the patch is that we don't link the z3 engine into wasmtime. Is that the case?
Alex Crichton (May 15 2020 at 18:49):sorry I should have clarified, but dist/test is what I was talking about there
fitzgen (he/him) (May 15 2020 at 18:49):My understanding from reading the patch is that we don't link the z3 engine into wasmtime. Is that the case?
this is correct
Alex Crichton (May 15 2020 at 18:49):I think we should build z3 on precisely one builder
Alex Crichton (May 15 2020 at 18:49):the one that does peepmatic/blob testing
Dan Gohman (May 15 2020 at 18:51):Offhand, that sounds reasonable to me. In theory z3 or the peepmatic code could differ depending on the host, but that should be very rare and arguably not something we need to be testing for every CI run
fitzgen (he/him) (May 15 2020 at 18:52):I don't disagree with the conclusion per se, but I guess I am confused where the line is drawn. when peepmatic is enabled by default, do we test it on all the testers?
Alex Crichton (May 15 2020 at 18:53):I think it depends on what you mean by testing peepmatic, I personally think that no matter what at all times we should only build z3 in one place one itme
Alex Crichton (May 15 2020 at 18:53):and that's possible b/c wasmtime doesn't depend on z3
fitzgen (he/him) (May 15 2020 at 18:53):also, if testing peepmatic is fast, does it change the calculus? because all the time currently is spent in building z3, and if we don't do that, then it will be pretty fast
fitzgen (he/him) (May 15 2020 at 18:54):("don't build z3" == "fetch built release from Z3 github releases")
Alex Crichton (May 15 2020 at 18:54):It would from a practical point of view, but I would personally say we still shouldn't do it
Alex Crichton (May 15 2020 at 18:54):like it'd change to the point that I still not want to build the z3 crate everywhere but I wouldn't care enough to pursue it
Dan Gohman (May 15 2020 at 18:55):I hold out hope that someday CI machines will have better ways to cache builds of dependencies between runs, but until then, I agree.
fitzgen (he/him) (May 15 2020 at 18:56):aside: it might be nice to set up sccache for our ci
Dan Gohman (May 15 2020 at 18:56):Normal users building from source on Windows won't regenerate the bytecode, because that's behind a feature flag disabled by default, so even if z3/peepmatic would generate different bytecode on Windows, it won't affect people's actual experience by default
fitzgen (he/him) (May 15 2020 at 18:57):I guess I am fearful of it breaking on windows and no one noticing, but I guess that is pretty unlikely given that the z3 repo's CI is testing windows builds now
Pat Hickey (May 15 2020 at 18:58):I have no opinion on most of this conversation, but Adam recently setup Lucet with CircleCI caching the target directory and it made a significant improvement on build times. I’m not sure if GH CI can do that sort of caching.
Pat Hickey (May 15 2020 at 18:58):(We need CircleCI to test our userfaultfd memory management backend, that syscall isn’t available on GH CI)
Alex Crichton (May 15 2020 at 19:02):now there's a reason for picking a CI system I haven't heard of before
fitzgen (he/him) (May 15 2020 at 19:05):https://github.com/bytecodealliance/wasmtime/pull/1714
Till Schneidereit (May 15 2020 at 19:10):Would it make sense to only run this at all if the peepmatic input changed in a PR?
Till Schneidereit (May 15 2020 at 19:10):or any configuration affecting it, I guess
fitzgen (he/him) (May 15 2020 at 19:12):figuring out all the things that could affect it could be a game a wack a mole, I think
e.g. changing a clif operator could require changes to the glue between peepmatic and cranelift
Till Schneidereit (May 15 2020 at 19:13):ah, that's a good point!
Last updated: Dec 23 2024 at 13:07 UTC