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

Handler runtime improvements #282

Open
wants to merge 26 commits into
base: hkmc2
Choose a base branch
from

Conversation

AnsonYeung
Copy link

@AnsonYeung AnsonYeung commented Feb 13, 2025

Changes:

  • Moved handler runtime function to Runtime.mls
  • Massive rewrite of handler runtime logic, simplifies it a lot, removes the need for fixup code and matches closer to what is described in the report. I have some further improvement in mind which should allow some program that cannot be run currently to work fine. (Update: done)
    • Namely, resume function will be able to be used entirely outside of the handler, experimentation still need to be done to verify what I have in mind is correct.
  • Improvement of debuggability of handler runtime, via generation of class name with source code line info so that the continuation can be traced back to the belonging code and also added a pretty printer.
  • Removal of double underscore in name
  • Enable tail call optimization in stack safety, this removes the need of continuation class in the handler and reduces code size. (Note: tail call optimization != optimizing tail resumptive handlers)

Problem:
I added Runtime module in decl/Prelude.mls just to get the class symbols. I think this is not correct as it cause Runtime to pass elab but I don't know how to get the class symbols otherwise. (Runtime.mls has dependence on these classes, and Predef.mls imports Runtime.mls, so I don't think these classes can go to Predef?)

  • Fixed by shadowing Runtime during elab

TODO:

  • Right now the continuation class rely on Origin, which turns out to emit absolute path in MLsCompiler.scala. It should not use full path.
  • There is some test diff that I missed when I committed the change. Need to address these issues.
  • Implement further improvment mentioned above
  • Move enterHandleBlock to Predef (as it's intended to be used by programmers directly) and document its semantics there
  • Add a tests mirroring the examples at https://ocaml.org/manual/5.3/effects.html, most notably the fork/yield/xchg one

Test diffs:

  • UserThreads*.mls: these tests are malformed, and the new implementation throws Error on them
  • LeakingEffects.mls: the new test output matches with intended semantics
  • ZCombinator.mls: the stack depth changes because of Runtime.stackDepth is a selection which gets sanity checks. The sanity check moves the selection before the stack depth is increased for the function call console.log.

Follow-up:

  • Decouple non-local return from HandlerLowering
  • HandleBlock can be separated as instantiation of the handler and binding of the handler, this may work better with IR and syntax changes.
    • e.g. have handler as a normal let declaration syntax allowing handler method, and have a handler binding syntax which behaves the same as the current enterHandleBlock
    • Current JS includes 2 key constructs:
      • The handler method in HandlerLowering is lowered to (...handlerArgs) => return mkEffect(h, (k) => handlerMtdBody);
      • The handler body is essentially enterHandleBlock(h, () => handlerBody)
    • So in the syntax, we only need to ensure that h and k is clear in the handler method, i.e. we can even have standalone handlers like
      let h = new Object()
      handler[h, k] fun raise(x) = k(x)
      handle h in raise(3)
      
      Note: h is a value, but k is binded as param, so it is kind of awkward. Also for type checking this means you retroactively add behaviour to h which is not great and probably won't work.
      while awkward, Object are not essential for codegen to work, h can be anything that can be equality checked, i.e. everything in JS is technically fine (but this is kind of awkward and might not work well with type checking).
      handler[123, k] fun raise(x) = k(x)
      handle 123 in raise(3)
      
      So we can also have a relatively minor adjustment to the current syntax and get (currently I prefer this syntax)
      let h = handler with
        handler fun raise(x)(k) = k(x)
      handle h in raise(3); 4
      handle h in raise(3); "abc"
      
      This syntax allow normal methods and other class members, and behave like an object except occurence of handler fun inside it is legal.
      It might be worthwhile to add yet another keyword for handler fun for clarity and conciseness.

Comment on lines 33 to 47
// LIFO
handle h = ThreadEffect with
fun fork(thread)(k) =
this.tasks.unshift(thread)
k(())
fun yld()(k) =
this.tasks.push(k)
globalThis.eval("runtime").enterHandleBlock(this, () => this.tasks.shift()(this))
fun start(main)(k) =
this.tasks.push(main)
while this.tasks.length != 0 do
globalThis.eval("runtime").enterHandleBlock(this, () => this.tasks.shift()(this))
k()
in
h.start(main)
Copy link
Author

Choose a reason for hiding this comment

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

This is needed, because handler method themselves cannot throw effect of the handler itself. So here it just rebinds the handler and invoke it. Previously, the handler is still binded during the handler method with some weird semantics as the order of which handler is resumed is simply very messy (refer to RecursiveHandler.mls L56-79). This should be the correct behaviour now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait but that looks like major shift in the official semantics of effect handlers. I'm afraid it might make our support for the feature quite atypical/weird and not aligned with the standard definitions from the literature.

This seems like a significant design decision that should be properly discussed. Could you start by specifying what's the intended semantics of enterHandleBlock ? It doesn't seem to appear anywhere in your project report.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I just saw your other comment:

The semantic of that function is simply entering a handle block with handler h binded, except as opposed to handle block, it does not create the handler h

  1. Are you saying this is a way of obtaining deep handler semantics in an otherwise shallow handler implementation?

  2. Could you please give examples of what happens if you forget to do it?

  3. Why not implicitly wrap all handler blocks inside this call? Would that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

  1. the handler is still deep, you can still raise effect multiple times in the handler body but not inside the actual handler. The actual handler method cannot raise effects.
  2. This will be throwing error "Unhandled effect" as if the effect leaks.
  3. Yes, this is possible. A handle block is simply let h = new Handler(); runtime.enterHandleBlock(h, () => handlerBody);

Copy link
Author

Choose a reason for hiding this comment

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

So what happens here is this: because this.tasks contain some function that will still raise effect due to .fork() just adding those function directly, those added by .yld won't raise effects

@LPTK
Copy link
Contributor

LPTK commented Feb 15, 2025

  • Some illustrative test cases to add to the tests
:re
handle h = Object with
  fun write(x)(k) =
    if x < 10 do write(10)
    [x, k(())]
h.write(1)
h.write(2)
//│ ═══[RUNTIME ERROR] Error: Unhandled effects

handle h = Object with
  fun write(x)(k) =
    if x < 10 do runtime.enterHandleBlock(this, () => write(10))
    [x, k(())]
h.write(1)
h.write(2)
//│ = [1, [2, ()]]

@AnsonYeung AnsonYeung marked this pull request as ready for review February 15, 2025 14:54
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.

3 participants