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(ops): Automatic fast ops creation #15527

Merged
merged 8 commits into from
Sep 22, 2022

Conversation

aapoalas
Copy link
Collaborator

@aapoalas aapoalas commented Aug 21, 2022

Use options.data to get access to the OpCtx struct instead of using the receiver object (essentially the this argument).

This puts fast ops' context access on essentially the same path as "slow ops", ie. both use the external_data argument.

I was hoping this would enable removing all or at least some of the ExternalResource stuff but that does not seem to be the case, though I do not understand why they're needed now and why they need to be kept out of the snapshot creation.

There might be a very nasty crash hidden in this PR: Once when running the deno_common benchmarks my whole computer froze...

I expect I'll need help to polish this PR to a proper level where it can be merged. Especially the ExternalResource stuff is something that I'd love to remove one way or another but I'm not sure how.

EDIT: As a test, I added a minor change with major effect to the fast ops macro: It's no longer opt in but instead is used for all sync ops if applicable. Also, I fixed the completely broken data extracting from the options bag. Build still passes, which is nice.

@aapoalas
Copy link
Collaborator Author

aapoalas commented Aug 22, 2022

So it turns out that the data member on the FastApiCallbackOptions is likely completely wrong. Getting the External from there does not work at present.

https://chromium-review.googlesource.com/c/v8/v8/+/3844662

@aapoalas aapoalas force-pushed the feat/ops-simplified-fast-ops branch from 1beb3e8 to 9ab657d Compare September 21, 2022 08:06
@aapoalas aapoalas marked this pull request as ready for review September 21, 2022 12:06
@aapoalas aapoalas changed the title [WIP] feat(ops): Simplify fast ops creation feat(ops): Automatic fast ops creation Sep 21, 2022
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work

@littledivy littledivy merged commit 707e9e3 into denoland:main Sep 22, 2022
@aapoalas aapoalas deleted the feat/ops-simplified-fast-ops branch September 22, 2022 04:55
dsherret pushed a commit to dsherret/deno that referenced this pull request Sep 22, 2022
hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024
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