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.

Sign up for GitHub

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

Jump to bottom

Experimental version of API that replaces Handle<'a, V> with &V #347

Closed
wants to merge 11 commits into from
Closed

Experimental version of API that replaces Handle<'a, V> with &V #347

wants to merge 11 commits into from

Conversation

dherman
Copy link
Collaborator

@dherman dherman commented Jul 31, 2018 •
edited
Loading

This experimental PR gets rid of the somewhat unpleasant Handle<'a, V> smart pointer type and replaces it with a simple &'a V. Every context carries an internal arena for allocating handles so that the APIs can return references to those arena-allocated handles.

As a simple example:

fn forty_two(mut cx: FunctionContext) -> NeonResult<&JsNumber> {
    Ok(cx.number(42))
}

matklad
matklad reviewed Aug 8, 2018
View reviewed changes
src/context/internal.rs Outdated Show resolved Hide resolved
@amilajack
Copy link
Member

🎉 🎉 🎉 Awesome work Dave!

@dherman
Copy link
Collaborator Author

dherman commented Feb 23, 2019

Looks like something is wrong specifically with Node 6. Gotta go to bed but I'll investigate that next.

@kjvalencik
Copy link
Member

FYI, Node 6 is end of life in April.

@amilajack amilajack mentioned this pull request Feb 25, 2019
Closed
@dherman
Copy link
Collaborator Author

dherman commented Mar 1, 2019

Ugh, and now I can't even build it on my mac to investigate, because Node 6 has been left in an uncompilable state on macOS Mojave.

@dherman
Copy link
Collaborator Author

dherman commented Mar 1, 2019

Ugh, and now I can't even build it on my mac to investigate

OK but I can still build on Linux. Investigating…

@dherman
Copy link
Collaborator Author

dherman commented Mar 1, 2019

OK I believe this is the issue: DefinitelyTyped/DefinitelyTyped#32897 — I'm talking with the handlebars maintainers and I'll track their fix as it lands.

dherman added 11 commits March 8, 2019 21:40
@dherman
WIP: Add a HandleArena to every context, and proof of concept with …
34ede8c 
…`JsUndefined::new_thin()` and `cx.undefined_thin()`.
@dherman
Got the basic type hierarchy working, with a few manual smoke tests w…
ee856bd 
…orking. Still a good amount of stubbed code left to finish...
@dherman
Finish migrating the Task API.
977e7a1
@dherman
- Finish migrating class and object APIs
ca754c3 
- Move `build` and `build_infallible` into `cx.new()` and `cx.new_infallible()`, respectively
@dherman
Lots of cleanup and refactoring:
f7c63ad 
- runtime API consistency improvements
- eliminate HandleArena
- build/build_infallible -> ContextInternal::new/new_infallible, and they abstract the extraction of the raw Isolate
- abstract away ContextInternal::alloc_persistent into the internals of new/new_infallible
- add ContextInternal::new_opt for string construction that can fail without throwing a JS exception
- execute_scoped
- uncomment cx.empty_array
- rename persistent_arena to handles
@dherman
- implemented compute_scoped
d8d5f0b 
- eliminated all HandleScopes
- changed classes to use Persistents
@dherman
Remove remaining commented-out bits (CallContext::this, prelude re-ex…
7090370 
…ports)
@dherman
rename PersistentArena to HandleArena
32cfdd3
@dherman
cleanup:
9eb8bc5 
- rename "init" functions back to idiomatic "new" names, removing old versions
- rename "thin" functions back to idiomatic names, removing old versions
- implement function call/construct
- eliminate obsolete neon_runtime::convert module
@dherman
All tests passing again! \o/
749cd6f
@dherman
Add back accidentally-removed JS [[ToString]] operation (`Value::to_s…
aea5dc0 
…tring()`)
@dherman
Copy link
Collaborator Author

dherman commented Mar 9, 2019

I merged #396 to fix the typescript issue, and I'm rebasing and trying again…

@dherman dherman changed the title [WIP] Experimental version of API that replaces Handle<'a, V> with &V Experimental version of API that replaces Handle<'a, V> with &V Mar 9, 2019
@dherman
Copy link
Collaborator Author

dherman commented Mar 9, 2019

@kjvalencik This PR is ready for experimentation! If you have any time and are still interested, it'd be super interesting to see an experiment or two with real code, both for comparing readability and to ensure there aren't noticeable performance issues.

@kjvalencik
Copy link
Member

@dherman Yes, I can do that. I'll take notes along the way similar to what I did for the v0.2. I do have a decent amount of clean-up I would like to do prior to trying it out (upgrade dependencies, switch to Rust 2018).

@dherman
Copy link
Collaborator Author

dherman commented May 17, 2020

This is out of date, and @kjvalencik's experience experimenting with this change was pretty negative. I would love to know more about it at some point, but this isn't very high priority right now so I'm going to close the PR. We can always look at it for reference in future if we're interested in reviving the idea.

@dherman dherman closed this May 17, 2020
@kjvalencik
Copy link
Member

Unfortunately, I deleted the branch that tested this API. If I recall correctly, this change forced users to define lifetimes in many places that it was previously not necessary. It made it very difficult for a new Rust user to try out Neon without immediately being thrown in the deep end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matklad matklad matklad left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dherman @amilajack @kjvalencik @matklad