SoA improvments part 2 #7163
SoA improvments part 2 #7163
Conversation
| /// Push to a std::vec::Vec<T> and then return an index to that new element's | ||
| /// position in the Vec. | ||
| /// | ||
| /// This is not a method on soa::Index because the `soa` is `no_std` by design. |
There was a problem hiding this comment.
Since we want to incentivize low-allocation collection usage and soa is no_std, we should consider making this a method on soa::Index for bumpalo::Vec since bumpalo is no_std by default and should be a good long-term standard to follow.
There was a problem hiding this comment.
I have a custom bumpalo alternative I want to use with this, but haven't gotten it ready for PR yet!
| @@ -540,6 +540,7 @@ pub fn try_run_lib_function<T>( | |||
| } | |||
| } | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
Is the plan to use this code later, so we are avoiding removing it even if it's dead? This seems like a recipe for dead code falling through the cracks, though I'm sure it would get cleaned up eventually.
There was a problem hiding this comment.
Some of these test helpers are considered dead code when they get imported by tests in a tests/ directory (which are treated as their own crates) but the test doesn't use all of them. So they aren't dead in the sense that if you take them out, some tests (but not others) will no longer compile, but depending on the test that's running, they can generate annoying warnings about dead code.
There's probably a better way to do this, but until we know what it is, this is the quick fix. 😄
This reverts commit c79d774.
| @@ -1,5 +1,9 @@ | |||
| #![cfg_attr(not(any(debug_assertions, test)), no_std)] | |||
There was a problem hiding this comment.
Why no std? Are you going to pass an allocator in explicitly?
There was a problem hiding this comment.
Yeah, for new things I'm trying to use no_std as much as possible because for the caching strategy to work properly, it's crucial that all allocations happen in the arena so they can be serialized to/from disk trivially. no_std is an effective way to make sure the various passes aren't accidentally using the global allocator.
| ], | ||
| space_after: [ | ||
| Slice(start = 0, length = 1), | ||
| Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 1 }, |
There was a problem hiding this comment.
Not directly related to this PR, but I see a couple problems with the snapshot in this way:
- Marking indexes/slices doesn't tell you much other than how the implementation works. I don't think this is useful for snapshot tests. It's probably better to print a debug representation of the AST that is materialized, or just print back the program fully and make sure it looks reasonable. (If there's ever a source of non-determinism in the parsing, e.g. parallel parsing, the indexes are also going to be non deterministic)
- Embedding the type names is going to create a big diff every time there's a refactor, like is done here. Probably worth avoiding that.
There was a problem hiding this comment.
Yeah that's fair - I took the type name back out 👍
Co-authored-by: Ayaz <[email protected]> Signed-off-by: Richard Feldman <[email protected]>
Co-authored-by: Ayaz <[email protected]> Signed-off-by: Richard Feldman <[email protected]>
| @@ -1054,8 +1051,7 @@ fn insert_tags_fast_path( | |||
| let variable_slice = | |||
| register_tag_arguments(env, rank, arena, types, stack, arguments_slice); | |||
|
|
|||
| let new_variable_slices = | |||
| SubsSlice::extend_new(&mut env.subs.variable_slices, [variable_slice]); | |||
| let new_variable_slices = slice_extend_new(&mut env.subs.variable_slices, [variable_slice]); | |||
There was a problem hiding this comment.
super nitpick: this should use slice_push_new
There was a problem hiding this comment.
We don't actually have that in the API, but maybe we should!
Follow-up to #7156