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: parenthetical syntax for cycles, timeout etc. #4608

Draft
wants to merge 136 commits into
base: master
Choose a base branch
from

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Jul 10, 2024

ICCallPrim (and friends) should carry the fragment to set the SystemCyclesAddPrim while the call is being set up towards the replica.

Parentheticals

  • now: (with cycles = 42_000_000) Actor.call(param)
  • later: (with timeout = 10)
    ic0.call_with_best_effort_response : (timeout_seconds : i32) -> ()
  • maybe: (with receiveMax = 50_000) (to limit the response size)
  • maybe: (with resend = true; resendDelay = 3) when SYS_TRANSIENT reject response (like Unix EINTR)
  • support feat(system-api): add call_cycles_add128_up_to ic#1158
  • usecase (with memoryLimit = 1G) ActorClass(<args>)

TODOs:

  • test one-ways
  • typecheck
    • warn when an attribute is moot
    • cycles : Nat for canister sends (self and raw sends too)
  • async blocks
  • ICCallPrim, see top
  • ICCallRawPrim, not directly annotatable (desisting for now)
  • FIXMEs, TODOs
  • warn on queries?
  • best-effort: new error code when deadline passed?

@@ -312,14 +312,16 @@ let funcE name sort ctrl typ_binds args typs exp =
note = Note.{ def with typ; eff = T.Triv };
}

let recordE' = ref (fun _ -> nullE ()) (* gets correctly filled below *)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use and to allow mutual recursion

begin match e1.it, env with
| VarE f1, { tail_pos = true;
info = Some { func; typ_binds; temps; label; tail_called } }
when f1 = func && are_generic_insts typ_binds insts ->
tail_called := true;
(blockE (assignEs temps (exp env e2)) (breakE label (unitE ()))).it
| _,_-> PrimE (CallPrim insts, [exp env e1; exp env e2])
| _,_-> PrimE (CallPrim (insts, pars), [exp env e1; exp env e2])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
| _,_-> PrimE (CallPrim (insts, pars), [exp env e1; exp env e2])
| _,_-> PrimE (CallPrim (insts, exp env pars), [exp env e1; exp env e2])

ggreif added 10 commits July 10, 2024 15:55
unfortunately it doesn't arrive in the `async.ml`
at least now I get
> ingress Err: IC0504: Canister rwlgt-iiaaa-aaaaa-aaaaa-cai violated contract: ic0_call_cycles_add128 called when no call is under construction.
we should use the system call though, instead of
assigning to `@cycles`, as that will go away
@ggreif ggreif changed the title WIP: surface syntax for parentheticals feat: parenthetical syntax for cycles etc. Jul 10, 2024
@ggreif ggreif self-assigned this Jul 10, 2024
@ggreif ggreif added the language design Requires design work label Jul 10, 2024
Copy link

github-actions bot commented Dec 3, 2024

Comparing from 76f9087 to dc37044:
In terms of gas, 1 tests improved and the mean change is -0.0%.
In terms of size, 5 tests regressed and the mean change is +0.0%.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

Some interim comments

@@ -2541,7 +2551,7 @@ module Closure = struct
I32Type :: Lib.List.make n_args I32Type,
FakeMultiVal.ty (Lib.List.make n_res I32Type))) in
(* get the table index *)
Tagged.load_forwarding_pointer env ^^
(*Tagged.load_forwarding_pointer env ^^ FIXME: NOT needed, accessing immut slots*)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is ok? I'd first verify with @luc. Also, not related to this PR at all.

assert env.flavor.has_await;
async env
exp.at
(fun k' r ->
let env' = { env with labs = V.Env.empty; rets = Some k'; throws = Some r }
in interpret_exp env' exp1 k')
k
| AsyncE (Type.Cmp, _, exp1, _) ->
| AsyncE (_, Type.Cmp, _, exp1, _) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| AsyncE (_, Type.Cmp, _, exp1, _) ->
| AsyncE (_FIXME, Type.Cmp, _, exp1, _) ->

Comment on lines +515 to +516
## FOR NOW drun-eop-release = enhanced_orthogonal_persistence_subdir "run-drun" [ moc nixpkgs.drun ] ;
## FOR NOW drun-eop-debug = snty_enhanced_orthogonal_persistence_subdir "run-drun" [ moc nixpkgs.drun ] ;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget to revert

@@ -116,7 +116,7 @@ and lexp' =
all call-by-value. Many passes can treat them uniformly, so they are unified
using the PrimE node. *)
and prim =
| CallPrim of Type.typ list (* function call *)
| CallPrim of Type.typ list * exp (* function call *)
Copy link
Contributor

