Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include NAPI module path when calling an unsupported libuv function #18766

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

zackradisic
Copy link
Member

What does this PR do?

This PR includes the path of the NAPI module when the NAPI module tries to call an unsupported libuv function inside a native function.

This is done by storing the function pointer to the native function inside NapiClass_ConstructorFunction(...) before the native function is called, then later the function pointer is passed to dladdr(...) to get the filename of the NAPI module

@zackradisic zackradisic requested a review from 190n April 4, 2025 00:25
@robobun
Copy link

robobun commented Apr 4, 2025

Updated 1:01 PM PT - Apr 8th, 2025

@zackradisic, your commit 315732ed45e310b611396945018fa73a954fca2e passed in Build #14658! 🎉


🧪   try this PR locally:

bunx bun-pr 18766

@@ -824,6 +824,8 @@ if(WIN32)
endif()

target_compile_definitions(${bun} PRIVATE
# This is needed for dladdr in dlfcn.h, all libc's add it if you do #define _GNU_SOURCE
_GNU_SOURCE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cirospaciari didn't this cause issues like a year ago?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if we could use existing crash handler logic instead of dladdr. Don't we know which library an address was in in the stack traces?

Copy link
Collaborator

@190n 190n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good thing to do, but we have to be careful about adding code which runs every time a Node-API function is called. Currently this PR increases the overhead of these calls by about 25% (bench-create-double.js is a script that calls a trivial Node-API function 100 million times):

Benchmark 1: bun bench-create-double.js
  Time (mean ± σ):      1.515 s ±  0.011 s    [User: 1.496 s, System: 0.041 s]
  Range (min … max):    1.502 s …  1.532 s    10 runs
 
Benchmark 2: bun-18766 bench-create-double.js
  Time (mean ± σ):      1.901 s ±  0.049 s    [User: 1.875 s, System: 0.044 s]
  Range (min … max):    1.843 s …  1.993 s    10 runs
 
Summary
  bun bench-create-double.js ran
    1.25 ± 0.03 times faster than bun-18766 bench-create-double.js

But I don't think this is insurmountable. I have some ideas already to reduce the overhead; see comments on crash_handler.zig.

@@ -824,6 +824,8 @@ if(WIN32)
endif()

target_compile_definitions(${bun} PRIVATE
# This is needed for dladdr in dlfcn.h, all libc's add it if you do #define _GNU_SOURCE
_GNU_SOURCE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if we could use existing crash handler logic instead of dladdr. Don't we know which library an address was in in the stack traces?

// We use libuv on Windows
describe.if(!isWindows)("uv stubs", () => {
// These tests are super slow on debug builds
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? I feel like someone would only run this test in a debug build if they're interested in this test specifically, and in that case they don't care that it's slow, they still want to see the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants