smithy4s icon indicating copy to clipboard operation
smithy4s copied to clipboard

Improve cache in code generation

Open majk-p opened this issue 10 months ago • 1 comments

Solves part of #1495

PR Checklist (not all items are relevant to all PRs)

  • [ ] Added unit-tests (for runtime code)
  • [ ] Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • [ ] Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • [ ] Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • [ ] Updated dynamic module to match generated-code behaviour
  • [ ] Added documentation
  • [ ] Updated changelog

majk-p avatar Apr 22 '24 14:04 majk-p

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 22 '24 14:04 CLAassistant

Don't forget to sign the CLA

Baccata avatar Apr 24 '24 06:04 Baccata

Since it was requested in Mill's chat room, here is some input regarding Mill's cache. I have to admit, I didn't review the full PR and all comments, but from what I digested, here are my notes:

  • If you return a PathRef that points to somewhere outside of Mill's cache, there is a chance Mill will miss it's later invalidation due to removal or change. Therefore in Mill 0.11 we introduced the revalidate flag which you can enable with PathRef.withRevalidateOnce. When enabled and before re-using a targets cache, Mill will re-calculate the PathRef.sig and will only use the cached value if the sig is still identical. I think you should use this flag since you don't have exclusive control over the output directory.

  • Using an T.input or a PathRef in general for a path you intend to change from a downstream target, will most likely cause unnecessary re-evaluation. Since you intend to generate something into that path, it's sig will change. If you're not interested in the content of a path, which is the case for config values, the path itself should be enough.

lefou avatar Apr 24 '24 19:04 lefou

LGTM (@Baccata, you haven't committed here so I'll leave it for you to approve if, it looks OK now)

kubukoz avatar May 17 '24 21:05 kubukoz