Skip to content
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

Cache templates compiled by proc-macro #125

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

w-flo
Copy link
Contributor

@w-flo w-flo commented Jun 21, 2023

I'm putting this up as a draft because I'm not convinced I can test it sufficiently. I have tested it a bit and it seems to work for my use case and fixes #58.

Possible disadvantages / risks:

  • Files are never deleted from the cache (but cargo clean should drop all of them, right?)
  • Not sure if the data used for the hash is specific enough, maybe more needs to be taken into account?
  • A bunch of unwrap()s :-) Some of them might be problematic, I'm not quite sure which ones though

Instead of recompiling sailfish templates on every proc-macro invocation, see if an output file for the same input file content + compiler config combination already exists.

This also fixes #58 because it avoids multiple proc-macro invocations writing to the same output file at roughly the same time from different processes, for example in clippy checks that execute in parallel.

In addition to the compiled output, now the list of dependencies (file names), which was previously generated by the compiler for each template on every proc-macro invocation, needs to be stored to disk for re-use.

Instead of recompiling sailfish templates on every proc-macro
invocation, see if an output file for the same input file content +
compiler config combination already exists.

This also fixes rust-sailfish#58 because it avoids multiple proc-macro invocations
writing to the same output file at roughly the same time from different
processes, for example in clippy checks that execute in parallel.

In addition to the compiled output, now the list of dependencies (file
names), which was previously generated by the compiler for each template
on every proc-macro invocation, needs to be stored to disk for re-use.
@w-flo
Copy link
Contributor Author

w-flo commented Jun 21, 2023

Also, I don't handle the case when the sailfish-compiler crate is updated. Of course, files compiled by previous versions should no longer be used in that case. I assume that's not an issue because the new crate version would automatically get a new "OUT_DIR" environment variable?

@w-flo w-flo marked this pull request as ready for review July 27, 2023 12:46
@vthg2themax
Copy link
Collaborator

@w-flo @djarb @imbolc @botika @Kogia-sima I will be merging this pull request, and then working on trying to get the rm_whitespace test working correctly.

@vthg2themax vthg2themax merged commit 2265829 into rust-sailfish:main Jul 28, 2023
1 of 9 checks passed
@w-flo
Copy link
Contributor Author

w-flo commented Jul 28, 2023

Thanks @vthg2themax! Feel free to ping me if you find / suspect there are new issues caused by the PR later on.

I suspect upgrading to proc_macro2 1.0.66 could fix the rm_whitespace test. It includes this PR which looks related. Currently sailfish uses 1.0.56.

@vthg2themax
Copy link
Collaborator

After running the tests again, the issue has disappeared. Bonus!
Thanks everyone!

ggwpez added a commit to ggwpez/substrate-weight-compare that referenced this pull request Jul 31, 2023
Use fix from rust-sailfish/sailfish#125

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez added a commit to ggwpez/substrate-weight-compare that referenced this pull request Jul 31, 2023
Use fix from rust-sailfish/sailfish#125

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expected expression, found <eof>
2 participants