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

feat: candid subtype check #3171

Merged
merged 156 commits into from
Jan 25, 2023
Merged

feat: candid subtype check #3171

merged 156 commits into from
Jan 25, 2023

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Mar 24, 2022

Builds on (in reverse order)

Since this PR includes more test and fixes to previous PR, its probably best to just review this one as a diff against master:

Over #3170, this PR:

  • Introduces a stack allocated memo table allocated on entry to deserialize and shared amongst all recursive calls. The extended boolean flag is replaced by a possibly null memo table (bit_rel_opt) - the table is null for extended deserialization of stable values (for which subtype checks on references are just ommitted since unnecessary).

  • Generalises the subtype check and bitrel cache to support sharing across multiple calls and caching of both positive and negative subtype test results:

  • Adds 1 bit per pair of types in subtype cache to record true/false outcome separately from plain addition to cache.

  • Adjust each exit point that returns false to invalidate the positive subtyping assumption already in the cache (thus recording the negative result in the cache).

Unfortunately caching negative results doubles the space required to store the cache (we now need 2 bits, not 1, for every possible pair of type indices).
This subtyping between type tables with 128 entries each will consume 8K = (2 * 2 * 128 * 128/ 8) of stack space. Not great, but not terrible either.

The first, visited bit (bit 0 per (t1,t2) pair), is stored in unnegated form. The second, related bit (bit 1 per (t1,t2) type pair), is stored in negated form so that assuming the relation holds when first visited is actually a no-op (since the matrix is initialized with zero bits).

Also fixes a lurking bug where I failed to skip the value when the subtype check fails (meaning previous PR were buggy).

  • create the cache once on entry to deserialize and share it between calls to idl_sub
  • avoid explicitly assuming true by defaulting to full relations on entry (perhaps by inverting true and false bits)
  • only produce global type table entries in unextended deserialization. Not worth the effort.
  • add basic test for recursive types
  • overflow check on Stack.dynamic_alloc
  • idl_sub on future types.
  • fix broken re-initialization of bitrel on every sub type check
  • integrate with changes due to to_candid/from_candid Operations to_candid and from_candid as surface syntax. #3155

nix/sources.json Outdated Show resolved Hide resolved
Copy link
Contributor

@ggreif ggreif 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, but it is a lot.

@crusso
Copy link
Contributor Author

crusso commented Jan 25, 2023

@chenyan-dfinity can you check the changelog description makes sense?

Changelog.md Outdated Show resolved Hide resolved
@crusso
Copy link
Contributor Author

crusso commented Jan 25, 2023

@chenyan-dfinity PTAL at the changelog.

Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

LGTM

@crusso crusso added the automerge-squash When ready, merge (using squash) label Jan 25, 2023
@mergify mergify bot merged commit 479b683 into master Jan 25, 2023
@mergify mergify bot deleted the claudio/candid-sub-deser-opt-cache branch January 25, 2023 18:30
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jan 25, 2023
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.

5 participants