N-API by artemp · Pull Request #90 · mapbox/vtcomposite

artemp · 2018-10-25T15:30:05Z

No description provided.

…373: exit: : numeric argument required)

e-n-f

I am no expert on native Node bindings, but it looks reasonable to me

springmeyer · 2018-10-29T12:07:37Z

This looks great @artemp - The next steps in my mind would be:

are there any performance differences? If so, can they be mitigated?
getting the binary build/publish to work so that we can publish based on NAPI version and have future compatibility.

mapsam

@artemp this looks pretty great! Thanks so much for running through this port. Per @springmeyer's suggestion - you should be able to run benchmarks with the steps listed at https://github.com/mapbox/vtcomposite#benchmarks. Once those are run, mind posting the results from master and this branch?

I'll start working on the vtquery port as well, to get a better sense of the N-API flow.

mapsam · 2018-10-29T16:32:39Z

 #include "vtcomposite.hpp"
-#include <nan.h>
+#include <napi.h>
+#include <uv.h>


Reading a little about guaranteed ABI stability across major node versions and it includes <uv.h> as one of the ports that does not guarantee stability. And then continues to say, if you want a stable napi include, to use #include <node_api.h> - Will this be an issue?

#include <napi.h> is a correct header from c++ wrapper : https://github.com/nodejs/node-addon-api/blob/master/napi.h
But, I removed #include <uv.h> as we shouldn't use it, thanks.

mapsam · 2018-10-29T16:50:00Z

 */
-inline void CallbackError(std::string message, v8::Local<v8::Function> func)
+
+inline Napi::Value CallbackError(std::string const& message, Napi::CallbackInfo const& info, Napi::Function const& func)


Curious question: do the https://nodejs.org/api/n-api.html#n_api_n_api_callback_types types come into play here?

