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

WIP: CPP async workers #882

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

robinchrist
Copy link

@robinchrist robinchrist commented Dec 29, 2019

This PR aims to improve the existing Async Workers using C++17 features

For the time being, only AsyncProgressQueueWorker was improved.

The improvements include

  • Memory management using smart pointers (i.e. unique_ptr) to prevent memory leaks in case of exceptions
  • Modern memory management techniques, i.e. std::aligned_alloc and std::uninitialized_copy, which remove the necessity of the MessageStruct being default constructible
  • Better locking of critical sections (i.e. std::mutex instead of libuv's mutexes and lock_guard to prevent locking forever in case of error)
  • A new API which allows progress messages to be put into the queue without being copies (using std::unique_ptr)
  • Fast paths if the legacy API is used and the progress message class is trivially copyable

As mentioned above, the legacy API SendProgress_(const T * const messages, const size_t nMessages) is still supported (but marked as deprecated). Therefore, this works as a drop-in replacement even for C++17.
Additionally, the unmodified legacy workers are still there and used if the C++ version is below C++17.

This is not ready to merged yet for some reasons:

  • It is entirely untested at the moment
  • It's not possible to test it with the current configuration. The node headers force the usage of -std=gnu++1y, but C++17 is required for this
  • An std::bad_alloc exception should be thrown in case of error, but the node headers force -fno-exceptions
  • Documentation is missing

@robinchrist
Copy link
Author

@bnoordhuis What do you think?

@bnoordhuis
Copy link
Member

The first two commits look okay to me but the last one results in an unpleasant amount of code duplication.

The node headers force the usage of -std=gnu++1y, but C++17 is required for this

A binding.gyp can override that (ditto for -fno-exceptions):

{
  'cflags_cc!': ['-std=gnu++1y'],
  'cflags_cc': ['-std=c++17'],
  'xcode_settings': {
    'CLANG_CXX_LANGUAGE_STANDARD': 'c++17',
  },
  # ..
}

But as to -fno-exceptions, you should probably not override that unless you know exactly what you're doing.

You can use exceptions in add-ons (and some do) but they should never ever escape into code compiled without exception support.

@robinchrist
Copy link
Author

The first two commits look okay to me but the last one results in an unpleasant amount of code duplication.

The problem is that this is the only way to maintain backwards compatibility, i.e. not force users to use C++17.
Of course I could deduplicate the code that hasn't been C++17ified, but I will most probably also modify the other async worker classes.

But as to -fno-exceptions, you should probably not override that unless you know exactly what you're doing.

I think it's always better to throw an exception instead of just calling std::terminate() or silently dropping the error...

As nan is a header only library, it should imho be left to the users to decide whether they want exceptions or not.

I think that, at the moment, if no memory can be allocated (which usually never happens), std::terminate() is called.

But from what I read, it might be worth creating and maintaining a nan-cpp17 fork which includes the new C++17 features and does not compile with C++11.
This is however, going to be a merge-hell, because nan has this mega-header nan.h...

@bnoordhuis
Copy link
Member

As nan is a header only library, it should imho be left to the users to decide whether they want exceptions or not.

Just to be clear, nan takes no position on the use of exceptions. -fno-exceptions is inherited from node's common.gypi through node-gyp: nodejs/node-gyp#1118

it might be worth creating and maintaining a nan-cpp17 fork which includes the new C++17 features and does not compile with C++11

Speaking for myself (not other maintainers), I have no interest in maintaining such a fork at the moment. I expect that most new add-ons will be written using n-api, not nan.

That's not to discourage you, of course. You're welcome to start that fork.

@robinchrist
Copy link
Author

Speaking for myself (not other maintainers), I have no interest in maintaining such a fork at the moment. I expect that most new add-ons will be written using n-api, not nan.

That's not to discourage you, of course. You're welcome to start that fork.

Of course I'd maintain the C++17 fork, should've mentioned that in the first place!

What I was trying to say is that I would much appreciate if it's possible to modularise the nan.h header a bit so that it's easier for me to maintain the fork (i.e. avoid the "merge-hell")...

@bnoordhuis
Copy link
Member

I'm open to that if it's not too much maintenance and review work. What plan of attack do you propose?

@kkoopa
Copy link
Collaborator

kkoopa commented Feb 2, 2020

Perhaps something based on the documentation along with the pre-0.12 / post-0.12 split?

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