FromNapiValue trait for parameters #183

adespawn · Apr 12, 2025

Before, we created QueryParameterWrapper with from_... functions, and then cloning this object when executing query.

Now, with implementation of FromNapiValue trait, we pass parameters to queries directly, without any additional conversion steps. In order to correctly handle value types, each parameter is passed as a pair (type, value). Information about type is then used on the rust side to create correct CqlValue object.

This significantly improves performance for concurrent inserts benchmark, and slightly for plain inserts benchmark.

Concurrent inset. Around 30% improvement in time elapsed, 20% in task clock

After:

sudo perf stat node ./benchmark/logic/concurrent_insert.js scylladb-javascript-driver 1000000

 Performance counter stats for 'node ./benchmark/logic/concurrent_insert.js scylladb-javascript-driver 1000000':

         70,343.46 msec task-clock                       #    1.892 CPUs utilized             
         2,909,393      context-switches                 #   41.360 K/sec                     
            40,946      cpu-migrations                   #  582.087 /sec                      
           190,074      page-faults                      #    2.702 K/sec                     
   273,230,048,329      cycles                           #    3.884 GHz                       
    72,133,512,748      stalled-cycles-frontend          #   26.40% frontend cycles idle      
   205,423,999,244      instructions                     #    0.75  insn per cycle            
                                                  #    0.35  stalled cycles per insn   
    39,462,664,735      branches                         #  561.000 M/sec                     
     1,481,648,921      branch-misses                    #    3.75% of all branches           

      37.183329662 seconds time elapsed

      46.102799000 seconds user
      25.690728000 seconds sys

Before

sudo perf stat node ./benchmark/logic/concurrent_insert.js scylladb-javascript-driver 1000000

 Performance counter stats for 'node ./benchmark/logic/concurrent_insert.js scylladb-javascript-driver 1000000':

         86,873.69 msec task-clock                       #    1.633 CPUs utilized             
         3,221,740      context-switches                 #   37.085 K/sec                     
            48,446      cpu-migrations                   #  557.660 /sec                      
           190,596      page-faults                      #    2.194 K/sec                     
   344,373,066,097      cycles                           #    3.964 GHz                       
    87,340,988,626      stalled-cycles-frontend          #   25.36% frontend cycles idle      
   246,727,230,093      instructions                     #    0.72  insn per cycle            
                                                  #    0.35  stalled cycles per insn   
    47,143,750,188      branches                         #  542.670 M/sec                     
     1,750,876,479      branch-misses                    #    3.71% of all branches           

      53.200649700 seconds time elapsed

      60.397169000 seconds user
      28.058204000 seconds sys

Plain insert. Around 10% improvement in user time and number of instructions, 90% drop in page faults!

After:

sudo perf stat node ./benchmark/logic/insert.js scylladb-javascript-driver 300000

 Performance counter stats for 'node ./benchmark/logic/insert.js scylladb-javascript-driver 300000':

         30,435.03 msec task-clock                       #    0.839 CPUs utilized             
         2,126,428      context-switches                 #   69.868 K/sec                     
            19,269      cpu-migrations                   #  633.119 /sec                      
             8,034      page-faults                      #  263.972 /sec                      
   104,314,726,271      cycles                           #    3.427 GHz                       
    51,460,535,207      stalled-cycles-frontend          #   49.33% frontend cycles idle      
    91,254,504,996      instructions                     #    0.87  insn per cycle            
                                                  #    0.56  stalled cycles per insn   
    17,802,023,048      branches                         #  584.919 M/sec                     
     1,010,940,030      branch-misses                    #    5.68% of all branches           

      36.283305820 seconds time elapsed

      14.569449000 seconds user
      17.101418000 seconds sys

Before:

sudo perf stat node ./benchmark/logic/insert.js scylladb-javascript-driver 300000

 Performance counter stats for 'node ./benchmark/logic/insert.js scylladb-javascript-driver 300000':

         31,847.19 msec task-clock                       #    0.830 CPUs utilized             
         2,126,649      context-switches                 #   66.777 K/sec                     
            18,765      cpu-migrations                   #  589.220 /sec                      
            88,511      page-faults                      #    2.779 K/sec                     
   111,466,616,472      cycles                           #    3.500 GHz                       
    51,182,304,492      stalled-cycles-frontend          #   45.92% frontend cycles idle      
   100,318,097,101      instructions                     #    0.90  insn per cycle            
                                                  #    0.51  stalled cycles per insn   
    19,409,416,734      branches                         #  609.455 M/sec                     
     1,016,535,128      branch-misses                    #    5.24% of all branches           

      38.359245639 seconds time elapsed

      16.112863000 seconds user
      16.940505000 seconds sys

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/requests/parameter_wrappers.rs:132

The error message contains a typo ('ii8' should be 'i8').

try_into().map_err(|_| js_error("Value must fit in ii8 type to be small int"))?,

lib/types/cql-utils.js

wprzytula

I have a lot of doubts about the quality of this PR. Please:

use spell checkers (even Copilot could do the job);
pay extra attention when changing existing code, as we don't won't to introduce new bugs;
add comments and docstrings;
write a bird-eye view of the new way of passing values to and from Rust.

lib/types/cql-utils.js

src/requests/parameter_wrappers.rs

src/types/type_wrappers.rs

lib/types/cql-utils.js

src/requests/parameter_wrappers.rs

src/types/type_wrappers.rs

lib/types/cql-utils.js

src/requests/parameter_wrappers.rs

src/types/type_wrappers.rs

docs/src/internal/parameter_wrapper.md

src/requests/parameter_wrappers.rs

src/types/type_wrappers.rs

PiotrJunior · May 13, 2025

lib/types/cql-utils.js

+            types[i],
+            value.get(i) !== undefined ? value.get(i) : null,
+        );
+        newTypes.push(element[0]);


Please explain WDYM by newTypes. Can getWrapped return type other then type given to it as parameter?

If some of the types are not provided (as it's a possible case in type guessing) then the returned type will have this information, while provided argument -- not.

Add in-code documentation for this parameter in every place where it is used.

I'm not sure I understand your comment.

Now I also realized that tuples do not support type guessing.

docs/src/internal/parameter_wrapper.md

src/requests/parameter_wrappers.rs

src/types/type_wrappers.rs

src/requests/parameter_wrappers.rs

docs/src/internal/parameter_wrapper.md

Before, we created QueryParameterWrapper with from_... functions, and then cloning this object when executing query. Now, with implementation of FromNapiValue trait, we pass parameters to queries directly, without any additional conversion steps. In order to correctly handle value types, each parameter is passed as a pair (type, value). Information about type is then used on the rust side to create correct CqlValue object.

PiotrJunior · May 17, 2025

src/types/type_wrappers.rs

+    /// Create a new ComplexType for tuple with provided inner types.
+    #[napi]


This comment should be under #[napi]

i beg to differ

#113 (comment)

PiotrJunior requested a review from Copilot April 15, 2025 10:28

Copilot AI reviewed Apr 15, 2025

View reviewed changes

lib/types/cql-utils.js Outdated Show resolved Hide resolved

adespawn force-pushed the second-iteration branch from 5712c85 to 6f4c9e6 Compare April 15, 2025 11:24

adespawn force-pushed the QueryParameters-FromNapiValue branch 3 times, most recently from 085c3ca to 83b32f5 Compare April 23, 2025 08:08

adespawn changed the title ~~[WIP] - more opts~~ FromNapiValue trait for parameters Apr 23, 2025

adespawn marked this pull request as ready for review April 23, 2025 08:57

adespawn force-pushed the second-iteration branch from f048cb6 to 5e438d8 Compare April 23, 2025 12:12

adespawn force-pushed the QueryParameters-FromNapiValue branch from 83b32f5 to 7335f04 Compare April 23, 2025 12:13

adespawn requested a review from wprzytula April 23, 2025 12:19

wprzytula assigned adespawn Apr 23, 2025

wprzytula requested changes Apr 24, 2025

View reviewed changes

adespawn force-pushed the QueryParameters-FromNapiValue branch from 7335f04 to 0244e25 Compare April 26, 2025 17:57

wprzytula added this to the Iteration 2 milestone Apr 28, 2025

wprzytula added the Performance Driver goes brrrr label Apr 28, 2025

wprzytula requested changes Apr 28, 2025

View reviewed changes

adespawn force-pushed the QueryParameters-FromNapiValue branch 4 times, most recently from 90bc4e7 to 0489f49 Compare April 30, 2025 13:15

ZuzaOsa approved these changes May 4, 2025

View reviewed changes

lib/types/cql-utils.js Outdated Show resolved Hide resolved

src/requests/parameter_wrappers.rs Outdated Show resolved Hide resolved

src/types/type_wrappers.rs Outdated Show resolved Hide resolved

ZuzaOsa requested a review from Copilot May 6, 2025 08:59

This comment was marked as abuse.

Sign in to view

wprzytula requested review from wprzytula and removed request for wprzytula May 6, 2025 10:20

adespawn force-pushed the QueryParameters-FromNapiValue branch 2 times, most recently from 9496daa to dc585c4 Compare May 7, 2025 08:34

adespawn requested review from ZuzaOsa and wprzytula May 7, 2025 08:34

wprzytula requested changes May 7, 2025

View reviewed changes

PiotrJunior reviewed May 13, 2025

View reviewed changes

This was referenced May 13, 2025

Varint #195

Open

Decimal #197

Open

adespawn force-pushed the QueryParameters-FromNapiValue branch from dc585c4 to 2c66fb1 Compare May 13, 2025 20:26

adespawn requested a review from wprzytula May 13, 2025 20:32

ZuzaOsa requested changes May 14, 2025

View reviewed changes

adespawn force-pushed the QueryParameters-FromNapiValue branch from 2c66fb1 to d00d256 Compare May 14, 2025 07:52

adespawn requested a review from ZuzaOsa May 14, 2025 07:53

wprzytula requested a review from Copilot May 14, 2025 16:10

This comment was marked as outdated.

Sign in to view

wprzytula requested changes May 14, 2025

View reviewed changes

src/types/type_wrappers.rs Outdated Show resolved Hide resolved

adespawn force-pushed the QueryParameters-FromNapiValue branch from d00d256 to 1dfdd1c Compare May 14, 2025 19:05

adespawn requested a review from wprzytula May 14, 2025 19:05

wprzytula requested changes May 15, 2025

View reviewed changes

src/types/type_wrappers.rs Outdated Show resolved Hide resolved

src/types/type_wrappers.rs Outdated Show resolved Hide resolved

PiotrJunior reviewed May 15, 2025

View reviewed changes

src/requests/parameter_wrappers.rs Show resolved Hide resolved

ZuzaOsa requested changes May 15, 2025

View reviewed changes

docs/src/internal/parameter_wrapper.md Outdated Show resolved Hide resolved

docs/src/internal/parameter_wrapper.md Outdated Show resolved Hide resolved

adespawn added 3 commits May 16, 2025 10:41

Extend unit tests for parameters wrapper

7f379e5

Add some overview documentation

95900c8

adespawn force-pushed the QueryParameters-FromNapiValue branch from 1dfdd1c to 95900c8 Compare May 16, 2025 08:41

adespawn requested review from wprzytula and ZuzaOsa May 16, 2025 08:42

ZuzaOsa requested a review from Copilot May 16, 2025 08:47

This comment was marked as abuse.

Sign in to view

PiotrJunior requested changes May 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FromNapiValue trait for parameters #183

FromNapiValue trait for parameters #183

adespawn commented Apr 12, 2025 •

edited

Loading

Copilot AI left a comment

wprzytula left a comment

This comment was marked as abuse.

PiotrJunior May 13, 2025

adespawn May 13, 2025

PiotrJunior May 15, 2025

adespawn May 16, 2025

This comment was marked as outdated.

This comment was marked as abuse.

PiotrJunior May 17, 2025

adespawn May 17, 2025

		/// Create a new ComplexType for tuple with provided inner types.
		#[napi]

Search code, repositories, users, issues, pull requests...

FromNapiValue trait for parameters #183

Are you sure you want to change the base?

FromNapiValue trait for parameters #183

Conversation

adespawn commented Apr 12, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

wprzytula left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

PiotrJunior May 13, 2025

Choose a reason for hiding this comment

adespawn May 13, 2025

Choose a reason for hiding this comment

PiotrJunior May 15, 2025

Choose a reason for hiding this comment

adespawn May 16, 2025

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as abuse.

PiotrJunior May 17, 2025

Choose a reason for hiding this comment

adespawn May 17, 2025

Choose a reason for hiding this comment

adespawn commented Apr 12, 2025 •

edited

Loading