Skip to content

Switch from Cap'n'proto to Serde for cache#5358

Open
GearsDatapacks wants to merge 5 commits intogleam-lang:mainfrom
GearsDatapacks:capnp-to-serde
Open

Switch from Cap'n'proto to Serde for cache#5358
GearsDatapacks wants to merge 5 commits intogleam-lang:mainfrom
GearsDatapacks:capnp-to-serde

Conversation

@GearsDatapacks
Copy link
Member

Closes #1599 and fixes #5260

  • The changes in this PR have been discussed beforehand in an issue
  • The issue for this PR has been linked
  • Tests have been added for new behaviour
  • The changelog has been updated for any user-facing changes

This PR replaces Cap'n'proto with Serde and Bincode for module caching. I have also merged the cache_inline and cache_module files into the main .cache as I don't see a reason for them to be kept separate anymore.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I love this!!!!

// Write cache file
let bytes = ModuleEncoder::new(&module.ast.type_info).encode()?;
let bytes =
metadata::encode(&module.ast.type_info).expect("Failed to serialise module cache");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slight difference in behaviour here in that previously warnings would not be stored for dependencies, but now they are. I don't think it results in them being shown to the programmer, but it does mean we do a little bit more work to encode and decode them each time.

Probably doesn't matter? What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested and they still are not shown to the programmer with this change. I can remove them from the cache as well though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's worth doing that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, removing warning caching would mean we need to clone the ModuleInterface, as we only have an immutable reference here. Not sure if that's a worthwhile tradeoff

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we mutate it instead? Swap the vector for an empty one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are only given an immutable reference, so we cannot mutate it without cloning

@lpil lpil marked this pull request as draft February 25, 2026 18:25
@GearsDatapacks GearsDatapacks marked this pull request as ready for review February 25, 2026 21:36
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.

Metadata decoder error: "Message is too deeply-nested" when using deeply nested constants Switch from capnproto to serde

2 participants