Use node-api instead of NAN #6452

SiarheiFedartsou · 2022-11-11T20:48:26Z

Issue

The problem is that with nan we have to rebuild binary for each major node version, e.g. it is one of the reasons why we have no support for Node 18 on Windows(we could add one more job though on CI, but it requires a lot of boilerplate etc). After migration to Node-API our binaries will become "forward compatible", i.e. they will work with any new Node version which support NAPI version it was built with. Besides that node-api(to be precise its C++ wrapper node-addon-api we use here) has less verbose and more readable API rather than NAN.

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-11-11T21:06:50Z

I was a bit afraid that it could cause some drop of performance, but I benchmarked it with quite simple script available in PR and it seems there are no any issues with this(it seems that Node API version is even slightly faster).

Result of benchmark.js(see code in PR) on this branch(numbers are in microseconds):

➜  osrm-backend git:(sf-node-api) ✗ sh bench.sh
Histogram {
  min: 5420,
  max: 9095,
  mean: 5616.318,
  exceeds: 0,
  stddev: 311.16782429422227,
  count: 1000,
  percentiles: SafeMap(12) [Map] {
    0 => 5420,
    50 => 5496,
    75 => 5624,
    87.5 => 5824,
    93.75 => 6140,
    96.875 => 6324,
    98.4375 => 6768,
    99.21875 => 7100,
    99.609375 => 7692,
    99.8046875 => 7992,
    99.90234375 => 9088,
    100 => 9088
  }
}
➜  osrm-backend git:(sf-node-api) ✗ sh bench.sh
Histogram {
  min: 5416,
  max: 7163,
  mean: 5512.436,
  exceeds: 0,
  stddev: 129.44126816436867,
  count: 1000,
  percentiles: SafeMap(12) [Map] {
    0 => 5416,
    50 => 5468,
    75 => 5516,
    87.5 => 5588,
    93.75 => 5656,
    96.875 => 5824,
    98.4375 => 6012,
    99.21875 => 6136,
    99.609375 => 6380,
    99.8046875 => 6576,
    99.90234375 => 7160,
    100 => 7160
  }
}
➜  osrm-backend git:(sf-node-api) ✗ sh bench.sh
Histogram {
  min: 5436,
  max: 8367,
  mean: 5692.902,
  exceeds: 0,
  stddev: 334.17535276558027,
  count: 1000,
  percentiles: SafeMap(12) [Map] {
    0 => 5436,
    50 => 5520,
    75 => 5828,
    87.5 => 6028,
    93.75 => 6268,
    96.875 => 6440,
    98.4375 => 6792,
    99.21875 => 7008,
    99.609375 => 7460,
    99.8046875 => 8048,
    99.90234375 => 8360,
    100 => 8360
  }
}

On master:

➜  osrm-backend git:(master) ✗ sh bench.sh
Histogram {
  min: 5500,
  max: 8639,
  mean: 5884.29,
  exceeds: 0,
  stddev: 271.74253973200445,
  count: 1000,
  percentiles: SafeMap(12) [Map] {
    0 => 5500,
    50 => 5820,
    75 => 6004,
    87.5 => 6104,
    93.75 => 6284,
    96.875 => 6532,
    98.4375 => 6780,
    99.21875 => 6932,
    99.609375 => 7304,
    99.8046875 => 7480,
    99.90234375 => 8632,
    100 => 8632
  }
}
➜  osrm-backend git:(master) ✗ sh bench.sh
Histogram {
  min: 5492,
  max: 7891,
  mean: 5854.16,
  exceeds: 0,
  stddev: 246.93598846664702,
  count: 1000,
  percentiles: SafeMap(12) [Map] {
    0 => 5492,
    50 => 5804,
    75 => 5976,
    87.5 => 6060,
    93.75 => 6172,
    96.875 => 6436,
    98.4375 => 6664,
    99.21875 => 6808,
    99.609375 => 7232,
    99.8046875 => 7636,
    99.90234375 => 7888,
    100 => 7888
  }
}
➜  osrm-backend git:(master) ✗ sh bench.sh
Histogram {
  min: 5456,
  max: 9703,
  mean: 5886.806,
  exceeds: 0,
  stddev: 271.55144331047114,
  count: 1000,
  percentiles: SafeMap(12) [Map] {
    0 => 5456,
    50 => 5852,
    75 => 6008,
    87.5 => 6092,
    93.75 => 6172,
    96.875 => 6384,
    98.4375 => 6696,
    99.21875 => 6996,
    99.609375 => 7272,
    99.8046875 => 7712,
    99.90234375 => 9696,
    100 => 9696
  }
}

In both cases OSRM was built as cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DENABLE_NODE_BINDINGS=ON .. && ninja

Full code of benchmark(route from Gdansk to Krakow, i.e. ~600km):

REGION=poland-latest

mkdir -p $REGION
cd $REGION
wget http://download.geofabrik.de/europe/$REGION.osm.pbf
../build/osrm-extract --profile ../profiles/car.lua $REGION.osm.pbf
../build/osrm-partition ./$REGION
../build/osrm-customize ./$REGION
cd ..


node test/nodejs/benchmark.js $REGION/$REGION.osrm "18.638306,54.372158;19.944544,50.049683"

Node Version which was used:

➜  osrm-backend git:(master) ✗ node --version
v18.7.0

Hardware:

Macbook Pro with Apple M1 Pro and 32 gb RAM

SiarheiFedartsou · 2022-11-12T11:09:47Z

