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] Update the co_await operator and promise_type for val to make it moveable rather than just copy. #21432

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mrolig5267319
Copy link
Contributor

Update the co_await operator and promise_type for val to make it moveable rather than just copy.

This removes C++ -> JS incref/decref calls for lifetime management. For test_embind_val_coro this removes all calls to incref, except for the two added explicitly to test that code-path.

I believe the change to get_return_object() to move the JS promise val is safe since it is not used again and part of the state owned by awaiter, and discussion here emphasizes that such a return type should likely be move-only.

Still permits copy of val for a co_await if the promise is retained, as covered by new test code.

@mrolig5267319 mrolig5267319 changed the title Coro move [emval] Update the co_await operator and promise_type for val to make it moveable rather than just copy. Feb 26, 2024
@RReverser
Copy link
Collaborator

While I can see this helping co_await specifically, it's not really different from tons of other operators we have on val, so I'm somewhat hesitant to optimise incref/decref count on case-by-case basis for maintenance reasons.

That's why I went in the direction of making sure that compiler can eliminate any incref/decref altogether instead in that first PR that started all of this, and I'd still prefer going for that kind of solution (moving it to C++ in one form or another) so that we wouldn't have to care about implicit reference counts altogether rather than have to provide duplicates of each function.

That would be a win both for performance and for future maintainability of Emval code.

@mrolig5267319
Copy link
Contributor Author

Indeed, see my latest discussion on #21300 for overall direction.

I do not think that focusing on the elimination of incref/decref for copy within C++ alone will help the "transliterated" code paths.

I sent this PR simply because it was such simple low hanging fruit it seems silly to wait for a bigger overhaul.

@mrolig5267319
Copy link
Contributor Author

Please reply on #21300 if you think I should revive that and move forward. I hesitated because it comes at a code-size cost to both JS and C++

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.

2 participants