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

Experiment: Change Pony errors to use return flags (internally) rather than exceptions #4443

Open
jemc opened this issue Sep 15, 2023 · 2 comments
Labels
enhancement New feature or request needs discussion Needs to be discussed further needs investigation This needs to be looked into before its "ready for work"

Comments

@jemc
Copy link
Member

jemc commented Sep 15, 2023

We are looking at maybe changing the way that Pony errors work internally. Specifically, we are looking at using return flags (internally) rather than exceptions and LLVM invokes.

These changes would be almost entirely invisible to Pony users except in the following ways:

  • The pony_error C function would be removed, and any C code which currently uses it must be refactored use another approach to convey failures to the caller.

    • implied by this: FFI functions would no longer be allowed to be partial, because the only supported kind of partial function is one which uses pony_error to raise a Pony-specific exceptions (C++ exceptions are already not supported today)
    • this is technically a breaking change (though only for C code written for use with Pony), so it must go through the RFC process.
  • Performance may be impacted (for better and/or worse, depending on the application.

    • using a return flag for errors, puts the "did raise an error" case and "didn't raise an error" case on equal footing performance-wise, and it is up to CPU branch prediction (and/or likely/unlikely annotations) to decide to favor one case over the other
    • this means that the error-related overhead for a calling a function which may raise an error, and is not inlined during compilation, may be:
      • slightly slower in the "didn't raise an error" case
      • much faster in the "did raise an error" case
    • implied by this: the current advice in the performance cheat sheet about avoiding raising error in performance-critical code would likely be removed (and possibly replaced by new advice)
    • performance testing should/will be done to give more details on the impact.
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Sep 15, 2023
@jemc
Copy link
Member Author

jemc commented Sep 15, 2023

Note that I have already done this experiment in the Savi compiler, so I am familiar with the areas of code that need to change in the Pony compiler to make this work.

Here are my raw notes that I wrtoe up in a call with @SeanTAllen today where we went over what needs to be done:

  • get rid of pony_error
  • get rid of the ability for an FFI call to be partial
    • the invoke_fun branch of gen_ffi function goes away
  • refactor the FFI stuff that is currently using pony_error
    • in socket.c, we can probably just replace the pony_error calls with return -1
    • in time.c, just don't said the Windows invalid param handler (says Sean, after reading the Windows docs)
  • replace gencall_error with a either a jump to the nearest try block's else, or an "error return" if there is no catching try block in the local scope
    • an "error return" is basically a tuple return where the first element is the "real" return type, and the second element is an int1 (a.k.a. boolean) which represents "error or no error"
  • rename invoke_target in compile_frame_t type since we're no longer using LLVM invoke
  • in the gen_call function, the logic around invoke_fun needs to change:
    • if ast_canerror is false, we do codegen_call as normal
    • if ast_canerror is true, we need to destructure the return value to get the error flag
      • in the "error flag says it did not error" branch that we generate, we just get the other tuple value as the true return, and that's the expression result
      • in the "error flag says it did error" branch that we generate, we check if there's an invoke_target in the current frame
        • if there is an invoke_target, jump to that target block (with LLVMBuildBr or similar)
        • if there is no invoke_target, do an "error return"
  • a few places that currently do LLVMBuildRet/LLVMBuildRetVoid directly need to call gen_return (or a similar function) so that they can do separate handling of partial function return structure vs non-partial
    • genfun_fun function in genfun.c
    • genfun_new function in genfun.c
  • gen_return (or a similar function) needs to get revamped to be able to do an error-flag-including return type for partial functions
    • you can use LLVM's undef for the undefined return value in an "error return"
  • make_signature in genfun.c needs to have a special case for error-flag-including return type for partial functions
  • probably a lot of stuff breaks indirectly in codegen for code that is relying on getting the return type of a function and using that to do something - all such cases need to account for the possibility that they need to dig into the error-flag-including tuple to get the "real" return type when necessary

@SeanTAllen
Copy link
Member

SeanTAllen commented Sep 17, 2023

When getting rid of pony_error(). There are a few runtime functions that need to change. I will be updating this comment with notes on them.

time.c: ponyint_formattime

The windows version uses format_invalid_parameter_handler to set up an invalid format handler which will do a pony_error. Instead of doing pony_error, we can consider returning an empty string. We already return empty string for "potentially problematic format codes":

  // Bail out on strftime formats that can produce a zero-length string.
  if((fmt[0] == '\0') || !strcmp(fmt, "%p") || !strcmp(fmt, "%P"))
  {
    buffer = (char*)pony_alloc(ctx, 1);
    buffer[0] = '\0';
    return buffer;
  }

On Windows, instead of using the handler function that has a void return type, we can (on windows only) check if len is 0 after calling strftime and if it is AND errno is EINVAL then we bail because we weren't able to validate all the parameters to strftime.

This would make ponyint_formattime not partial and we should update its FFI definition in posix_date.pony.

When can consider leaving format on PosixDate partial and having it error if the returned string is empty.

socket.c: multiple functions

We have two options.

  1. Use -1 to indicate "unrecoverable error"
  2. Return a struct that includes "len" etc value that we are currently conveying. And an error value that we set to the error code if we get an error. In the case of an error, we make sure "len" is 0 and set the correct error code. For success, we make sure error code is 0. It is then the responsibility of calling code to handle each of the error types.

Although it is more work, I am leaning towards 2 being a better option.

@SeanTAllen SeanTAllen added needs investigation This needs to be looked into before its "ready for work" needs discussion Needs to be discussed further enhancement New feature or request and removed discuss during sync Should be discussed during an upcoming sync labels Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs discussion Needs to be discussed further needs investigation This needs to be looked into before its "ready for work"
Projects
None yet
Development

No branches or pull requests

3 participants