package.json

  },
  "main": "lib/index.js",
  "binary": {
+    "napi_versions": [8],


Version 8 is the latest and it is compatible with any Node version we currently support:

https://nodejs.org/api/n-api.html#node-api-version-matrix

SiarheiFedartsou · 2022-11-12T11:12:09Z

package.json

    "module_name": "node_osrm",
-    "module_path": "./lib/binding/",
+    "module_path": "./lib/binding_napi_v{napi_build_version}/",


node-pre-gyp is designed to support multiple NAPI versions, that's why they require NAPI version to be part of module_path(as I understand it may be needed if you want to use some features from new versions of NAPI, but still provide builds for older Node versions which do not support this new version of NAPI) https://github.com/mapbox/node-pre-gyp#the-napi_versions-array-property

SiarheiFedartsou · 2022-11-12T11:13:49Z

src/nodejs/CMakeLists.txt

@@ -21,6 +25,14 @@ set_target_properties(node_osrm PROPERTIES CXX_CLANG_TIDY "")
 target_no_warning(node_osrm suggest-destructor-override)
 target_no_warning(node_osrm suggest-override)

+# https://github.com/cjntaylor/node-cmake/issues/53#issuecomment-842357457


We could use https://github.com/cmake-js/cmake-js here which is recommended by Node API docs, but it would require much more refactoring in the way how we build our binaries: they force us to use their own node script to run cmake commands what is not very convenient.

SiarheiFedartsou · 2022-11-12T11:15:05Z

include/nodejs/node_osrm_support.hpp

    }
 }

+inline bool IsUnsignedInteger(const Napi::Value &value)


Node API doesn't distinguish integers and floats, so I had to add this to be compatible with some input parameter checks we had before.

SiarheiFedartsou · 2022-11-12T11:17:05Z

.github/workflows/osrm-backend.yml

        npm run nodejs-tests
+    - name: Use Node 18


Paranoia check that resulting binary is compatible with any Node version we currently support. I am going to revisit it and probably try to do it in more elegant and less verbose way(probably we can use nvm instead of GitHub actions in order to switch Node versions)

SiarheiFedartsou · 2022-11-12T11:18:37Z

Hi @mjjbell @DennisOSRM
I believe there are some things to be polished in this PR, but lets may be have preliminary review/discussion and the finish it :) Do you see any red flags with this PR?

SiarheiFedartsou · 2022-11-12T11:19:42Z

.github/workflows/osrm-backend.yml

@@ -264,83 +264,7 @@ jobs:
            CXXCOMPILER: g++-8
            CXXFLAGS: -Wno-cast-function-type

-          - name: conan-macos-x64-release-node-16


This PR allowed to get rid from this extra jobs, but at the same time we can now be compatible with more Node versions.

DennisOSRM · 2022-11-13T17:30:24Z

Hi @mjjbell @DennisOSRM
I believe there are some things to be polished in this PR, but lets may be have preliminary review/discussion and the finish it :) Do you see any red flags with this PR?

No red flags from my side.

SiarheiFedartsou added 8 commits November 6, 2022 14:45

Apply workaround for Conan installation on CI

95e2509

wip

d280475

wip

24e5d3b

wip

0a558c0

wip

3e7d067

wip

ac51de4

wip

3ab5bee

wip

b67529e

SiarheiFedartsou added 2 commits November 11, 2022 22:07

wip

1c41efe

wip

039768d

SiarheiFedartsou force-pushed the sf-node-api branch from 399facf to 039768d Compare November 11, 2022 21:16

SiarheiFedartsou added 10 commits November 11, 2022 22:23

wip

6880564

wip

73fb77e

wip

58081a3

wip

f5dcacd

wip

50ae613

wip

cc8ae97

wip

c7d8284

Merge branch 'master' into sf-node-api

2eac950

wip

824a725

wip

8a9efaa

SiarheiFedartsou commented Nov 12, 2022

View reviewed changes

SiarheiFedartsou marked this pull request as ready for review November 12, 2022 11:18

SiarheiFedartsou commented Nov 12, 2022

View reviewed changes

DennisOSRM approved these changes Nov 13, 2022

View reviewed changes

SiarheiFedartsou added 3 commits November 14, 2022 18:40

wip

3ff63eb

wip

c43a24c

wip

1f25c25

SiarheiFedartsou merged commit 18dcd27 into master Nov 16, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Use node-api instead of NAN (Project-OSRM#6452)

87b474f

DennisOSRM deleted the sf-node-api branch May 3, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use node-api instead of NAN #6452

Use node-api instead of NAN #6452

SiarheiFedartsou commented Nov 11, 2022 •

edited

Loading

SiarheiFedartsou commented Nov 11, 2022 •

edited

Loading

SiarheiFedartsou Nov 12, 2022

SiarheiFedartsou Nov 12, 2022

SiarheiFedartsou Nov 12, 2022

SiarheiFedartsou Nov 12, 2022

SiarheiFedartsou Nov 12, 2022

SiarheiFedartsou commented Nov 12, 2022

SiarheiFedartsou Nov 12, 2022 •

edited

Loading

DennisOSRM commented Nov 13, 2022

Use node-api instead of NAN #6452

Use node-api instead of NAN #6452

Conversation

SiarheiFedartsou commented Nov 11, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou commented Nov 11, 2022 • edited Loading

SiarheiFedartsou Nov 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Nov 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Nov 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Nov 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Nov 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Nov 12, 2022

SiarheiFedartsou Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

DennisOSRM commented Nov 13, 2022

SiarheiFedartsou commented Nov 11, 2022 •

edited

Loading

SiarheiFedartsou commented Nov 11, 2022 •

edited

Loading

SiarheiFedartsou Nov 12, 2022 •

edited

Loading