Choose a reason for hiding this comment

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

I makes me nervous that prim now as subexpressions since we might miss them in traversals. For example, is closure conversion in the backend considering these? It might be safer to put these arguments in the prim argument list.

@@ -167,12 +168,12 @@ and prim =
(* backend stuff *)
| CPSAwait of Type.async_sort * Type.typ
(* typ is the current continuation type of cps translation *)
| CPSAsync of Type.async_sort * Type.typ
| CPSAsync of Type.async_sort * Type.typ * exp
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

| ICPerformGC
| ICReplyPrim of Type.typ list
| ICRejectPrim
| ICCallerPrim
| ICCallPrim
| ICCallPrim of exp option
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -310,7 +310,7 @@ and interpret_exp_mut env exp (k : V.value V.cont) =
| PrimE (p, es) ->
interpret_exps env es [] (fun vs ->
match p, vs with
| CallPrim typs, [v1; v2] ->
| CallPrim (typs, _), [v1; v2] ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, needs fixing

{ e with it = CallE (Some (ObjE (Option.(to_list base0_opt @ to_list base), fs) @? e.at), f, is, args) }
| AsyncE (base0_opt, Type.Fut, binds, exp) ->
{ e with it = AsyncE (Some (ObjE (Option.(to_list base0_opt @ to_list base), fs) @? e.at), Type.Fut, binds, exp) }
| _ -> e (* FIXME: meh, can we emit a warning? *)
Copy link
Contributor

@crusso crusso Dec 5, 2024

Choose a reason for hiding this comment

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

Err - this will just drop the parenthetical on the floor, including its side-effects/non-termination. Would it not be possible to only allow these on calls and asyncs, syntactically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be possible. I'll look into it.

@ggreif ggreif force-pushed the gabor/parentheticals branch from d3085f0 to 57e213b Compare December 6, 2024 11:51
@ggreif ggreif force-pushed the gabor/parentheticals branch from 770d5c5 to a0d475a Compare December 6, 2024 14:42
@ggreif ggreif force-pushed the gabor/parentheticals branch from 0b40189 to 3bed728 Compare December 6, 2024 19:16
@@ -203,5 +203,8 @@ let error_codes : (string * string option) list =
"M0197", Some([%blob "lang_utils/error_codes/M0197.md"]); (* `system` capability required *)
"M0198", Some([%blob "lang_utils/error_codes/M0198.md"]); (* Unused field pattern warning *)
"M0199", Some([%blob "lang_utils/error_codes/M0199.md"]); (* Deprecate experimental stable memory *)
"M0200", Some([%blob "lang_utils/error_codes/M0200.md"]) (* Cannot determine subtyping or equality *)
"M0200", Some([%blob "lang_utils/error_codes/M0200.md"]); (* Cannot determine subtyping or equality *)
"M0202", None; (* parenthetical note must be applied to a message send *)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make this contiguous

let cyclesSetup = if hasCycles
then Some (thenE
(natE Mo_values.Numerics.Nat.zero |> assignVarE "@cycles")
(primE SystemCyclesAddPrim [dotE pars "cycles" T.nat]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bind pars to an object variable? Otherwise we get duplicated effects!

@ggreif ggreif changed the title feat: parenthetical syntax for cycles etc. feat: parenthetical syntax for cycles, timeout etc. Dec 11, 2024
@@ -12187,6 +12215,29 @@ and compile_prim_invocation (env : E.t) ae p es at =
SR.Vanilla, Cycles.refunded env
| SystemCyclesBurnPrim, [e1] ->
SR.Vanilla, compile_exp_vanilla env ae e1 ^^ Cycles.burn env
| ICCyclesPrim, [] ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an idempotent operation, so we have to be careful to not call it twice. E.g. it fails for paired up environment+parenthetical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should return two options. Possibly just the Nat (cycles) and Nat32 (timeout). We definitely have to search the attributes.

Arr.load_field env 1l ^^
G.i (LocalSet (nr 0l))
; Tagged.Object,
Opt.inject_simple env G.nop
Copy link
Contributor Author

@ggreif ggreif Dec 11, 2024

Choose a reason for hiding this comment

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

Don't care storing back anything, as there is no captured environment.

Tagged.branch_with env [I32Type]
[ Tagged.Closure,
G.i Drop ^^
Opt.null_lit env
Copy link
Contributor Author

@ggreif ggreif Dec 11, 2024

Choose a reason for hiding this comment

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

use br 1 to fall out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request language design Requires design work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants