-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust cache invalidated between native and Wasm builds #971
Comments
Perhaps we should have two separate Cargo workspaces, one for x86 and one for wasm? cc @project-oak/core |
We could have the following top-level directories:
Thoughts? |
Couple of cross-refs:
|
@rbehjati could you try and pull out the |
I am not sure things are in a consistent state after #1034; is
|
This may also be the cause of #1037 |
FYI I just realised that |
It should help with project-oak#971, until a more proper solution is in place.
It should help with project-oak#971, until a more proper solution is in place.
@tiziano88 25c1d06#diff-35e128d19b2a49f8257e7a6ed82e3f44 |
It should help with project-oak#971, until a more proper solution is in place.
Good catch @blaxill , thanks! I do think it is solved by reusing the same directory, hopefully |
It should help with #971, until a more proper solution is in place. In my experiments, this brings the time to run `./scripts/run_examples` without having made any changes from 440s down to 39s.
I am not sure if
If instead of
So, most of the changes in #1044 are still relevant. We could remove [edit: I noticed that I've included results from experimenting with |
I think if you undo your changes, then #1044 definitely makes a difference. Now that your changes are in, as you say, it does not seem to make a difference any more, since target dirs are already separated by workspace anyways. Hence my point that we can now remove that flag. But I haven't actually tried this myself. |
My hypothesis is that #1044 would not have made a huge difference if you had not added |
Could you try to remove |
The cache does not seem to get invalidated in that case.
I noticed that the change you made to aggregator backend improved things, but I was not timing the commands before. So I could not really measure the improvement... all I have is just a hunch! |
If the tests were done in master, after your change that separated the examples workspace already, then I don't think this experiment is particularly conclusive though, is it? |
Yes. The tests were done in master. What do we want to reach a conclusion about? Whether to remove |
I guess I'm still confused how this interacted with the |
Yeah. Me too. Here is more data that may or may not help with the confusion. Please don't ask me to do a full factorial experiment! Before #1044 (on commit 0b133c0: no
Still on commit 0b133c0, but after adding
On #1044 (both
Still on #1044, but
|
Getting back to your earlier question... the aggregator backend seems to have a significant impact on invalidating the cache, if not being the only thing. Do you agree? |
Does it mean that compiling the aggregator to x86 and then wasm is what is causing the issue? |
Apart from the cache issue, I think we should at least split out the crates so that the runtime / loader have a dedicated Cargo.lock file, which is the list of dependencies that are actually part of the TCB. Specifically, I think this would mean trimming down this list so that it only contains Lines 1 to 10 in a2f74a4
@rbehjati does this make sense? |
I agree. I'll make After this we can perform a thorough analysis to understand what is contributing to the invalidation of the cache. |
Note this is still an issue, try and run the following command twice in a row: cc @ipetr0v |
@rbehjati if possible, let's try to build an understanding of what's happening, rather than just trying to get the numbers down. My theory (not tested) is still that some dependency has a feature flag that causes it to be compiled differently in wasm vs x86, and / or has optional dependencies that cause part of the cache to go missing in one case. |
Your theory seems consistent with my observation that specifying the target helps. From what I can see the change in #1179 solves the problem with Apparently, Getting back to your theory, is it expected that everything should be compiled the same for wasm and x86? In other words, do we have a requirement for excluding anything that compiles differently for different architectures? |
Thanks for putting together #1179, AFAICT it is doing two things at once:
Which one is actually having the desired effect? Or is it really the combination of |
For reference, I think this is pretty much what I thought it was happening: https://stackoverflow.com/questions/60869985/why-is-cargo-build-cache-invalidating Though it does not really explain why #1179 actually makes things work correctly 😅 |
I have not seen
I think when
The difference between this case and the previous case is that everything that is now in
In this case nothing is shared between the
|
So is |
It is the same. The following command (source) gives the default target (which is used when
On my linux machine, and our docker image, the output from this command is
This is also the target specified in |
In that case, does it mean that we actually don't need to specify |
I don't think so. If we don't specify |
I have been digging deeper into this and here are some of my findings. The following is the list of packages that are recompiled when running
After this, running Each of these folders has the following content:
The binary files Corresponding to each of the folders, there is another folder inside
These seem to be the output from some I am still not entirely sure how
More specifically it is advised that:
We set |
For now we are happy with the solution using |
To reproduce:
cargo build --release --target=wasm32-unknown-unknown --package=aggregator
Finished release [optimized] target(s) in 43.39s
(slow)cargo build --release --target=wasm32-unknown-unknown --package=aggregator
Finished release [optimized] target(s) in 0.14s
(fast)cargo build --release --package=aggregator_backend
Finished release [optimized] target(s) in 1m 15s
(slow)cargo build --release --package=aggregator_backend
Finished release [optimized] target(s) in 0.15s
(fast)cargo build --release --target=wasm32-unknown-unknown --package=aggregator
Finished release [optimized] target(s) in 43.40s
(expected: fast; actual: slow again)I am guessing that switching compilation target causes some dependencies to be rebuilt (looks like
libc
may be a potential culprit).@rbehjati could you look into it? It will become more and more relevant as we switch to the Rust version of
main
for theoak_loader
, as we will interleave compiling native and Wasm code even more often then.The text was updated successfully, but these errors were encountered: