Implement a more efficient vecFromJSArray #11119

Lectem · 2020-05-09T22:13:12Z

This is a followup of #5519 and #5655 since they were closed.

It is possible to implement emscripten::vecFromJSArray more efficiently for numeric arrays by using the optimized TypedArray.prototype.set function.

The main issue with this method is that it will silently fail(or succeed) if elements of the array or not numbers, as it does not do any type checking but instead works as if it called the javascript Number() function for each element. (See ToNumber for more details)

So instead of simply updating vecFromJSArray to use this new implementation and break code (since there's no typechecking anymore) I added a new convertJSArrayToNumberVector (name subject to change) and improved performance a tiny bit for vecFromJSArray by:

Taking the val parameter by const reference instead of copy
Reserving the storage of the vector

I benchmarked locally with node.js (v12.16.3) and here are the timings I got:

complexityN	ns/op	op/s	err%	vecFromJSArray
1	469.41	2,130,354.23	1.1%	`vecFromJSArray-Old`
1	434.65	2,300,682.86	0.8%	`vecFromJSArray-New`
1	554.89	1,802,160.82	0.8%	`convertJSArrayToNumberVector`
2	681.91	1,466,477.20	0.5%	`vecFromJSArray-Old`
2	607.00	1,647,437.94	1.0%	`vecFromJSArray-New`
2	559.08	1,788,654.17	0.8%	`convertJSArrayToNumberVector`
4	1,120.90	892,142.81	4.8%	`vecFromJSArray-Old`
4	1,009.54	990,547.11	0.7%	`vecFromJSArray-New`
4	594.34	1,682,541.26	1.1%	`convertJSArrayToNumberVector`
8	1,750.80	571,168.57	0.4%	`vecFromJSArray-Old`
8	1,467.54	681,413.02	1.9%	`vecFromJSArray-New`
8	594.47	1,682,180.70	0.8%	`convertJSArrayToNumberVector`
16	2,981.75	335,374.01	0.4%	`vecFromJSArray-Old`
16	2,641.99	378,501.89	2.2%	`vecFromJSArray-New`
16	568.48	1,759,091.27	0.9%	`convertJSArrayToNumberVector`
32	5,453.92	183,354.39	0.7%	`vecFromJSArray-Old`
32	4,839.54	206,631.19	0.2%	`vecFromJSArray-New`
32	615.68	1,624,219.01	1.3%	`convertJSArrayToNumberVector`
64	10,255.66	97,507.14	0.4%	`vecFromJSArray-Old`
64	9,472.98	105,563.37	0.8%	`vecFromJSArray-New`
64	618.15	1,617,723.64	0.9%	`convertJSArrayToNumberVector`
128	19,951.11	50,122.53	0.4%	`vecFromJSArray-Old`
128	18,672.15	53,555.69	0.8%	`vecFromJSArray-New`
128	690.25	1,448,749.45	0.9%	`convertJSArrayToNumberVector`
256	38,982.72	25,652.39	0.4%	`vecFromJSArray-Old`
256	37,108.51	26,948.00	0.1%	`vecFromJSArray-New`
256	803.84	1,244,022.24	0.7%	`convertJSArrayToNumberVector`
512	76,771.60	13,025.65	0.2%	`vecFromJSArray-Old`
512	74,031.69	13,507.73	0.6%	`vecFromJSArray-New`
512	1,045.95	956,065.05	0.6%	`convertJSArrayToNumberVector`
1,024	152,126.01	6,573.50	0.4%	`vecFromJSArray-Old`
1,024	148,383.52	6,739.29	0.2%	`vecFromJSArray-New`
1,024	1,541.85	648,571.68	0.2%	`convertJSArrayToNumberVector`

jgravelle-google

Nice work, sorry I let this languish for so long

I think the approach of adding a new API rather than break the old one is a good one. And the name is fine as-is, the length of it makes it harder to misuse.

Also PRs with benchmarks is great to see :)
Nicely done!

system/include/emscripten/val.h

jgravelle-google · 2020-06-04T21:07:24Z

OK I can't for the life of me figure out how to resolve the AUTHORS conflict in the web UI. Will take another pass on this later, after seeing how the tests go

Lectem · 2020-06-05T08:40:27Z

OK I can't for the life of me figure out how to resolve the AUTHORS conflict in the web UI

Same issue here, keeps telling me the file is out of date. I can resolve it manually if you want?

It seems the failing tests are unrelated to my changes ? Seems like the NODE_JS env var is missing in CircleCI.

Lectem · 2020-06-05T09:03:13Z

Somehow pushing new commits to my fork branch is not updating the pull request... I think you might need to merge this one manually (or push to emscripten-core/emscripten/pr-11119-merge ? it's weird it exists).

kripken · 2020-06-08T17:01:29Z

In general this looks good and useful, thanks @Lectem !

Before landing we should add a test for this.

Also, #11158 is still open - @jgravelle-google , it might be good to have that fixed first so we are more confident about the safety of things like this?

jgravelle-google · 2020-06-08T18:58:49Z

Looks like I screwed this up trying to resolve it, sorry :|
Seems the branch is Lectem/emscripten/pr-11119-merge. In principle we should be able to retarget it, but it might be simplest to just work off that one. Also it looks like the conflict is resolved, so this commit of yours seems to have fixed it.

@Lectem can you add a test for this? Looks like we currently have tests for vecFromJSArray in tests/embind/test_val.cpp... right next to the handy note that is isn't a great place for that to live. We can find a better place in a follow-up I think.

system/include/emscripten/val.h

Lectem · 2020-06-30T09:20:32Z

@Lectem can you add a test for this? Looks like we currently have tests for vecFromJSArray in tests/embind/test_val.cpp... right next to the handy note that is isn't a great place for that to live. We can find a better place in a follow-up I think.

Sorry I have been quite busy lately, I'll try to add some tests asap.

…onversion

kripken · 2020-07-01T17:15:45Z

tests/embind/test_val.cpp

@@ -570,19 +571,41 @@ int main()
  ensure_js("test_val_throw_(new TypeError('message'))");

  // this test should probably go elsewhere as it is not a member of val
-  test("template<typename T> std::vector<T> vecFromJSArray(val v)");
+  test("template<typename T> std::vector<T> vecFromJSArray(const val& v)");


I don't know the embind code or tests well, but it would be better to add new tests rather than modify existing ones - to make review easier, and to avoid the risk of losing some test coverage. Unless this is actually a necessary change?

I can split it if you want, I just thought that there was no need to go through EM_ASM for almost the same setup code

Let's see what @jgravelle-google thinks - maybe I don't know embind enough to tell if these test changes are obviously ok or not.

Ping @jgravelle-google to know if the changes are ok or not

kripken · 2020-07-01T17:16:24Z

The CI errors might be fixed by merging in latest upstream.

kripken · 2020-07-30T17:31:21Z

I'm not sure if @jgravelle-google is available to review this. Perhaps @Brion or @chadaustin ?

chadaustin

No objections here!

system/include/emscripten/val.h

Co-authored-by: Chad Austin <[email protected]>

kripken · 2020-10-07T18:11:53Z

To save time I did a rebase and landed this in #12463, so we can close this.

Thanks @Lectem and @chadaustin !

This change replaces the [`vecFromJSArray()`](https://emscripten.org/docs/api_reference/val.h.html?highlight=vecfromjsarray#_CPPv4N10emscripten10emscripten3val14vecFromJSArrayERK3val) call in `vec_from_val()` with a call to [`convertJSArrayToNumberVector()`](https://emscripten.org/docs/api_reference/val.h.html?highlight=convertjsarraytonumbervector#_CPPv4N10emscripten10emscripten3val28convertJSArrayToNumberVectorERK3val), which reduces the time consumption of Postject from ~30s to ~6s on a Mach-O Node.js binary when run on my x86_64 macOS. Fixes: nodejs#85 Refs: emscripten-core/emscripten#11119 Signed-off-by: Darshan Sen <[email protected]>

* feat: make postject faster This change replaces the [`vecFromJSArray()`](https://emscripten.org/docs/api_reference/val.h.html?highlight=vecfromjsarray#_CPPv4N10emscripten10emscripten3val14vecFromJSArrayERK3val) call in `vec_from_val()` with a call to [`convertJSArrayToNumberVector()`](https://emscripten.org/docs/api_reference/val.h.html?highlight=convertjsarraytonumbervector#_CPPv4N10emscripten10emscripten3val28convertJSArrayToNumberVectorERK3val), which reduces the time consumption of Postject from ~30s to ~6s on a Mach-O Node.js binary when run on my x86_64 macOS. Fixes: #85 Refs: emscripten-core/emscripten#11119 Signed-off-by: Darshan Sen <[email protected]> * fix: increase timeout to pass tests on Windows Refs: https://app.circleci.com/pipelines/github/RaisinTen/postject/7/workflows/4bfbf11d-4796-459d-a339-a28098670f37/jobs/77/tests Signed-off-by: Darshan Sen <[email protected]> --------- Signed-off-by: Darshan Sen <[email protected]>

Implement a more efficient vecFromJSArray

f32c023

kripken requested a review from jgravelle-google May 11, 2020 16:52

jgravelle-google approved these changes Jun 4, 2020

View reviewed changes

system/include/emscripten/val.h Show resolved Hide resolved

Merge branch 'master' of https://github.com/emscripten-core/emscripten

18a5b3a

earonesty reviewed Jun 27, 2020

View reviewed changes

system/include/emscripten/val.h Outdated Show resolved Hide resolved

Lectem added 2 commits June 30, 2020 12:38

Add unit tests for val.h convertJSArrayToNumberVector

ca3db9d

Update convertJSArrayToNumberVector comment to describe typed array c…

dcf1644

…onversion

kripken reviewed Jul 1, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into pr-11119-merge

2cfd456

Lectem requested a review from jgravelle-google July 27, 2020 07:26

kripken added the embind label Jul 30, 2020

chadaustin approved these changes Oct 4, 2020

View reviewed changes

system/include/emscripten/val.h Outdated Show resolved Hide resolved

system/include/emscripten/val.h Outdated Show resolved Hide resolved

Apply suggestions from code review

a4ed3b8

Co-authored-by: Chad Austin <[email protected]>

kripken mentioned this pull request Oct 7, 2020

Implement a more efficient vecFromJSArray #12463

Merged

kripken closed this Oct 7, 2020

RaisinTen mentioned this pull request May 15, 2023

feat: make postject faster nodejs/postject#86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a more efficient vecFromJSArray #11119

Implement a more efficient vecFromJSArray #11119

Lectem commented May 9, 2020

jgravelle-google left a comment

jgravelle-google commented Jun 4, 2020

Lectem commented Jun 5, 2020

Lectem commented Jun 5, 2020 •

edited

Loading

kripken commented Jun 8, 2020

jgravelle-google commented Jun 8, 2020

Lectem commented Jun 30, 2020

kripken Jul 1, 2020

Lectem Jul 2, 2020

kripken Jul 6, 2020

Lectem Jul 27, 2020

kripken commented Jul 1, 2020

kripken commented Jul 30, 2020

chadaustin left a comment

kripken commented Oct 7, 2020

Implement a more efficient vecFromJSArray #11119

Implement a more efficient vecFromJSArray #11119

Conversation

Lectem commented May 9, 2020

jgravelle-google left a comment

Choose a reason for hiding this comment

jgravelle-google commented Jun 4, 2020

Lectem commented Jun 5, 2020

Lectem commented Jun 5, 2020 • edited Loading

kripken commented Jun 8, 2020

jgravelle-google commented Jun 8, 2020

Lectem commented Jun 30, 2020

kripken Jul 1, 2020

Choose a reason for hiding this comment

Lectem Jul 2, 2020

Choose a reason for hiding this comment

kripken Jul 6, 2020

Choose a reason for hiding this comment

Lectem Jul 27, 2020

Choose a reason for hiding this comment

kripken commented Jul 1, 2020

kripken commented Jul 30, 2020

chadaustin left a comment

Choose a reason for hiding this comment

kripken commented Oct 7, 2020

Lectem commented Jun 5, 2020 •

edited

Loading