Skip to content

Conversation

@Radvendii
Copy link
Contributor

Motivation

See this tracking issue for more big-picture motivation.

This change in particular stores small strings that have no context directly in the value payload itself. This reduces memory usage by the string length, and also removes a layer of indirection.

Context


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Radvendii Radvendii requested a review from edolstra as a code owner September 26, 2025 16:46
@Radvendii
Copy link
Contributor Author

I wanted to open this issue so that i can keep track of the work somewhere, but I discussed with @xokdvium and at the moment this doesn't bring any benefit, so I'll shelve it once CI runs

Introduce tSmallString, an InternalType for strings that can fit
directly in the Value payload, while still leaving room for the
discriminant.
@Ericson2314
Copy link
Member

I wanted to open this issue so that i can keep track of the work somewhere, but I discussed with @xokdvium and at the moment this doesn't bring any benefit, so I'll shelve it once CI runs

OK sounds good. I was going to point out that if --- per the issue --- we're shrinking Value to 4 bytes, then this buys us even less / will stop working.

@Ericson2314 Ericson2314 marked this pull request as draft September 28, 2025 01:23
@Radvendii
Copy link
Contributor Author

There's some gcc-specific error (i.e. not happening with clang) causing CI to fail. Given that I'm shelving this for now anyways, It's not worth it to me to fix it. If it turns out to be useful in the future I'll fix it then.

@Radvendii Radvendii closed this Sep 28, 2025
@Radvendii
Copy link
Contributor Author

OK sounds good. I was going to point out that if --- per the issue --- we're shrinking Value to 4 bytes, then this buys us even less / will stop working.

Values payloads will be shrinking to 8 bytes, so it's true we lose a lot of benefit, but it's not as useless as 4 bytes would be.

@Ericson2314
Copy link
Member

Oh yeah, the payload. Right. :)

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.

2 participants