Skip to content

Conversation

@gabriel-barrett
Copy link
Contributor

Needed refactorization for #372. The changes are:

  1. Ctrl::Choose will now match field elements onto indices of the list of unique branches (its third argument), instead of matching onto a duplicated branch. Execute and trace generation will now need to match onto the index and use the index to jump to the branch. That also means that the default case is also an index and thus the default branch has been added to the list of unique branches, which simplifies eval and compute_layout_sizes a bit.
    Doing this indirection is only a slight penalty, not really noticeable, and it is basically the same cost as using Arc, which is also another layer of indirection (though I believe the indices will be better for cache hits).

  2. generate_trace has been refactored to remove duplicated work, get_func_range has also been refactored slightly

  3. I've added the query_index to Record basically for identification of which coroutine has done the lookup. Nonces are not unique, thus they do not completely identify the caller. Chipset has also included the query index (i.e. which coroutine it is embedded in) so that it properly records the identification. Byte lookups have also included the index.
    Note: we could later perhaps parametrize this query_index field so that it accounts for whatever metadata is needed. Perhaps shard indices will also become important

Copy link
Contributor

@arthurpaulino arthurpaulino left a comment

Choose a reason for hiding this comment

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

Thanks a lot!
This is a much smoother transition towards #372

pub struct Record {
pub nonce: u32,
pub count: u32,
// Original query that did the lookup. `None` is for the root lookup
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: please turn this into a docstring

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.

4 participants