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

[emval] Add unique_val analog to be have like a std:unique_ptr version of val. #21433

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mrolig5267319
Copy link
Contributor

Following from explorations in #20447 and #21300, it may be that the simplest approach to reducing JS/C++ overhead for reference counting is to simply use move-able instances more.

One way to encourage this would be to have a class similar to std::unique_ptr to force move-semantics and singular ownership of a handle.

At the moment this is a proof of concept for this:

It carves out a base class base_val with no public constructor to capture the static factory functions and methods to operate with either val or unique_val

It changes all the return types of these methods and factories to return unique_val to encourage its use.

unique_val has no copy constructor/assignemtn, only move.

val has implicit constructor from unique_val that will do the incref to have a second reference in C++. This allows the return value change to be safe with existing code expecting val.

It's not clear to me at the moment if this is valuable, or if it's best to just encourage the use of std::move with val as is.

If this direction is useful, I wonder if the analogy to standard library smart pointers could be made stronger with different names:

emscripten::unique_js_ptr and emscripten::shared_js_ptr. emscripten:vall would then be an alias for emscripten::shared_js_ptr. the shared_js_ptr could build on #21300 to further minimize overhead.

Leaving this as draft -- I don't have any urgent need to make this more efficient, and don't have the bandwidth to push for a big refactor here. Just adding here to share thoughts for future work.

@RReverser

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

Successfully merging this pull request may close these issues.

None yet

1 participant