GeezerVison

Review requested:

• @nodejs/gyp
• @nodejs/security-wg
• @nodejs/startup
• @nodejs/url
• @nodejs/web-standards

Ada now enables exceptions just like UV and V8

Can you elaborate? libuv is a C library so I don't think exceptions exist there, and I'm pretty sure V8 is built with exceptions disabled.

Ada now enables exceptions just like UV and V8

Can you elaborate? libuv is a C library so I don't think exceptions exist there, and I'm pretty sure V8 is built with exceptions disabled.

My bad UV does not enable exceptions. Referencing v8.gyp file:

{
  'target_name': 'torque_base',
  'type': 'static_library',
  'toolsets': ['host', 'target'],
  'sources': [
    '<!@pymod_do_main(GN-scraper "<(V8_ROOT)/BUILD.gn"  "\\"torque_base.*?sources = ")',
  ],
  'dependencies': [
    'v8_shared_internal_headers',
    'v8_libbase',
  ],
  'defines!': [
    '_HAS_EXCEPTIONS=0',
    'BUILDING_V8_SHARED=1',
  ],
  'cflags_cc!': ['-fno-exceptions'],
  'cflags_cc': ['-fexceptions'],
  'xcode_settings': {
    'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',  # -fexceptions
  },
  'msvs_settings': {
    'VCCLCompilerTool': {
      'RuntimeTypeInfo': 'true',
      'ExceptionHandling': 1,
    },
  },
}

This is not really V8. It's a build-time executable (torque) used to generate code for V8

Can you also include a fairly simple benchmark?

Codecov Report

Attention: Patch coverage is 78.64769% with 120 lines in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (f2001e3) to head (49d1fd7).
Report is 583 commits behind head on main.

@@            Coverage Diff             @@
##             main   #56452      +/-   ##
==========================================
- Coverage   89.21%   89.18%   -0.03%     
==========================================
  Files         663      665       +2     
  Lines      192001   192574     +573     
  Branches    36921    37046     +125     
==========================================
+ Hits       171286   171749     +463     
- Misses      13582    13635      +53     
- Partials     7133     7190      +57     

@jasnell I’ll try to build this and get a benchmark going against the ecosystem routers.

@jasnell I’ll try to build this and get a benchmark going against the ecosystem routers.

Right now, this pull-request does not pass WPT, and not at all optimized. Any benchmarks will not be beneficial.

@nodejs/cpp-reviewers @nodejs/TSC appreciate it if you could review

@nodejs/tsc @nodejs/platform-smartos ... I can not justify the reasoning for waiting for smartOS build to succeed for more than 5.5 hours (https://ci.nodejs.org/job/node-test-commit-smartos/58956/nodes=smartos22-x64/) but here we are... The latest discussion didn't involve cases like this, we only had a discussion about flaky tests.

Please ask yourselves, if you were a new contributor, would you still continue contributing to Node.js after waiting for 5 hours for CI to succeed?

@anonrig As you know, this is currently being discussed in nodejs/build#4011 (which you opened), and there are proposals there to correct the issue. I'd like to keep the main discussion in that issue so that the conversation is all in one place.

Thanks!

@anonrig As you know, this is currently being discussed in nodejs/build#4011 (which you opened), and there are proposals there to correct the issue. I'd like to keep the main discussion in that issue so that the conversation is all in one place.

Thanks!

@bahamat That discussion doesn't solve the frustration of this PR right now. Any suggestions on how to land this pull-request now?

CI: https://ci.nodejs.org/job/node-test-pull-request/64891/

CI: https://ci.nodejs.org/job/node-test-pull-request/64892/

Landed in 44d2648...b78df6c