Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Encapsulating js type checking and conversion #3035

Closed
wants to merge 1 commit into from

Conversation

ssuda
Copy link

@ssuda ssuda commented Mar 30, 2012

First thing we will do in c++ land is js type checking and converting them to c++ types, we can encapsulate it into one function. ScanArgs is such function, we will do the type checking and conversions to c++ types. node_file.cc is modified to demonstrate ScanArgs. ScanArgs is modeled after rb_scan_args
but with type safety

First thing we will do in c++ land is js type checking and
converting them to c++ types,  we can encapsulate it into
one function. ScanArgs is such function, we will do the type
checking and conversions to c++ types
@bnoordhuis
Copy link
Member

I wished you would've discussed this on nodejs-dev or in the IRC channel first. You've obviously put a lot of effort in this patch and I like the idea but I don't want to take it without further measurements.

Your approach to argument scanning is clever but it's not particularly cheap. Your patch increases the executable size by about 4K and that's while only touching node_file.cc. What is worse is that it has a lot of function call overhead (most generated ScanArgs instances are fairly tight though). For example, this is what the prolog in node::Symlink() looks like:

   0x00000000005924c8 <+56>:    xor    %esi,%esi
   0x00000000005924ca <+58>:    mov    $0xe23f18,%ecx
   0x00000000005924cf <+63>:    mov    $0xa7cb82,%edx
   0x00000000005924d4 <+68>:    mov    %rbx,%rdi
   0x00000000005924d7 <+71>:    callq  0x58de70 <_ZN4node8ScanArgsIPvEEN2v86HandleINS2_5ValueEEERKNS2_9ArgumentsEiPKcRT_>
   0x00000000005924dc <+76>:    mov    %rax,%rdi
   0x00000000005924df <+79>:    mov    %rax,%r12
   0x00000000005924e2 <+82>:    callq  0x6be030 <_ZNK2v85Value6IsNullEv>
   0x00000000005924e7 <+87>:    test   %al,%al
   0x00000000005924e9 <+89>:    je     0x592505 <_ZN4nodeL7SymlinkERKN2v89ArgumentsE+117>
   0x00000000005924eb <+91>:    mov    $0xe23f18,%ecx
   0x00000000005924f0 <+96>:    mov    $0xa7cb83,%edx
   0x00000000005924f5 <+101>:   mov    $0x1,%esi
   0x00000000005924fa <+106>:   mov    %rbx,%rdi
   0x00000000005924fd <+109>:   callq  0x58de70 <_ZN4node8ScanArgsIPvEEN2v86HandleINS2_5ValueEEERKNS2_9ArgumentsEiPKcRT_>
   0x0000000000592502 <+114>:   mov    %rax,%r12
   0x0000000000592505 <+117>:   mov    %r12,%rdi
   0x0000000000592508 <+120>:   callq  0x6be030 <_ZNK2v85Value6IsNullEv>
   0x000000000059250d <+125>:   test   %al,%al
   0x000000000059250f <+127>:   jne    0x592558 <_ZN4nodeL7SymlinkERKN2v89ArgumentsE+200>

In node_file.cc that might be acceptable because the overhead of thread pool operations and file I/O dwarfs everything else but I don't want to take chances in files like node_http_parser.cc until benchmarks conclusively show that there is no performance impact.

Then again, I may be a micro-optimizing bastard. Other committers, what do you think?

@isaacs
Copy link

isaacs commented Apr 1, 2012

-1. Seems like it adds more complexity than it removes. Too clever, sorry.

@isaacs isaacs closed this Apr 1, 2012
richardlau pushed a commit to ibmruntimes/node that referenced this pull request Oct 6, 2015
Revert 0af4c9e, parts of
921f2de and port
nodejs#25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

Fixes nodejs#3035.

PR: nodejs#3036
PR-URL: nodejs/node#3036
Reviewed-By: Ben Noordhuis <[email protected]>
richardlau pushed a commit to ibmruntimes/node that referenced this pull request Oct 8, 2015
Revert 0af4c9e, parts of
921f2de and port
nodejs#25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

Fixes nodejs#3035.

PR: nodejs#3036
PR-URL: nodejs/node#3036
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants