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.

Sign up for GitHub

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

Jump to bottom

refactor: Worker is not a Future #7895

Merged
merged 15 commits into from Oct 9, 2020
Merged

refactor: Worker is not a Future #7895

merged 15 commits into from Oct 9, 2020

Conversation

bartlomieju
Copy link
Member

No description provided.

@bartlomieju
refactor(core): JsRuntime is not a Future
ef7aaf5 
This commit rewrites deno_core::JsRuntime to not implement Future
trait.

Instead there are two separate methods:
- JsRuntime::poll_event_loop() - does single tick of event loop
- JsRuntime::run_event_loop() - runs event loop to completion
@bartlomieju
runtime -> self
d617e5c
@bartlomieju
Merge branch 'master' into refactor_runtime_future
06bd828
@bartlomieju
update readme
d56bcf2
@bartlomieju
refactor: rename isolate to js_runtime
27e0c13 
This commit renames occurrences of "isolate" variable name
to "js_runtime". This was outstanding debt after renaming
deno_core::CoreIsolate to JsRuntime.
@bartlomieju
rename poll methods
ba3ae5c
@bartlomieju
Merge branch 'refactor_rename_isolate_runtime' into refactor_worker_f…
6c69681 
…uture
@bartlomieju
refactor(cli): remove Deref implementation for Worker
2ef1da2
@bartlomieju
make Worker.inspector private
7d06362
@bartlomieju
fields visibility
1b9d839
@bartlomieju
Merge branch 'master' into refactor_worker_future
4a925eb
@bartlomieju
workers are not futures
75dd141
@bartlomieju
Merge branch 'master' into refactor_worker_future
1110ffe
@bartlomieju bartlomieju requested a review from ry October 9, 2020 11:10
@bartlomieju bartlomieju changed the title [WIP] refactor: Worker is not a Future refactor: Worker is not a Future Oct 9, 2020
@caspervonb
Copy link
Contributor

caspervonb commented Oct 9, 2020 •
edited

Welcome change from my perspective, nice to be explicit about this but a few open questions.

  1. Event loop implies inspector loop? Maybe this is a good time to decouple and be explicit about that.
  2. Creating a session from the worker, seems like this should hang off the inspector instead? The panic is a bit awkward.

@bartlomieju
Copy link
Member Author

Welcome change from my perspective, nice to be explicit about this but a few open questions.

  1. Event loop implies inspector loop? Maybe this is a good time to decouple and be explicit about that.

How do you propose to do that?

  1. Creating a session from the worker, seems like this should hang off the inspector instead? The panic seems very awkward.

It would mean that Worker::inspector would be public which I explicitly don't want to do and that's why there's a method

@ry
Copy link
Member

ry commented Oct 9, 2020 •
edited

What happened to d8879fe?

nevermind - I see this is worker not jsruntime

ry
ry approved these changes Oct 9, 2020
View reviewed changes
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

@bartlomieju bartlomieju merged commit f4357f0 into denoland:master Oct 9, 2020
@bartlomieju bartlomieju deleted the refactor_worker_future branch October 9, 2020 17:08
ry added a commit to ry/deno that referenced this pull request Oct 10, 2020
@ry
Revert "refactor: Worker is not a Future (denoland#7895)"
75c812e 
This reverts commit f4357f0.
bartlomieju pushed a commit that referenced this pull request Oct 10, 2020
@ry
Fix 100% CPU idling problem by reverting #7672 (#7911)
08bb8b3 
* Revert "refactor: Worker is not a Future (#7895)"

This reverts commit f4357f0.

* Revert "refactor(core): JsRuntime is not a Future (#7855)"

This reverts commit d8879fe.

* Revert "fix(core): module execution with top level await (#7672)"

This reverts commit c7c7677.
iykekings pushed a commit to iykekings/deno that referenced this pull request Oct 10, 2020
@bartlomieju @iykekings
refactor: Worker is not a Future (denoland#7895)
713a3ba 
This commit rewrites deno::Worker to not implement Future
trait.

Instead there are two separate methods:
- Worker::poll_event_loop() - does single tick of event loop
- Worker::run_event_loop() - runs event loop to completion

Additionally some cleanup to Worker's field visibility was done.
iykekings pushed a commit to iykekings/deno that referenced this pull request Oct 10, 2020
@ry @iykekings
Fix 100% CPU idling problem by reverting denoland#7672 (denoland#7911)
19846bc 
* Revert "refactor: Worker is not a Future (denoland#7895)"

This reverts commit f4357f0.

* Revert "refactor(core): JsRuntime is not a Future (denoland#7855)"

This reverts commit d8879fe.

* Revert "fix(core): module execution with top level await (denoland#7672)"

This reverts commit c7c7677.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bartlomieju @caspervonb @ry