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 case for a context object #1390

Open
ghost opened this issue Feb 2, 2024 · 2 comments
Open

The case for a context object #1390

ghost opened this issue Feb 2, 2024 · 2 comments

Comments

@ghost
Copy link

ghost commented Feb 2, 2024

Among embedded scripting languages, Janet is as far as I know joined only by Chez Scheme in storing VM state statically, rather than as an opaque context object passed by handle to API functions. The rationale given for this on the website is that C has no type-namespaced functions, so a context object is another parameter to every API call, which is clutter.

However, as also noted on the website, this confines VMs to threads, which causes problems when one VM doesn't necessarily stay on one thread or have a whole thread to itself:

  • Coroutines on a multithreaded event loop, à la Zig, C++: there was previously discussion of a save/restore mechanism that would address this
  • noMMU environments: perhaps a non-concern, as most such environments can't run much Janet anyway
  • Large multi-component programs where multiple DSOs have an extension interface: this seems intractable with the current model, as there's no opportunity to save/restore (any function call could be a DSO boundary), and DSOs from different vendors could not practically centralise VM setup/teardown
  • As above, with heterogeneous VM requirements (allocator, sandbox etc.): this is the real bugbear, as even cross-component centrality won't help

A context object would solve these problems, but of course introduce problems of its own:

  • The object would then have to be tracked and passed to anywhere that wants to use it, which would be trivial in functions called from Janet but slightly awkward in the code that calls into the top-level VM. In my mind however, a maximally correct program will signal if VM setup fails, and then subsequent code won't call into Janet in that case; this could be a lot more robust if failed setup simply doesn't produce an object, so subsequent code can't use it.
  • A handle could potentially be shared between multiple threads at once, which would cause weirdness. I personally think that's easy enough to avoid with due diligence, and if someone does wind up doing it, they deserve what they get.
  • It would be an apocalyptically breaking change. It'd probably be easy enough to write a tool that auto-patches the affected signatures and calls, but asking every API user to run said tool on their own code would be a lot.

I won't be so arrogant as to assume I know what's best, but personally I feel that a context object is the more correct solution. And if we agree, I think it's better to do it sooner rather than later.

@bakpakin
Copy link
Member

bakpakin commented Feb 3, 2024

Short answer: It's faster and simpler, and that is just how it was originally written.

Long answer:
These are all good points, but at the end of the day I'm not really confident that a rewrite is worth it. Also usually this sort of things causes a perf hit due to extra state being passed around everywhere, or the presence of extra pointer dereferencing and extra arguments. If I were to start over from scratch I might create a global context object (mostly for ease of implementation, the creation of sub-contexts is straightforward), but there are downsides. And just because a context object would help people attach custom Zig and C++ event loops doesn't mean that that is a good idea - Janet has it's own and they would conflict (the whole idea of frameworks with pluggable event loops seems completely insane to me).

Coroutines on a multithreaded event loop, à la Zig, C++: there was previously discussion of a save/restore mechanism that would address this

Since Janet comes with it's own event loop (for better or for worse), this is really not something I want to address. However, save restore functionality works right now with janet_vm_save and janet_vm_load (see state.c). As pointed out though, this is not quite as nice if you are for some reason trying to embed in strange way. I feel like there is some circumstances where this just doesn't work but I can't think of any offhand.

And if we agree, I think it's better to do it sooner rather than later.

Unfortunately, it's not that soon from my perspective. This kind of argument always comes up when people are new to a project. That said, I think it would be a moderate but tedious effort to thread the JanetVM * object as the first argument to all functions, and then some more effort to make some things more idiomatic (I would also be curious on the perf impact - my guess would be small but measurable, like 5%).

But I think you will also find that certain things like signals require setting up some implicit global or per-thread state anyway.

@iacore
Copy link
Contributor

iacore commented Feb 3, 2024

That said, I think it would be a moderate but tedious effort to thread the JanetVM * object as the first argument to all functions, ...

Now I know semgrep, this should be easier.

My prior attempt at making GC functions use a context: #1199

Since Janet comes with it's own event loop (for better or for worse), this is really not something I want to address.

I often use Janet to process data, and the event loop is not needed. One example project I have is an HTTP server that runs some processing (need JanetVM) on every request. The current is API is usable, but I often forget to switch VM so I ended up creating a new VM on every request.

The language I use has defer so I can use defer c.janet_vm_save(&vm);. In C, error handling would be messier, since you have to save the VM state if you want to use it further (other requests might reset thread-local VM).

If the host language has its own event loop thread-switching (like Go), then the current API cannot be used easily (need janet_vm_save/load on context switching).

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

No branches or pull requests

2 participants