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

Lookup serialization #1111

Merged
merged 9 commits into from
Jul 24, 2023
Merged

Conversation

Nashtare
Copy link
Collaborator

@Nashtare Nashtare commented Jul 6, 2023

This PR updates the way lookup tables are being serialized, and more generally gives access to CommonCircuitData to gates and generators during serialization/deserialization.

This allows:

  • to serialize only once a lookup_table in a given circuit
  • to remove the need for the special DummyCircuitGenerator to be placed first in the GeneratorSerializer object (could have easily been source of errors for external users defining their own gate/generators and needing to implement a custom serializer.

For this, it updates the underlying F type of WitnessGeneratorRef from F: Field to F: RichField + Extendable<D>. I am not sure as to why it was set only to Field in the first place, as the restriction is natural here.

@Nashtare Nashtare changed the title Lookup serial Lookup serialization Jul 6, 2023
@Nashtare
Copy link
Collaborator Author

Nashtare commented Jul 6, 2023

The CI errors are not related to these changes. See #1113 .

@@ -1689,12 +1697,14 @@ pub trait Write {
&mut self,
gate: &GateRef<F, D>,
gate_serializer: &dyn GateSerializer<F, D>,
common_data: &CommonCircuitData<F, D>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

common_data is actually being used in quite a lot of places (and consistently across this file). Do you want me to address this particular case still?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm maybe it should just be called common_data everywhere (instead of cd)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok! Addressed in additional commit.

@Nashtare
Copy link
Collaborator Author

@npwardberkeley I've added another commit on top of what you already reviewed, addressing a point mentioned in #1114 about circuit building which can take quite some time when we are loaded with lookups.
The commit adds a hash (keccak as already present) of the lookup table when defining a new LookupGate / LookupTableGate, and uses a custom implementation of Gate::id() for those two gate types, to ignore the entire table.

For 8192 LookupGates, I am getting a circuit building time of 2.45 sec vs 4.5 sec for the current implementation. Let me know what you think! 🙂

Copy link
Contributor

@npwardberkeley npwardberkeley left a comment

Choose a reason for hiding this comment

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

Looks good!

plonky2/src/util/serialization/mod.rs Show resolved Hide resolved
@npwardberkeley npwardberkeley merged commit bfa7ab3 into 0xPolygonZero:main Jul 24, 2023
2 checks passed
@Nashtare Nashtare deleted the lookup_serial branch July 26, 2023 00:20
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.

2 participants