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

src: drop homegrown thread pool, use libplatform #1329

Merged
merged 1 commit into from
Apr 3, 2015

Conversation

bnoordhuis
Copy link
Member

Drop the homegrown thread pool that was introduced in commit 50839a0
("v8_platform: provide default v8::Platform impl") and use one from
V8's libplatform library. Performance is comparable and it removes
a few hundred lines of code.

The calls to v8::platform::PumpMessageLoop() are currently no-ops
because V8 does not (yet?) use v8::Platform::CallOnForegroundThread().

Packagers that link against a shared libv8 now also need to make
libv8_platform available.

R=@indutny

more = uv_run(env->event_loop(), UV_RUN_ONCE);
v8::platform::PumpMessageLoop(default_platform, isolate);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: this one can be moved into the if (more == false) { branch below.

@indutny
Copy link
Member

indutny commented Apr 2, 2015

LGTM, if it works.

@Fishrock123
Copy link
Contributor

Packagers that link against a shared libv8 now also need to make
libv8_platform available.

Is this semver-minor or something then?

@bnoordhuis
Copy link
Member Author

Is this semver-minor or something then?

I don't think so. Packagers are a self-help group, we never made any promises what will and won't work between releases. Besides, I don't think any distros currently carry io.js, it's too new!

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/431/

@jbergstroem
Copy link
Member

FWIW, amongst the packages I've found - no one resorts to using a shared v8 any longer. It's too painful to manage.

@bnoordhuis
Copy link
Member Author

Good to know. In that case we should gut the shared library support from configure and node.gyp.

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 2, 2015
@mscdex
Copy link
Contributor

mscdex commented Apr 2, 2015

For what tasks does io.js use its own thread pool versus the one from libuv?

@bnoordhuis
Copy link
Member Author

Only V8 tasks at the moment, i.e., background recompilation and concurrent sweeping. Longer term, I want to unify both thread pools (comment).

@jbergstroem
Copy link
Member

@bnoordhuis I think removing shared support for v8 is the right way forward. I'll do a PR. Packagers that insist will find a way.

Drop the homegrown thread pool that was introduced in commit 50839a0
("v8_platform: provide default v8::Platform impl") and use one from
V8's libplatform library.  Performance is comparable and it removes
a few hundred lines of code.

The calls to v8::platform::PumpMessageLoop() are currently no-ops
because V8 does not (yet?) use v8::Platform::CallOnForegroundThread().

Packagers that link against a shared libv8 now also need to make
libv8_platform available.

PR-URL: nodejs#1329
Reviewed-By: Fedor Indutny <[email protected]>
@bnoordhuis bnoordhuis closed this Apr 3, 2015
@bnoordhuis bnoordhuis deleted the use-libplatform branch April 3, 2015 22:23
@bnoordhuis bnoordhuis merged commit 4a801c2 into nodejs:v1.x Apr 3, 2015
@@ -664,7 +664,7 @@ def configure_v8(o):
if options.shared_v8_libname:
o['libraries'] += ['-l%s' % options.shared_v8_libname]
elif options.shared_v8:
o['libraries'] += ['-lv8']
o['libraries'] += ['-lv8', '-lv8_platform']
Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis won't this be an issue when providing your own shared libname (since you can only provide one)? I guess it's irrelevant when my shared v8 patch lands..

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, it's an issue. And yes, it won't be an issue any more once shared library support is removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants