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

Better-typed string context #4124

Open
Ericson2314 opened this issue Oct 8, 2020 · 7 comments
Open

Better-typed string context #4124

Ericson2314 opened this issue Oct 8, 2020 · 7 comments

Comments

@Ericson2314
Copy link
Member

The string context current is made up various patterns like !out!/path/to/drv and =/path/to/drv. Are these encodings intrinsically important, or could we replace with a std::variant?

CC @edolstra

@Ericson2314 Ericson2314 added the bug label Oct 8, 2020
@edolstra
Copy link
Member

edolstra commented Oct 8, 2020

They're not intrinsically important but they avoid another allocation.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Oct 8, 2020

@edolstra are you worried about indirection or just having multiple strings? I was thinking something like:

// probably exists already
struct StorePathWithOutput {
    StorePath path;
   std::string output;
}; 

// is that what the `=` thing means?
struct Closure {
    StorePath path;
};

typedef std::variant<StorePathWithOutput, Closure> StringCtxItem;

so more strings with things split, but not more indirection since the variant is stored directly in the set. This seems fine to me?

@edolstra
Copy link
Member

edolstra commented Oct 9, 2020

Currently contexts are stored in Value::string as a const char * *, which is one heap allocation for the array and one allocation per context string. If we changed this to a const StringCtxItem *, then we get an additional heap allocation for each StorePath::baseName and maybe another heap allocation for output.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Oct 9, 2020

So we have a const char[][] (trying to make the arrays clear) and are going to a const StringCtxItem[]. That means before we had heap allocs for:

  • each context item, a string context item []
  • the context, an array of strings const char [] []

Now we have them for:

  • std::strings/StorePaths of each item (drv path, output, etc)
  • the contex, an array of variants const StringCtxItem []. Note the the variants are stored directly in the array.

So what exactly is worse about the second option?

  • there's no more indirection
  • the context array is bigger
  • the items are smaller but more numerous
  • the total allocated bytes should be roughly the same

Are you just worried about the constant factor space and time overhead of fine-grained allocations?

@stale
Copy link

stale bot commented Apr 10, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Apr 10, 2021
@stale
Copy link

stale bot commented Apr 19, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this as completed Apr 19, 2022
@Ericson2314 Ericson2314 reopened this Apr 19, 2022
@stale stale bot removed the stale label Apr 19, 2022
@Ericson2314
Copy link
Member Author

Since #6237 "deocoded" string contexts are better typed, but we should still do something about the interpreter heap original.

@stale stale bot added the stale label Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants