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(trampoline): add a trampoline module (@endo/trampoline) #2263

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented May 1, 2024

we will probably need something like this. based on work by @naugtur

UPDATE: I plan to use this in #2310 and someone should eventually refactor ses to use it.

@boneskull boneskull self-assigned this May 1, 2024
@boneskull boneskull added the enhancement New feature or request label May 1, 2024

From [Wikipedia](https://en.wikipedia.org/wiki/Trampoline_(computing)):

> A trampoline is a loop that iteratively invokes [thunk](https://en.wikipedia.org/wiki/Thunk_(functional_programming))-returning functions ([continuation-passing style](https://en.wikipedia.org/wiki/Continuation-passing_style)).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to highlight that this is one of the usage of the word "trampoline" in programming contexts, and I've actually used trampoline to refer to other usages (in particular a piece of code that allows me to reach another context).

I'd prefer if we named this package in a way that reflects the generator iteration model that this trampoline uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, you're not saying, "don't call it a trampoline", yes? There is precedent in the JS ecosystem for this meaning.

I would prefer we name it something silly, of course.

Open to suggestions. I like rainbow-trampoline, because it supports functions of any color 🌈.

Copy link
Contributor

Choose a reason for hiding this comment

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

Function coloring is also a loaded term. Sync/async coloring is currently the main kind in JS, but coloring has been used in the context of a couple other proposals lately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can, without rancor, use the more-widely-understood meaning of words within their context (in this case, the community of JavaScript developers) while disregarding those less-widely-understood meanings. Naming is hard and we do the best we can 😄

But if you (or anyone else) has a suggestion for naming this package, please offer it!

Copy link
Member

Choose a reason for hiding this comment

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

optional-a-sync

Copy link
Member

@naugtur naugtur Aug 14, 2024

Choose a reason for hiding this comment

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

@mhofman @boneskull need more razors for this yak shaving?

I tend to use GPT for seeding name ideas. gpt-4o is really bad at it tho, had to go back to gpt-4
Here are some not awful seeds which we could use as inspiration for coming up with something reasonable.

1. AsyncBridge
2. SyncAsyncHarmonizer
3. JumpSync
4. YieldBounce
5. GenTrampolinator
6. AsyncOrNot
8. CodeChameleon
12. SyncAsyncLeap
16. AsyncFlip
17. YieldFlex
18. BounceBetween
19. SyncAsyncFusion

I still stand by optional-a-sync but these could also work from above:
async-or-not sync-async-chameleon sync-async-bridge

Also, I honestly don't see anything wrong in using the word trampoline in an opinionated way. All packages called trampoline that I've found are using it in various meanings unrelated to each other.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to name this trampoline.

Copy link
Contributor Author

@boneskull boneskull Aug 26, 2024

Choose a reason for hiding this comment

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

I'd name it "The Right Honourable Boris von Mejía" if it were totally up to me, but I can live with @endo/trampoline.

@boneskull boneskull force-pushed the boneskull/supertramp branch 2 times, most recently from f419a5d to 894f91a Compare August 1, 2024 23:30
@boneskull boneskull marked this pull request as ready for review August 1, 2024 23:30
@boneskull boneskull changed the title feat(trampoline): [DRAFT] add a trampoline module feat(trampoline): add a trampoline module Aug 1, 2024
@boneskull boneskull changed the title feat(trampoline): add a trampoline module feat(trampoline): add a trampoline module (@endo/trampoline) Aug 1, 2024
"private": true,
"eslintConfig": {
"extends": [
"plugin:@endo/internal"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy/pasted this.

I don't understand why this is here and why we are pulling in all of those eslint dev deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I am not sure why we're pulling in all of these dev deps in any workspace.

Why not let them all live in the monorepo root?

Copy link
Member

Choose a reason for hiding this comment

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

Let’s follow-up about this separately and solve it homogeneously if we can. The config for ses and below is a little different than above ses, where we can count on Hardened JavaScript for some invariants. But, otherwise, if we can hoist more, I’m in favor.

If we are going to use this package in SES, it requires the more-strict lint (no-polymorphic-call), and I assume you’re shooting for that. (to follow-up with a SES refactor). There’ll be a bit of duplication with uncurryThis and getting commons.

@naugtur did you review this with a close eye on whether this would interfere with Hermes?


## What is this?

The pattern exposed by this library—known as [trampolining][]—helps manage control flow in a way that avoids deep recursion and potential stack overflows. It effectively "converts" recursive calls into a loop. This is especially helpful in a language like JavaScript [because reasons][proper-tail-calls].
Copy link
Member

@naugtur naugtur Aug 27, 2024

Choose a reason for hiding this comment

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

This line remains slightly confusing - first time reading it I didn't grok that it's describing what the library is NOT.

Suggested change
The pattern exposed by this library—known as [trampolining][]helps manage control flow in a way that avoids deep recursion and potential stack overflows. It effectively "converts" recursive calls into a loop. This is especially helpful in a language like JavaScript [because reasons][proper-tail-calls].
This library is not just an implementation of [trampolining][] that helps manage control flow in a way that avoids deep recursion and potential stack overflows (by turning recursive calls into a loop).

I don't think [proper-tail-calls] is something we want to discuss here as the goal of this trampoline is not that. It's to allow deduplicating logic when both sync and async implementations are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for posterity: even if it is not an explicit goal, we get mitigation of the lack of proper TCO for free. right?

boneskull

This comment was marked as resolved.

boneskull

This comment was marked as outdated.

naugtur

This comment was marked as resolved.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I like it. I agree with @naugtur that it should export syncTrampoline and asyncTrampoline without leaving the consumer to guess which of these trampoline aliases.

packages/trampoline/README.md Outdated Show resolved Hide resolved
packages/trampoline/README.md Outdated Show resolved Hide resolved
"private": true,
"eslintConfig": {
"extends": [
"plugin:@endo/internal"
Copy link
Member

Choose a reason for hiding this comment

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

Let’s follow-up about this separately and solve it homogeneously if we can. The config for ses and below is a little different than above ses, where we can count on Hardened JavaScript for some invariants. But, otherwise, if we can hoist more, I’m in favor.

If we are going to use this package in SES, it requires the more-strict lint (no-polymorphic-call), and I assume you’re shooting for that. (to follow-up with a SES refactor). There’ll be a bit of duplication with uncurryThis and getting commons.

@naugtur did you review this with a close eye on whether this would interfere with Hermes?

*/
const { getPrototypeOf } = Object;
const { bind } = Function.prototype;
const uncurryThis = bind.bind(bind.call); // eslint-disable-line @endo/no-polymorphic-call
Copy link
Member

Choose a reason for hiding this comment

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

Mind that the rule might not be enabled for this package (and probably should be enabled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the eslint rule that warns if eslint directives are unused should be enabled 😄

@boneskull
Copy link
Contributor Author

The only thing I dislike about this now is the trampoline export - syncTrampoline and asyncTrampoline are a perfect pair. There's not much special about the async one and using only one of them doesn't make much sense.

trampoline has been removed; now only asyncTrampoline and syncTrampoline exist

This package will be used by `@endo/compartment-mapper` and potentially `ses`.
@boneskull boneskull merged commit 4406f5d into master Sep 10, 2024
15 checks passed
@boneskull boneskull deleted the boneskull/supertramp branch September 10, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants