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

Implement 'AsyncRefCell' and 'ResourceTable2' #8273

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

piscisaureus
Copy link
Member

No description provided.

@piscisaureus piscisaureus changed the title what if this just works after all [TRY] what if this just works after all Nov 6, 2020
@piscisaureus
Copy link
Member Author

Unfortunately it did not, and I do know why, and it is complicated ...

@piscisaureus piscisaureus changed the title [TRY] what if this just works after all [WIP] Upgrade to Tokio 0.3 Nov 8, 2020
@piscisaureus piscisaureus reopened this Nov 8, 2020
@piscisaureus
Copy link
Member Author

Far from green, but this works:

cargo run --release -p deno_core --example http_bench_bin_ops

@ry
Copy link
Member

ry commented Nov 24, 2020

I ran the benchmarks - there is some minimal performance degradation on linux (~5% slower)
https://gist.github.com/ry/3fd8d1420fd3710755f9de1f1641d930

core/resources2.rs Show resolved Hide resolved
@@ -34,6 +34,7 @@ pub enum Op {
/// Maintains the resources and ops inside a JS runtime.
pub struct OpState {
pub resource_table: crate::ResourceTable,
pub resource_table_2: crate::resources2::ResourceTable,
Copy link
Member

Choose a reason for hiding this comment

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

nit: use resource_table2 - shorter.

core/examples/http_bench_bin_ops.rs Show resolved Hide resolved
Comment on lines +112 to +117
/// cause the resource to be dropped. However, since resources are reference
/// counted, therefore pending ops are not automatically cancelled.
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional design? I thought the main point of close is to drop pending ops for the resource.

core/async_cell.rs Show resolved Hide resolved
@piscisaureus piscisaureus changed the title [WIP] Upgrade to Tokio 0.3 Implement 'AsyncRefCell' and 'ResourceTable2' Nov 25, 2020
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

assert!(cell.try_borrow_mut().is_none());
assert!(cell.try_borrow().is_some());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

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

@piscisaureus piscisaureus merged commit 8d12653 into denoland:master Nov 25, 2020
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.

3 participants