Skip to content

feat: candid subtype check#3171

Merged
mergify[bot] merged 156 commits intomasterfrom
claudio/candid-sub-deser-opt-cache
Jan 25, 2023
Merged

feat: candid subtype check#3171
mergify[bot] merged 156 commits intomasterfrom
claudio/candid-sub-deser-opt-cache

Conversation

@crusso
Copy link
Copy Markdown
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

Comment thread nix/sources.json Outdated
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

crusso commented Jan 25, 2023

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

Comment thread Changelog.md Outdated
@crusso
Copy link
Copy Markdown
Contributor Author

crusso commented Jan 25, 2023

@chenyan-dfinity PTAL at the changelog.

Copy link
Copy Markdown
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