mapsam · 2018-10-29T17:16:59Z

      # cflags (linux) and xcode (mac)
      'system_includes': [
-        "-isystem <(module_root_dir)/<!(node -e \"require('nan')\")",
+        "-isystem <!@(node -p \"require('node-addon-api').include.slice(1,-1)\")",


@artemp what's the need for slicing the include instead of using

"<!@(node -p \"require('node-addon-api').include\")"

?

@mapsam this is the only way I found that works for me. The above expansion has too many quotes. Pls, suggest workaround, it's probably lack JS knowledge on my behalf.

artemp · 2018-10-30T15:20:39Z

@mapsam @springmeyer

N-API

time node bench/bench.js --iterations 50 --concurrency 1 --package vtcomposite --compress

1: single tile in/out ... 289 runs/s (173ms)
2: two different tiles at the same zoom level, zero buffer ... 25 runs/s (1985ms)
3: two different tiles from different zoom levels (separated by one zoom), zero buffer ... 26 runs/s (1960ms)
4: two different tiles from different zoom levels (separated by more than one zoom), zero buffer ... 27 runs/s (1885ms)
5: tiles completely made of points, overzooming, no properties ... 1389 runs/s (36ms)
6: tiles completely made of points, same zoom, no properties ... 1111 runs/s (45ms)
7: tiles completely made of points, overzoooming, lots of properties ... 820 runs/s (61ms)
8: tiles completely made of points, same zoom, lots of properties ... 345 runs/s (145ms)
9: buffer_size 128 - tiles completely made of points, same zoom, lots of properties ... 333 runs/s (150ms)
10: tiles completely made of linestrings, overzooming and lots of properties ... 397 runs/s (126ms)
11: tiles completely made of polygons, overzooming and lots of properties ... 167 runs/s (299ms)
12: tiles completely made of points and linestrings, overzooming and lots of properties ... 3571 runs/s (14ms)
13: buffer_size 128 - tiles completely made of points and linestrings, overzooming and lots of properties ... 4167 runs/s (12ms)
14: tiles completely made of points and linestrings, overzooming (2x) and lots of properties ... 5556 runs/s (9ms)
15: tiles completely made of polygons, overzooming and lots of properties ... 515 runs/s (97ms)
16: tiles completely made of polygons, overzooming (2x) and lots of properties ... 962 runs/s (52ms)
17: buffer_size 4096 - tiles completely made of polygons, overzooming (2x) and lots of properties ... 526 runs/s (95ms)
Note: pass --mem to track memory usage
Benchmark iterations: 50 concurrency: 1

real	0m7.267s
user	0m7.360s
sys	0m0.089s

NAN

time node bench/bench.js --iterations 50 --concurrency 1 --package vtcomposite --compress

1: single tile in/out ... 289 runs/s (173ms)
2: two different tiles at the same zoom level, zero buffer ... 25 runs/s (2003ms)
3: two different tiles from different zoom levels (separated by one zoom), zero buffer ... 26 runs/s (1945ms)
4: two different tiles from different zoom levels (separated by more than one zoom), zero buffer ... 27 runs/s (1875ms)
5: tiles completely made of points, overzooming, no properties ... 1316 runs/s (38ms)
6: tiles completely made of points, same zoom, no properties ... 1136 runs/s (44ms)
7: tiles completely made of points, overzoooming, lots of properties ... 833 runs/s (60ms)
8: tiles completely made of points, same zoom, lots of properties ... 345 runs/s (145ms)
9: buffer_size 128 - tiles completely made of points, same zoom, lots of properties ... 345 runs/s (145ms)
10: tiles completely made of linestrings, overzooming and lots of properties ... 400 runs/s (125ms)
11: tiles completely made of polygons, overzooming and lots of properties ... 167 runs/s (299ms)
12: tiles completely made of points and linestrings, overzooming and lots of properties ... 4167 runs/s (12ms)
13: buffer_size 128 - tiles completely made of points and linestrings, overzooming and lots of properties ... 4167 runs/s (12ms)
14: tiles completely made of points and linestrings, overzooming (2x) and lots of properties ... 5000 runs/s (10ms)
15: tiles completely made of polygons, overzooming and lots of properties ... 515 runs/s (97ms)
16: tiles completely made of polygons, overzooming (2x) and lots of properties ... 962 runs/s (52ms)
17: buffer_size 4096 - tiles completely made of polygons, overzooming (2x) and lots of properties ... 521 runs/s (96ms)
Note: pass --mem to track memory usage
Benchmark iterations: 50 concurrency: 1

real	0m7.387s
user	0m7.488s
sys	0m0.064s

…d storing `Napi::Reference<Napi::Buffer<char>>` in TileObject.

This reverts commit 09aef2e.

This reverts commit 6a77f2e.

artemp added 4 commits October 25, 2018 14:24

Initial implementation useing N-API via node-addon-api

698fa0f

check CallbackInfo index is valid + format/tidy

f2c8360

attempting to fix .travis.yml (/home/travis/.travis/job_stages: line …

f1765fd

…373: exit: : numeric argument required)

Perhaps more generic approach to obtaining 'node-addon-api' include path

7aabb89

artemp self-assigned this Oct 25, 2018

artemp requested review from e-n-f and mapsam October 25, 2018 15:30

e-n-f approved these changes Oct 25, 2018

View reviewed changes

Merge branch 'master' into n-api

23eeb11

mapsam reviewed Oct 29, 2018

View reviewed changes

remove <uv,h>

18e5359

artemp added 5 commits October 31, 2018 13:12

Simplify CallbackError signature

4efb356

use As<Napi::Object> consistently

ae290da

remove redundant conversion

93806a9

Minimise internal type conversions by passing Napi::Buffer<char> an…

4f50896

…d storing `Napi::Reference<Napi::Buffer<char>>` in TileObject.

Merge branch 'master' into n-api

86e4ab1

springmeyer mentioned this pull request Dec 7, 2018

Port to NAPI mapbox/vtshaver#22

Open

artemp and others added 10 commits January 23, 2019 10:48

Merge branch 'master' into n-api

0694dbe

bring back node-pre-gyp :)

4ab12a5

4x faster polygon decoding

b28077a

clang-format

c4daa7b

initialise ring_ member variable in initialiser list

44b8ef2

attempting to fix codecov

6a77f2e

update pip

09aef2e

Revert "update pip"

be4c9e8

This reverts commit 09aef2e.

Revert "attempting to fix codecov"

a7395ad

This reverts commit 6a77f2e.

codecov : add more dirs for ignore

8cef8a6

stderr $PATH

49cd191

artemp merged commit c3a0f11 into master Jan 25, 2019

artemp added 3 commits January 25, 2019 11:11

remove relative path from $PATH

7ef8a80

fix prev commit

ed85265

cleanup

463faa7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

N-API#90

N-API#90
artemp merged 25 commits intomasterfrom
n-api

artemp commented Oct 25, 2018

Uh oh!

e-n-f left a comment

Uh oh!

springmeyer commented Oct 29, 2018 •

edited

Loading

Uh oh!

mapsam left a comment

Uh oh!

mapsam Oct 29, 2018

Uh oh!

artemp Oct 30, 2018

Uh oh!

mapsam Oct 29, 2018

Uh oh!

mapsam Oct 29, 2018

Uh oh!

artemp Oct 30, 2018

Uh oh!

artemp commented Oct 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

artemp commented Oct 25, 2018

Uh oh!

e-n-f left a comment

Choose a reason for hiding this comment

Uh oh!

springmeyer commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mapsam left a comment

Choose a reason for hiding this comment

Uh oh!

mapsam Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

artemp Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

mapsam Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

mapsam Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

artemp Oct 30, 2018

Choose a reason for hiding this comment

Uh oh!

artemp commented Oct 30, 2018

N-API

NAN

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

springmeyer commented Oct 29, 2018 •

edited

Loading