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

The calling convention inside fastfuncs is antithetical to continuation calls #273

Open
masak opened this issue Nov 16, 2020 · 4 comments · Fixed by #274
Open

The calling convention inside fastfuncs is antithetical to continuation calls #273

masak opened this issue Nov 16, 2020 · 4 comments · Fixed by #274
Labels
bug Something isn't working

Comments

@masak
Copy link
Owner

masak commented Nov 16, 2020

At the point I'm writing this, we haven't merged #270 yet, but on that branch:

> (catch (map [throw 'aha] '(a b c)))
('unboundb throw)

That, in case you're unsure, is a bug; it should output aha.

I had a sinking suspicion what was going on, so I manually switched off the code that calls fastfuncs — gotta make an environment variable for that — and lo and behold:

> (catch (map [throw 'aha] '(a b c)))
aha

The bug manifests in the presence of fastfuncs.

More precisely, since map has a fastfunc that makes a call to the provided callback using the Perl stack, there's no way it ever could work correctly. The throw in that callback writes a check that the Perl stack can not cash.

The above minimal case was not the first one I found, which was this one when playing around with #136 . After adding this epiloge to the code for testing purposes:

(prn (run '(10 5) '(2 2) '(1 3 1 1 1 0)))

(Note in particular that the final 0 triggers a throw.)

The program output the baffling result (t t t t t (5 1)). I don't know for sure, but I suspect that the extraneous t values come from the or call in checkbounds; they really shouldn't be there.

I confirmed that removing the 0 resulted in the correct output (5 1).

Trying to run the script with fastfuncs entirely turned off took too long. I ran it for five minutes, then killed it.

Just now, I ran it but selectively turning off the map fastfunc. It correctly outputs (5 1). This suggests a way forward, for the short term. We need to switch off the fastfuncs which call one of their parameters, because the way they make that call is not compatible with continuation calls.

As part of de-activating these fastfuncs, we should write regression tests for each of them that exposes the correct behavior. Like, a whole test file full of them.


For the longer term, I dunno. We do have a fastfunc compiler these days. We could maybe use it to compile to a CPS form of these functions. That could work.

@masak masak added the bug Something isn't working label Nov 16, 2020
@masak
Copy link
Owner Author

masak commented Feb 3, 2021

Going to re-open this one, because after #288 there are again some higher-order fastfuncs.

Two things:

  • Have tests for every higher-order function, testing that continuations can go into and out of them.
  • Finish up the fastcont thing; making it possible to write fastfuncs that interact well with continuations.

@masak masak reopened this Feb 3, 2021
@masak
Copy link
Owner Author

masak commented Oct 2, 2021

Looking at this with fresh eyes, the whole problem is never going to be fixed as long as the Language::Bel package still has the call method. That whole method is in itself incompatible with the rest of Bel's evaluation strategy, and it needs to go.

The higher-order functions also needs a way to pause-and-continue, like coroutines, but that's a separate thing that needs to happen.

@masak
Copy link
Owner Author

masak commented Oct 3, 2021

PR #400 gets us rid of call. I'll consider this issue closable when we get our higher-order fastfuncs back.

@masak
Copy link
Owner Author

masak commented Oct 6, 2021

In other words, this now works on the master branch:

% bel
Language::Bel 0.59 -- darwin.
> (catch (map [throw 'aha] '(a b c)))
aha

Feels good.

But need those tests, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant