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: composite queries #4003

Merged
merged 41 commits into from Jun 30, 2023
Merged

feat: composite queries #4003

merged 41 commits into from Jun 30, 2023

Conversation

crusso
Copy link
Contributor

@crusso crusso commented May 25, 2023

Add new form of composite query that:

  • can call other queries and composite queries
  • can't be called from updates
  • can't do async expressions (no way to queue up a callback since state is lost)
  • can't call local async or async* (because those might contain update calls)

From the (informal) doc:

Composite query functions

Although queries can be fast, when called from a frontend, yet trusted though slower, when called from an actor, they are also limited in what they can do. In particular, they cannot themselves issue further messages, including queries.

To address this limitation, the Internet Computer supports another flavour of query function called a composite query. Like plain queries, the state changes made by a composite query are transient, isolated and never committed. Moreover, composite queries cannot call update functions, including those implicit in async expressions (which require update calls under the hood). Unlike plain queries, composite queries can call query functions and composite query functions, on the same and other actors, but only provided those actors reside on the same subnet.

As a contrived example, consider generalising the previous Counter actor to a class of counters. Each instance of the class provides an additional composite query to sum the values of a given array of counters:

actor class Counter () {

  var count = 0;

  // ...

  public shared query func peek() : async Nat {
    count
  };

  public shared composite query func sum(counters : [Counter]) : async Nat {
    var sum = 0;
    for (counter in counters.vals())  {
      sum += await counter.peek();
    };
    sum
  }

}

Declaring sum as a composite query enables it call the peek queries of its argument counters.

While update message can call plain query functions, they cannot call composite query functions. This distinction, which is dictated by the current capabilites of the IC, explains why query functions and composite query functions are regarded as distinct types of shared functions.

Note that the composite query modifier is reflected in the type of a composite query function:

  sum : shared composite query ([Counter]) -> async Nat

Since only a composite query can call another composite query, you may be wondering how any composite query gets called at all? The answer to this chicken-and-egg problem is that composite queries are initiated outside the IC, typically by an application (such as a browser frontend) sending an ingress message invoking a composite query on a backend actor on the IC.

