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

src: implement v8::Platform::CallDelayedOnWorkerThread #22383

Closed
wants to merge 1 commit into from
Closed

src: implement v8::Platform::CallDelayedOnWorkerThread #22383

wants to merge 1 commit into from

Conversation

alexkozy
Copy link
Member

@alexkozy alexkozy commented Aug 18, 2018 •
edited
Loading

This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this flag for
every execution in console.

Fixes: #22157

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@alexkozy alexkozy requested a review from addaleax August 18, 2018 02:00
@nodejs-github-bot
Copy link
Collaborator

@ak239 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/584/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 18, 2018
@alexkozy alexkozy mentioned this pull request Aug 18, 2018
Closed
addaleax
addaleax approved these changes Aug 18, 2018
View reviewed changes
src/node_platform.cc Outdated
std::unique_ptr<Task> TakeTimerTask(uv_timer_t* timer) {
std::unique_ptr<Task> task(static_cast<Task*>(timer->data));
uv_timer_stop(timer);
uv_close(reinterpret_cast<uv_handle_t*>(timer), [](uv_handle_t* handle) {});
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t the close callback free the memory allocated for handle here? Or, alternatively …

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we should, fixed.

src/node_platform.cc Outdated
TaskQueue<v8::Task> tasks_;
uv_loop_t loop_;
uv_async_t flush_tasks_;
std::set<uv_timer_t*> timers_;
Copy link
Member

Choose a reason for hiding this comment

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

… I think we could use the fact that std::set comes with retaining referential integrity in the presence of insert/erase operations, so we could avoid the double allocations and use std::set<uv_timer_t> instead, right? (If not: I don’t think there’s an advantage over std::unordered_set, right?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Explicit allocation looks simpler to me, so I replaced set with unordered_set. Thank you!

src/node_platform.cc Outdated

static void RunTask(uv_timer_t* timer) {
DelayedTaskScheduler* scheduler =
static_cast<DelayedTaskScheduler*>(timer->loop->data);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: scheduler = ContainerOf(&DelayedTaskScheduler::loop_, timer->loop);

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@addaleax
Copy link
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/16566/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2018
@joyeecheung
Copy link
Member

This PR needs a rebase against master to avoid the git failure in the CI.

@joyeecheung joyeecheung mentioned this pull request Aug 20, 2018
Closed
@alexkozy
Copy link
Member Author

I rebased and restarted CI: https://ci.nodejs.org/job/node-test-pull-request/16589/

@alexkozy
src: implement v8::Platform::CallDelayedOnWorkerThread
4cac425 
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Fixes: #22157
@alexkozy
Copy link
Member Author

Landed in b1e2612

@alexkozy alexkozy closed this Aug 21, 2018
alexkozy added a commit that referenced this pull request Aug 21, 2018
@alexkozy
src: implement v8::Platform::CallDelayedOnWorkerThread
b1e2612 
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

PR-URL: #22383
Fixes: #22157
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@alexkozy alexkozy deleted the implemented-post-delayed-task branch August 21, 2018 01:11
@addaleax
Copy link
Member

@ak239 Can you open a backport PR against v10.x-staging here (→ guide), since this doesn’t apply cleanly, unfortunately?

@alexkozy alexkozy mentioned this pull request Aug 28, 2018
Closed
3 tasks
@alexkozy
Copy link
Member Author

I uploaded rebased PR and started CI there, I won't have internet connection next 12 hours, so I would appreciate help with merging my backport, thanks ahead!

addaleax pushed a commit that referenced this pull request Aug 28, 2018
@alexkozy @addaleax
src: implement v8::Platform::CallDelayedOnWorkerThread
b3dab8c 
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 28, 2018
targos pushed a commit that referenced this pull request Sep 3, 2018
@alexkozy @targos
src: implement v8::Platform::CallDelayedOnWorkerThread
02e3daa 
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@alexkozy @targos
src: implement v8::Platform::CallDelayedOnWorkerThread
4d802f0 
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This was referenced Sep 6, 2018
Closed
Closed
blattersturm pushed a commit to citizenfx/node that referenced this pull request Nov 3, 2018
@alexkozy @blattersturm
src: implement v8::Platform::CallDelayedOnWorkerThread
a14e56b 
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

PR-URL: nodejs#22383
Fixes: nodejs#22157
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UNREACHABLE in WorkerThreadsTaskRunner::PostDelayedTask
7 participants
@alexkozy @nodejs-github-bot @addaleax @joyeecheung @jasnell @devsnek @targos