fix: check thread-safe-region feature(post-split)

[duguorong009]

Description

• check & re-enable the temporary-disabled thread-safe-region feature usage across the crates

Related issues

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

Changes

• remove the thread-safe-region feature & related code for halo2_backend::plonk::keygen::Assembly struct
• remove the thread-safe-region-related code in MockProver(halo2_frontend::dev)
• derive the Ord, PartialOrd for halo2_middleware::circuit::Cell struct
• add the sorting of permutation copies in compile_circuit(halo2_frontend) process

[davidnevadoc]

Even though the note in the code says that the thread-safe feature should be moved to the frontend, it must be kept in both front and back because in the front is needed for the assigning regions in parallel, and in the back it is needed for the assambly keyen. Is this right?

[duguorong009]

Even though the note in the code says that the thread-safe feature should be moved to the frontend, it must be kept in both front and back because in the front is needed for the assigning regions in parallel, and in the back it is needed for the assambly keyen. Is this right?

Yes, you are right. @davidnevadoc
#258 (comment)

[ed255]

I want to discuss this feature.

For context:

• Before the frontend-backend split, the way copy constrains worked is that for every copy constraint you include (by specifying 2 cells), the permutation::Assembly keeps growing a structure that holds the cycles (and they are updated in place). The order of elements in the cycle depend on the order of including the copy constraints, and this affects the setup keys.
  • This means that if you try to do assignment in parallel where you include copy constraints in parallel, the order of the copy constraints is not deterministic, and thus the cycles are not deterministic. Nevertheless this couldn't be done because Region was not Send + Sync, so you couldn't access a Region from different threads; and you need a Region to include a copy constraint.
• In feat: send sync region #180 we got the feature thread-safe-region. On one hand this makes the Region: Send + Sync, which allows it to be shared across different threads. But then the order of the copy constraints is not deterministic. For that reason, that PR also includes a new implementation of permutation::Assembly that regenerates the cycles before keygen such that the cycles are always deterministic (no matter the original order).
• With the frontend-backend split, the cycle generation only appears in the backend. Now the frontend just keeps a list of pairs of cells. Then in keygen, the backend receives this list, and builds the cycles in a single step:
  

  halo2/halo2_backend/src/plonk/permutation/keygen.rs

  Lines 45 to 55 in d6f7020

  	pub(crate) fn new_from_assembly_mid(
  	n: usize,
  	p: &ArgumentV2,
  	a: &AssemblyMid,
  	) -> Result<Self, Error> {
  	let mut assembly = Self::new(n, &p.clone().into());
  	for copy in &a.copies {
  	assembly.copy(copy.0.column, copy.0.row, copy.1.column, copy.1.row)?;
  	}
  	Ok(assembly)
  	}

So, while I think the current PR is correct and brings back the thread-safe-region (which I agree, needs to be in the frontend to have thread-safe Region, and in the backend to have deterministic permutation keygen); I believe that with the current frontend-backend split there's a much easier way to achieve the same:
Before this step:

halo2/halo2_backend/src/plonk/permutation/keygen.rs

Lines 51 to 53 in d6f7020

	for copy in &a.copies {
	assembly.copy(copy.0.column, copy.0.row, copy.1.column, copy.1.row)?;
	}

Sort a.copies.

And with that we would have the same outcome without the need of two implementations of permutation::Assembly.

What do you think about this?

There's an extra thing that we can look into: since now the cycles are generated in one pass (instead of being updated in-place), maybe we can find a simpler implementation for that.

[duguorong009]

Thanks for detailed comment! @ed255
I think you are right.
We can make this PR better.

I think we should come back to this PR after #290 is completed.
Then, we can discuss the feature in cleaner codebase.

[CPerezz]

I'm confused. The PR says it enables the feature but the actual PR removes the feature.
If we want to actually remove it (which I don't think we should (without discussing first)), we should also remove the docs about it that we have in Readme and MDBook.

[ed255]

There's one question that comes to my mind: should we add #[cfg(feature = "thread-safe-region")] before sorting the copies?

Without that feature, the regions are not thread safe so the copies should always be stable; so technically the sorting is only needed when the tread-safe-region feature is enabled. If we add the line before sorting, we'll skip sorting when it's not needed, which may reduce the overhead?

Now this only makes sense if the overhead of the sorting is significant, but I don't know how significant it is.

[ed255]

I'm confused. The PR says it enables the feature but the actual PR removes the feature. If we want to actually remove it (which I don't think we should (without discussing first)), we should also remove the docs about it that we have in Readme and MDBook.

The approach that the PR implements has changed since it was opened. In particular, since we now hold all the copies in a vector instead of building the cycles incrementally, we don't need a stable incremental cycle generation algorithm, because we can just sort the entries in the copies. For this reason the backend doesn't need the thread safe feature, only the frontend (which is where the Region exists).

So this PR is not removing any "capability".

[duguorong009]

There's one question that comes to my mind: should we add #[cfg(feature = "thread-safe-region")] before sorting the copies?

Without that feature, the regions are not thread safe so the copies should always be stable; so technically the sorting is only needed when the tread-safe-region feature is enabled. If we add the line before sorting, we'll skip sorting when it's not needed, which may reduce the overhead?

Now this only makes sense if the overhead of the sorting is significant, but I don't know how significant it is.

I think it is better to add the thread-safe-region feature before sorting the copies.
Sorting idea started from this thread-safe-region feature.
Hence, I believe it makes sense to add feature.
Also, I think the overhead of sorting vector is costly, even in small circuits.

[ed255]

There's one question that comes to my mind: should we add #[cfg(feature = "thread-safe-region")] before sorting the copies?
Without that feature, the regions are not thread safe so the copies should always be stable; so technically the sorting is only needed when the tread-safe-region feature is enabled. If we add the line before sorting, we'll skip sorting when it's not needed, which may reduce the overhead?
Now this only makes sense if the overhead of the sorting is significant, but I don't know how significant it is.

I think it is better to add the thread-safe-region feature before sorting the copies. Sorting idea started from this thread-safe-region feature. Hence, I believe it makes sense to add feature. Also, I think the overhead of sorting vector is costly, even in small circuits.

Sounds good, I think this is the safest option :)

[ed255]

LGTM!

[CPerezz]

Thanks for the explanations! LGTM!