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

[post frontend-backend split] Bring back thread-safe-region permutation Assembly #258

Closed
ed255 opened this issue Jan 30, 2024 · 2 comments · Fixed by #296
Closed

[post frontend-backend split] Bring back thread-safe-region permutation Assembly #258

ed255 opened this issue Jan 30, 2024 · 2 comments · Fixed by #296

Comments

@ed255
Copy link
Member

ed255 commented Jan 30, 2024

With the frontend-backend split, a Region becomes a frontend abstraction; and the frontend handling of copy constraints is simplified. The thread-safe-region feature was introduced to allow assigning to regions in parallel, and the permutation::keygen::Assembly was reimplemented to support parallel deterministic assembly. Maybe we don't need that now, because the real assembly is done later by the backend? For now I disabled that implementation in order to move forward with the frontend-backend split, we need to revisit this afterwards.

Blocked by #254

@duguorong009
Copy link

@ed255
While working on PR #296 , I found that the thread-safe-region is useful for both frontend & backend.
This is used in:

I think it is a bit confusing(at least for myself) to see the Column, ColumnMid in the backend crate where it is supposed to be frontend-agnostic.
Maybe, do you have any plan for this? Or do you think it is okay?

@ed255
Copy link
Member Author

ed255 commented Mar 13, 2024

@ed255 While working on PR #296 , I found that the thread-safe-region is useful for both frontend & backend. This is used in:

* [SyncDeps](https://github.com/privacy-scaling-explorations/halo2/blob/main/halo2_common/src/circuit/layouter.rs#L15-L27) (used for the parallel region assignments - frontend)

* [permutation::keygen::Assembly](https://github.com/privacy-scaling-explorations/halo2/blob/main/halo2_backend/src/plonk/permutation/keygen.rs#L32-L41) (used for parallel handling of permutations - backend)

I think it is a bit confusing(at least for myself) to see the Column, ColumnMid in the backend crate where it is supposed to be frontend-agnostic. Maybe, do you have any plan for this? Or do you think it is okay?

I'll base my comment on the code after #290

ColumnMid is defined in the middleware and is used in public types that the backend consumes, for example in permutation::ArgumentMid, or in Cell which is used to define the copy constraints. The backend receives this type from the frontend, and since this type has all the information that the backend needs (no transformation is needed), it just keeps using this type. I'm happy with this.

Column is defined in the frontend and is used as part of the frontend public API. It's a generic type that can be instantiated for Fixed, Advice, Instance and Any. Changing this type (even just the name) would break the frontend API, so I left it untouched. I think it's reasonable to keep this type in the frontend because it provides a level of type safety. For example, a method that assigns a fixed column must receive a Column<Fixed>, so if the user passes a Column<Advice> instead the code will not compile. On the other hand, ColumnMid doesn't provide this kind of type safety, but since it's not used to define circuits by the end user it's OK. Note that ColumnMid is very similar to Column<Any>. Apart from that ColumnMid is much more lightweight, for example it doesn't have all the query methods that are only useful in the frontend.

So in summary, Column is a frontend type and ColumnMid is a middleware type that the backend reuses.

I'm happy with this situation and I don't have plans for changing it. Do you have any idea for improving this? Could you give more details about what's confusing?

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 a pull request may close this issue.

2 participants