Skip to content

fix(core): remove async op inlining optimization #17899

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

Merged
merged 8 commits into from
Feb 23, 2023

Conversation

littledivy
Copy link
Member

Reverts #16428

Fixes #16535

Runtime generation of async op wrappers contributed to increased startup time and core became unusable with --disallow-code-generation-from-strings flag. The optimization only affects very small microbenchmarks so this revert will not cause any regressions.

@littledivy littledivy marked this pull request as ready for review February 23, 2023 14:24
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, this commit brings us back to sub-20ms startup time.

@bartlomieju bartlomieju enabled auto-merge (squash) February 23, 2023 18:00
@bartlomieju bartlomieju merged commit da78128 into denoland:main Feb 23, 2023
@bartlomieju bartlomieju deleted the remove_jit_opasync branch February 23, 2023 19:50
ramnivas added a commit to exograph/exograph that referenced this pull request Mar 23, 2023
The major change is how we invoke async functions (see denoland/deno#17899). Without the change (for example, `core.ops.op_claytip_execute_query_priv(query_string, variables, context_override)` to `core.opAsync("op_claytip_execute_query_priv", query_string, variables, context_override)`, we see failures with error message: "ERROR payas_deno::deno_module > err=TypeError: invalid promise id: expected type `v8::data::Integer`, got `v8::data::Value`"
ramnivas added a commit to exograph/exograph that referenced this pull request Mar 23, 2023
The major change is in how we invoke async functions (see denoland/deno#17899), so instead of, for example `core.ops.op_claytip_execute_query_priv(query_string, variables, context_override)`, we use `core.ops.op_claytip_execute_query_priv(query_string, variables, context_override)`. Without this change we see failures with an error message: "ERROR payas_deno::deno_module > err=TypeError: invalid promise id: expected type `v8::data::Integer`, got `v8::data::Value`"

Also, add a unit test for invoking Rust async operation through JavaScript to test/debug such issues in payas-deno itself.
shadaj pushed a commit to exograph/exograph that referenced this pull request Apr 20, 2023
The major change is in how we invoke async functions (see denoland/deno#17899), so instead of, for example `core.ops.op_claytip_execute_query_priv(query_string, variables, context_override)`, we use `core.ops.op_claytip_execute_query_priv(query_string, variables, context_override)`. Without this change we see failures with an error message: "ERROR payas_deno::deno_module > err=TypeError: invalid promise id: expected type `v8::data::Integer`, got `v8::data::Value`"

Also, add a unit test for invoking Rust async operation through JavaScript to test/debug such issues in payas-deno itself.
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.

Panic: "Code generation from strings disallowed for this context" with 1.27.1
2 participants