Add object_async example #52

GretaCB · 2017-07-11T23:00:47Z

This object-async example builds off the async standalone example and the object-sync example

Next Actions

Add code comments and relevant docs ("When would you use an async object or class?")
All possible Test Coverage for this example
Add some context around node::ObjectWrap and how it relates to Unwrap

Questions

@springmeyer Does this example hit any of the questions we pondered in [WIP] Questions/Brainstorming #50 ?
Does this custom constructor design make sense with an async object?
Is this PR relevant to real world usage of an async object?
Curious exactly what is node::ObjectWrap and how it relates to Unwrap

Any other thoughts/comments/edits?

cc @springmeyer @daniel-j-h @flippmoke

codecov-io · 2017-07-11T23:16:23Z

Codecov Report

Merging #52 into master will increase coverage by 0.37%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   92.85%   93.23%   +0.37%     
==========================================
  Files           5        8       +3     
  Lines          70      133      +63     
==========================================
+ Hits           65      124      +59     
- Misses          5        9       +4

Impacted Files	Coverage Δ
src/object_async/hello_async.hpp	`0% <0%> (ø)`
src/standalone_async/hello_async.cpp	`94.28% <100%> (-0.31%)`	⬇️
src/module.cpp	`100% <100%> (ø)`	⬆️
src/module_utils.hpp	`100% <100%> (ø)`
src/object_async/hello_async.cpp	`95.08% <95.08%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 324f034...99deaec. Read the comment docs.

springmeyer · 2017-07-12T01:38:21Z

src/object_async/hello_async.cpp

+  	v8::Local<v8::Value> argv[1] = { Nan::Error(message.c_str()) };
+  	Nan::MakeCallback(Nan::GetCurrentContext()->Global(), callback, 1, argv);
+  }
+


I see this same function getting repeated in multiple .cpp files. How about we refactor the code to move it to a new header file called module_utils.hpp? Then define it there, include module_utils.hpp in each of the cpp files that uses it, and that will reduce the code duplication.

Note: for fun context because of the inline keyword is used the compiler is doing away with this function in the binary code. When something is "inlined" it is re-arranged and optimized. So the v8::Local<v8::Value>... and Nan::MakeCallback... lines are being moved to where the code is calling CallbackError. This means that moving the source code into a common header will only impact how the source code looks. The final binary will probably not change at all because of how the compiler works.

@springmeyer so inline is great for reusable methods within a file? And a util header is one step further than that, great for reusable methods within an application/directory

@springmeyer so inline is great for reusable methods within a file?

inlining, when in a cpp file, may help performance.

inlining, when in an hpp file, may help performance and may also be absolutely necessary to allow the header to be included from multiple cpp files when the code in the header includes both the definition and the implementation (which a "header-only" library does). And example of this being fixed is Non-inline function definitions in header file vector-tile#19.

So, inlining is important to understand because it impacts performance, code size, and is critical to header-only library design.

And a util header is one step further than that, great for reusable methods within an application/directory

Exactly. If you want to share code in multiple .cpp files then you need to put at least definitions for it in an .hpp file (and, in the case of header-only design, also the implementation)

Ticketed adding this to the C++ glossary at mapbox/cpp#28

Finished at mapbox/cpp#29

springmeyer · 2017-07-12T01:50:43Z

src/object_async/hello_async.cpp

+    // Mention anything about "Unwrap"?
+    HelloObjectAsync* h = Nan::ObjectWrap::Unwrap<HelloObjectAsync>(info.Holder());
+
+    std::string name = h->name_;


Minor issue: this line is resulting in an uncessary copy. We are copying the h->name_ to a new variable named name. Instead, down below you could do:

new AsyncHelloWorker{louder, h->name_, new Nan::Callback{callback}};

That would avoid the copy. Or alternatively you could do:

std::string const& name = h->name_;

That would also avoid the copy by making the name variable a reference to the h->name_.

Why does this matter?

In short, performance. If this code needed to be as fast as possible we want to avoid all possible copies. Because copies require allocation (or re-allocation of memory) for the new variables space. And std::string allocates memory on the heap (internally) (https://github.com/mapbox/cpp/blob/master/glossary.md#heap-allocation) when newly created.

Since you're pointing out string copies here - not sure if it makes sense to use move-semantics throughout this example (e.g. to move the string into the async worker). Maybe it just makes it more complicated than it has to be..

👍 great suggestion @daniel-j-h - I agree that move semantics here would likely make the most sense given that the goal of node-cpp-skel is to set developers up for writing high performance code.

springmeyer · 2017-07-12T01:51:33Z

src/object_async/hello_async.cpp

+
+  // Expensive allocation of std::map, querying, and string comparison,
+  // therefore threads are busy
+  std::string do_expensive_work(bool louder, std::string name) {  


To avoid a copy here pass by reference like:

std::string do_expensive_work(bool louder, std::string const& name) {

springmeyer · 2017-07-12T01:52:23Z

src/object_async/hello_async.cpp

+
+      using Base = Nan::AsyncWorker;  
+
+      AsyncHelloWorker(bool louder, std::string name, Nan::Callback* callback)


Another spot where we can avoid a copy:

Best to pass by reference like:

AsyncHelloWorker(bool louder, std::string const& name, Nan::Callback* callback)

springmeyer · 2017-07-12T01:55:40Z

src/object_async/hello_async.cpp

+            else {
+                auto *const self = new HelloObjectAsync();
+                self->Wrap(info.This());
+            } 


I think a more realistic thing would be to throw an error if no argument is passed. Currently we default to world below. But I think instead we should throw a JS error here if no name arg is passed. What do you think?

springmeyer · 2017-07-12T02:00:34Z

@springmeyer Does this example hit any of the questions we pondered in #50 ?

Yes, perfectly. We are passing in a custom class that holds a custom data member. That member is safe to pass into the threadpool because it is not a V8 object, but rather a std::string.

Does this custom constructor design make sense with an async object?

You mean the custom constructor that accepts the name arg? Yes, I think this makes sense. However I did make one inline comment above that I think we should require this argument rather than allowing the object to be created without an arg.

Is this PR relevant to real world usage of an async object?

It is. Well done!

Curious exactly what is node::ObjectWrap and how it relates to Unwrap

We spoke about this voice. The short version is that node::ObjectWrap and wrapping/unwrapping objects is the somewhat clumsy way it is possible to bind the two languages. Let's not dig deeper than just knowing:

To access a class instance inside a C++ static method you must unwrap the object.
The C++ methods must be static to make them available at startup across the language boundary.

daniel-j-h · 2017-07-12T09:15:11Z

src/object_async/hello_async.cpp

+    {
+        return Nan::ThrowTypeError(
+            "Cannot call constructor as function, you need to use 'new' keyword");
+    }


I think we could handle both new T() as well as T() by adding this to the function's beginning:

if (!info.IsConstructCall()) { auto init = Nan::New(create_once()); info.GetReturnValue().Set(init->NewInstance()); return; } /*continue with New function impl*/

Not sure if we want to do that, though.

If not we could flatten the control flow here by exiting early on !IsConstructorCall().

@daniel-j-h - really great catch 🙇 . I agree that handling the case where a user forgets to use new in Javascript is important. I'm aware that some JS developers like to hide the complexity and allow object creation to work without new. But my preference would be to throw since this is a consistent pattern that has worked in other node C++ modules I have written. And I've never seen a complaint about this.

Going to lean towards being more explicit here and requiring new

daniel-j-h · 2017-07-12T09:19:20Z

src/object_async/hello_async.cpp

+    // Mention anything about "Unwrap"?
+    HelloObjectAsync* h = Nan::ObjectWrap::Unwrap<HelloObjectAsync>(info.Holder());
+
+    std::string name = h->name_;


Since you're pointing out string copies here - not sure if it makes sense to use move-semantics throughout this example (e.g. to move the string into the async worker). Maybe it just makes it more complicated than it has to be..

GretaCB · 2017-07-20T22:29:25Z

Thank you so much for your input @daniel-j-h @springmeyer 🙌

I've updated the PR with the optimization suggestions and comments/links for context. Once tests are 🍏 , I will merge! Feel free to add any other comments if I missed anything.

gretacb added 2 commits July 11, 2017 15:44

add object_async example

92882ca

add coverage and more comments

bdc66d4

springmeyer reviewed Jul 12, 2017

View reviewed changes

GretaCB mentioned this pull request Jul 12, 2017

[WIP] Questions/Brainstorming #50

Closed

4 tasks

springmeyer reviewed Jul 12, 2017

View reviewed changes

daniel-j-h reviewed Jul 12, 2017

View reviewed changes

gretacb added 4 commits July 13, 2017 11:48

Merge branch 'master' into object-async

8f3889d

move callbackError method into reusable util header

d050a99

add move semantics, comments, update tests

1089bce

include snippet on ObjectWrap

99deaec

GretaCB merged commit 6bd89f4 into master Jul 21, 2017

GretaCB deleted the object-async branch July 21, 2017 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add object_async example #52

Add object_async example #52

GretaCB commented Jul 11, 2017 •

edited

Loading

codecov-io commented Jul 11, 2017 •

edited

Loading

springmeyer Jul 12, 2017

GretaCB Jul 14, 2017 •

edited

Loading

springmeyer Jul 14, 2017

springmeyer Jul 14, 2017

springmeyer Jul 12, 2017

daniel-j-h Jul 12, 2017

springmeyer Jul 12, 2017

springmeyer Jul 12, 2017

springmeyer Jul 12, 2017

springmeyer Jul 12, 2017

springmeyer commented Jul 12, 2017

daniel-j-h Jul 12, 2017

springmeyer Jul 12, 2017

GretaCB Jul 20, 2017

daniel-j-h Jul 12, 2017

GretaCB commented Jul 20, 2017


		using Base = Nan::AsyncWorker;

		AsyncHelloWorker(bool louder, std::string name, Nan::Callback* callback)

Add object_async example #52

Add object_async example #52

Conversation

GretaCB commented Jul 11, 2017 • edited Loading

Next Actions

Questions

codecov-io commented Jul 11, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

GretaCB Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

springmeyer commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GretaCB commented Jul 20, 2017

GretaCB commented Jul 11, 2017 •

edited

Loading

codecov-io commented Jul 11, 2017 •

edited

Loading

GretaCB Jul 14, 2017 •

edited

Loading