c.f. dfinity/interface-spec#144

  • positive tests
  • negative tests
  • RTS support (subtyping, IDL validation etc)
  • doc
  • gc (to GC or not at end of each message)? We currently do a (redundant?) GC for composite queries
  • imports
  • inspect_message support? Since composite queries can't be called as updates, perhaps they don't matter at all?
  • I had to modify cycles adding and refund logic to avoid calling ic0.call_cycles_add128/ic0.cycles_refunded128 unless necessary - these aren't supported for composite query callbacks (see spec).
  • More accurate Lifetime modelling - we currently reuse state Lifecycle.InUpdate since callbacks are always in this state (and changeing that statically is tricky without introducing new forms of callback for CompositeQueries. Adding a new state would allow us to suppress GC at the end of each message, if desired, and more elegantly rule out forbidden ops like adding non-zero cycles, observing refunds. Of course, we could also just set a global to disable these until all state-changes are aborted at the end. Might be simpler.
  • regenerate ok test output once ic-ref available again by merge with master. At the moment, I can manually run the new tests, but can't accept their output. They pass, trust me.
  • better error messages, distinguishing capability (send update, send query, send composite query, perhaps) required.
  • enable ic-ref tests (once ic-ref supports composite queries)
  • add keyword composite to idl, local highlight.js

@github-actions
Copy link

github-actions bot commented Jun 12, 2023

Comparing from c09b751 to aaa9cfc:
In terms of gas, 3 tests regressed, 1 tests improved and the mean change is -0.0%.
In terms of size, 4 tests regressed and the mean change is +0.0%.

doc/md/examples/grammar.txt Outdated Show resolved Hide resolved
@crusso
Copy link
Contributor Author

crusso commented Jun 28, 2023

@rvanasa added you as a reviewer just so you are aware of what needs to change for VSCode, if anything (there's a new keyword composite that can appear in several places (see grammar.txt in PR)

@@ -1264,7 +1272,10 @@ The argument type of `inspect` depends on the interface of the enclosing actor.

- `arg : Blob`: the raw, binary content of the message argument;

- `msg : <variant>`: a variant of *decoding* functions, where `<variant> == {…​; #<id>: () → T; …​}` contains one variant per shared function, `<id>`, of the actor. The variant’s tag identifies the function to be called; The variant’s argument is a function that, when applied, returns the (decoded) argument of the call as a value of type `T`.
- `msg : <variant>`: a variant of *decoding* functions, where `<variant> == {…​; #<id>: () → T; …​}` contains one variant per `shared` or `shared query` function, `<id>`, of the actor
(`shared composite query` functions are ignored).
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that inspect is not called for composite queries? Or is the message argument for shared composite query simply mapped to shared query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inspect is indeed not called for composite queries (in my understanding) so we don't even include them in the variant type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, even for querie method, inspect is only called if the method is invoked as an (ingress) update call (with consensus). It's not used for the faster query calls (no-concensus). Very confusing.

Since composite query methods can't be called with (ingress) update calls, just (ingress) query calls, there is no need to include them here.

shared stable switch system throw to_candid true try type var while with

actor and assert async async* await await* break case catch class
composite continue debug debug_show do else flexible false for
Copy link
Contributor

Choose a reason for hiding this comment

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

Including composite as reserved keyword impacts backward compatibility (former identifiers named composite become invalid). This would technically be a breaking change, isn't it?

Copy link
Contributor Author

@crusso crusso Jun 29, 2023

Choose a reason for hiding this comment

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

Yes, it is, but probably won't break many. Maybe we should bump to 0.10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative would be to use composite_query (ugly IMO) or query* (confusing, subtle IMO)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think composite query is much nicer to read. (Another possibility would be to introduce non-reserved keywords but the complexity is not worth it.) I would keep the current syntax.

@@ -2022,6 +2036,17 @@ The context type is a record to allow extension with further fields in future re

:::

Shared functions have different capabilities dependent on their qualification as `shared`, `shared query` or `shared composite query`.

A `shared` function may call any `shared` or `shared query` function, but no `shared composite query` function.
Copy link
Contributor

Choose a reason for hiding this comment

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

I heard that update calls cannot invoke composite queries. Just for curiosity: Is it a technical reason? Or a conceptual aspect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When an update calls a query, the query is actually executed in replicated mode for concensus on the result (state change is still discarded).
Similary, inspect message is only called for query functions that are executed in update mode (with concensus), not ordinary fast queries.
I suspect that replicated composite queries are much harder to implement and haven't been tackled yet, and might not be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.

All varieties of shared functions may call unshared functions.

Composite queries, though composable, can only be called externally (from a frontend) and cannot be initiated from an actor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should mention that each composite query call rolls back its state on function exit, and also does not pass state changes to sub-query or sub-composite-query calls. Therefore, repeated calls (which includes recursive calls) have different semantics than in classical procedural execution (like with update calls).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, I was shying away from this ;->

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, we could also just reference the general design description of that feature, because it is not a Motoko-specific aspect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some clarification, plus an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great explanation!

@@ -7910,6 +7918,8 @@ module FuncDec = struct
Lifecycle.trans env Lifecycle.Idle
| Type.Shared Type.Query ->
Lifecycle.trans env Lifecycle.PostQuery
| Type.Shared Type.Composite ->
Lifecycle.trans env Lifecycle.Idle
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check: The subsequent state of the lifecycle is Idle and not PostQuery for composite queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is an unfortunate hack. For queries, you can just generate the atomic body and enter PostQuery. But composite queries are not atomic, so you need to enter some state after every callback. Unfortunately, the code for generating callbacks always enters InUpdate and its hard to make the transition conditional.
I couldn't see any (easy) way to enter a different state without complicated the rest of the code to track whether we are in an ordinary callback or a composite callback. I could may set and test a global but that seemed gross too.

As far as I know, the Lifecycle stuff is just for sanity checking, so I think we are just losing some accuracy here.

| C.ErrorCap
| C.QueryCap _
| C.NullCap ->
| C.NullCap
| _ ->
error env at "M0047"
"send capability required, but not available (need an enclosing async expression or function body)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, the error message is not very specific if the call between the modes update/query/composite query is not supported. But this is only cosmetics, one could always polish error messages...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was considering revising those (maybe splitting send capability into update query and composite query capability.
Maybe this is the right time to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is nothing important, could be also done later. I guess we can do that as part of an error message improvement work (together as a team across the entire compiler).

Copy link
Contributor

@luc-blaeser luc-blaeser left a comment

Choose a reason for hiding this comment

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

I like that Motoko offers composite query calls with static checking.
Nice compiler implementation and documentation of this feature.

@crusso
Copy link
Contributor Author

crusso commented Jun 29, 2023

@luc-blaeser, if you could take another (quick) look, I've improved the error messages (a bit) and added some more doc about side-effects.

contrecoeur indeed.

doc/md/language-manual.md Outdated Show resolved Hide resolved
@luc-blaeser
Copy link
Contributor

@luc-blaeser, if you could take another (quick) look, I've improved the error messages (a bit) and added some more doc about side-effects.

Very clear explanation. I would say it is ready to merge...

```

because none of the local updates to `state` are visible to any of the callers or callees.

Copy link
Contributor

@ggreif ggreif Jun 30, 2023

Choose a reason for hiding this comment

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

Maybe add a note that composite queries cannot be run in replicated mode (as opposed to normal queries).
Line 1290 has it.

src/idllib/compile_js.ml Outdated Show resolved Hide resolved
@crusso crusso added the automerge-squash When ready, merge (using squash) label Jun 30, 2023
@mergify mergify bot merged commit 2d9902f into master Jun 30, 2023
8 of 9 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jun 30, 2023
@mergify mergify bot deleted the claudio/ICQC branch June 30, 2023 12:21
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.

None yet

3 participants