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

node-cpp updates #43

Merged
merged 10 commits into from
Nov 16, 2017
Merged

node-cpp updates #43

merged 10 commits into from
Nov 16, 2017

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Nov 14, 2017

What changed?

  • adding new "concurrency" example section
  • porting https://github.com/mapbox/versioning-node-cpp into this doc to resolve Move node-cpp-versioning into this repo #11
  • remove "naming conventions" since it's a little too opinionated and we don't even follow it
  • add a table of contents to this doc
  • update "developing addons" section to essentially point at node-cpp-skel for docs
  • add this doc to the glossary under "node addon"
  • add profiling to glossary
  • add concurrency to glossary with link to new doc section

Best viewed https://github.com/mapbox/cpp/blob/concurrency-additions/node-cpp.md

cc @GretaCB @mapbox/core-tech

@mapsam mapsam requested a review from GretaCB November 14, 2017 20:41
Copy link
Contributor

@GretaCB GretaCB left a comment

Choose a reason for hiding this comment

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

I'm going to follow this up with a branch that implements all of my comments as part of this review

@@ -1,66 +1,115 @@
# Node.js C++ Addons

The following document outlines Mapbox's approach to writing C++ modules for Node.js (often referred to as _addons_).
* [Why create a node addon?](#why-create-a-node-addon)
Copy link
Contributor

Choose a reason for hiding this comment

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

heck yes, TOC

node-cpp.md Outdated
* [Versioning](#versioning)
* [Additional Resources](#additional-resources)

The following document outlines Mapbox's general approach to writing C++ modules for Node.js (often referred to as _addons_), and they _why_. Check out [node-cpp-skel](https://github.com/mapbox/node-cpp-skel), a skeleton library for creating a Node.js addon, to learn more about _how_ to create an addon.
Copy link
Contributor

Choose a reason for hiding this comment

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

and the why


To swing between Node and C++, the Node community maintains a project called [_NAN_](https://github.com/nodejs/nan) (Native Abstractions for Node.js) that simplifies running different versions of Node and, subsequently, v8. NAN is a header-only C++ library that provides a set of Macros for developing Node.js addons. Check out the [usage](https://github.com/nodejs/nan#usage) guidelines.
1. To port a C++ project to Node to expose a new interface for the tool (like Mapnik & Node Mapnik)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps link to Node glossary term

node-cpp.md Outdated
More examples of how to port C++ libraries to node can be found at [nodejs.org/api/addons](http://nodejs.org/api/addons.html). See https://nodesource.com/blog/c-add-ons-for-nodejs-v4/ for a detailed summary of the origins of Nan. And see http://blog.reverberate.org/2016/10/17/native-extensions-memory-management-part2-javascript-v8.html for a good introduction to v8 that is not specific to node.js.
**Concurrency**

Concurrency is the process of executing different pieces of the same process to allow for parallel execution of these pieces [[wikipedia](https://en.wikipedia.org/wiki/Concurrency_(computer_science))]. Node.js addons allow us to take advantage of concurrent operations within our node applications by reaching into more than just the v8 thread, but this can result in a few surprises. In the example below, we’ve passed data from our node application, into the v8 thread, and subsequently into our worker threadpool. Here there are multiple workers executing code at the same time. In our worker, we have the following line:
Copy link
Contributor

Choose a reason for hiding this comment

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

node-cpp.md Outdated

![concurrent](https://mapbox.s3.amazonaws.com/cpp-assets/addon-hey-concurrent.gif)

We run the following script node index.js. Here’s the output:
Copy link
Contributor

Choose a reason for hiding this comment

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

When we run the script again from within the threadpool, here's the output:

node-cpp.md Outdated
raeol
```

That’s a messy burger! 🍔 std::cout is logging at the same time and space is being filled concurrently. We’re literally seeing concurrency happen here–awesome! This makes testing pretty hard though. There are a few options to get your logs in order, such as adding a mutex to keep things straight. Since we’re just testing, we can tell our computer to only run this script with a single thread - forcing our application to run non-concurrently. We pass the following before running our script:
Copy link
Contributor

Choose a reason for hiding this comment

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

Link to mutex (not in our glossary yet)

node-cpp.md Outdated
royal with cheese
royal with cheese
royal with cheese
```

### How is it different than a C++ project?
Copy link
Contributor

Choose a reason for hiding this comment

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

How is an Node addon different than a C++ project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

a an Node addon

node-cpp.md Outdated
Developing an addon requires Node.js, NPM, and a C++ compiler.

* makefile: home to all of the development commands for building binaries, installing dependencies, and running tests
Developing an addon requires Node.js, NPM, and a C++ compiler. Check out node-cpp-skel's "extended tour" for an up-to-date and opinionated approach to developing an addon. Additinally, the repository has docs on [running benchmarks](https://github.com/mapbox/node-cpp-skel/blob/master/docs/benchmarking.md), [publishing binaries](https://github.com/mapbox/node-cpp-skel/blob/master/docs/publishing-binaries.md), and a breakdown of all the [necessary components](https://github.com/mapbox/node-cpp-skel/blob/master/docs/extended-tour.md#configuration-files) to make it run.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mapsam
Copy link
Contributor Author

mapsam commented Nov 14, 2017

@GretaCB 👍 for all these edits. Thank you!

@GretaCB
Copy link
Contributor

GretaCB commented Nov 14, 2017

@mapsam Just pushed some more changes

@GretaCB
Copy link
Contributor

GretaCB commented Nov 14, 2017

remove "naming conventions" since it's a little too opinionated and we don't even follow it

Agreed that from an open source perspective, this is opinionated. Perhaps we can add a blurb of what we've found useful and mostly implemented to help differentiate between node and C++ projects, for example

  • vt-shaver
  • vt-shaver-cpp
  • geojson-cpp
  • geojson.hpp o_O

@mapsam
Copy link
Contributor Author

mapsam commented Nov 14, 2017

@GretaCB how about we add it back once we can review the rest of our repos? That section was added purely from my own perspective, but I imagine so many more of our repositories follow other conventions as well, i.e. starting with mapbox- first 🤷‍♂️

@mapsam
Copy link
Contributor Author

mapsam commented Nov 14, 2017

Thanks for the changes @GretaCB! 🙏

@GretaCB
Copy link
Contributor

GretaCB commented Nov 16, 2017

@mapsam just tied in a bunch of other pieces and added more glossary terms. If looks 👍 , I can merge

@mapsam
Copy link
Contributor Author

mapsam commented Nov 16, 2017

@GretaCB thanks so much! Merge away :shipit:

@GretaCB GretaCB merged commit 3a9b478 into master Nov 16, 2017
@GretaCB GretaCB deleted the concurrency-additions branch November 16, 2017 23:40
@springmeyer
Copy link
Contributor

Great work on this @mapsam @GretaCB.

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.

Move node-cpp-versioning into this repo
3 participants