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

fs(readfile): merge open and fstat #45314

Closed
wants to merge 5 commits into from
Closed

Conversation

tony-go
Copy link
Member

@tony-go tony-go commented Nov 4, 2022

Context

@anonrig shared this discussion with me . I spent time during the last weeks around the readFilecode.

Updates

Merge open and fstat into a single c++ function.

Gains

It's where I'm stuck, and it's probably linked to my implementation (🙈). I failed to run the benchmark properly.

When I try to execute the targeted benchmark file: benchmark/fs/readfile.fs

  • With my build
./node benchmark/fs/readfile.js
fs/readfile.js concurrent=1 len=16777216 encoding="" duration=5: 367.1841192868408
fs/readfile.js concurrent=1 len=16777216 encoding="utf-8" duration=5: 195.96019558527175
  • With the main branch build
./node-main benchmark/fs/readfile.js
fs/readfile.js concurrent=1 len=1024 encoding="" duration=5: 34,928.87992225262
fs/readfile.js concurrent=10 len=1024 encoding="" duration=5: 98,718.82706159452
fs/readfile.js concurrent=1 len=16777216 encoding="" duration=5: 385.6378889225866
fs/readfile.js concurrent=10 len=16777216 encoding="" duration=5: 881.9940685898887
fs/readfile.js concurrent=1 len=1024 encoding="utf-8" duration=5: 35,145.19621474622
fs/readfile.js concurrent=10 len=1024 encoding="utf-8" duration=5: 99,417.17644259878
fs/readfile.js concurrent=1 len=16777216 encoding="utf-8" duration=5: 199.9139287372592
fs/readfile.js concurrent=10 len=16777216 encoding="utf-8" duration=5: 295.9984238083932

It looks like my implementation failed somewhere...

Note: it's my first c++ code

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Nov 4, 2022
@tony-go
Copy link
Member Author

tony-go commented Nov 4, 2022

My timeline is:

  1. make compliant aiming to run the benchmark
  2. fix edge cases (tests)

@tony-go
Copy link
Member Author

tony-go commented Nov 5, 2022

List of failing tests:

Test file Status Error
test/parallel/test-fs-error-messages.js  ✅ Assertion failed (error message)
 test/parallel/test-fs-options-immutable.js 🔴 bus error/segfault
test/parallel/test-domain-promise.js  🔴 xxxxx
test/parallel/test-fs-readfile.js 🔄 bus error/segfault
test/parallel/test-npm-install.js 🔴 xxxxxxx

Error message for test/parallel/test-fs-readfile.js:

➜  node git:(read-file-refac) ✗ lldb ./node test/parallel/test-fs-readfile.js
(lldb) target create "./node"
Current executable set to '/Users/tonygorez/projects/node/node' (arm64).
(lldb) settings set -- target.run-args  "test/parallel/test-fs-readfile.js"
(lldb) run
Process 30077 launched: '/Users/tonygorez/projects/node/node' (arm64)
node was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 30077 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x100080001120)
    frame #0: 0x00000001000c1eec node`node::fs::FSReqAfterScope::FSReqAfterScope(node::fs::FSReqBase*, uv_fs_s*) [inlined] node::Environment::isolate(this=0x0000100080001000) const at env-inl.h:243:10 [opt]
   240  }
   241
   242  inline v8::Isolate* Environment::isolate() const {
-> 243    return isolate_;
   244  }
   245
   246  inline Environment* Environment::from_timer_handle(uv_timer_t* handle) {
Target 0: (node) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x100080001120)
  * frame #0: 0x00000001000c1eec node`node::fs::FSReqAfterScope::FSReqAfterScope(node::fs::FSReqBase*, uv_fs_s*) [inlined] node::Environment::isolate(this=0x0000100080001000) const at env-inl.h:243:10 [opt]
    frame #1: 0x00000001000c1eec node`node::fs::FSReqAfterScope::FSReqAfterScope(this=0x000000016fdf6a10, wrap=0x0000000105704080, req=0x00000001057040d8) at node_file.cc:714:34 [opt]
    frame #2: 0x00000001000c23c0 node`node::fs::AfterStat(uv_fs_s*) [inlined] node::fs::FSReqAfterScope::FSReqAfterScope(this=<unavailable>, wrap=0x0000000105704080, req=0x00000001057040d8) at node_file.cc:715:46 [opt]
    frame #3: 0x00000001000c23b0 node`node::fs::AfterStat(req=0x00000001057040d8) at node_file.cc:775:19 [opt]
    frame #4: 0x0000000100a54620 node`uv__work_done(handle=<unavailable>) at threadpool.c:318:5 [opt]
    frame #5: 0x0000000100a57dd0 node`uv__async_io(loop=0x0000000103c89b70, w=<unavailable>, events=<unavailable>) at async.c:163:5 [opt]
    frame #6: 0x0000000100a69970 node`uv__io_poll(loop=0x0000000103c89b70, timeout=0) at kqueue.c:0 [opt]
    frame #7: 0x0000000100a58260 node`uv_run(loop=0x0000000103c89b70, mode=UV_RUN_DEFAULT) at core.c:389:5 [opt]
    frame #8: 0x00000001000056f0 node`node::SpinEventLoopInternal(env=0x0000000106016000) at embed_helpers.cc:38:7 [opt]
    frame #9: 0x00000001000f9950 node`node::NodeMainInstance::Run() [inlined] node::NodeMainInstance::Run(this=<unavailable>, exit_code=<unavailable>, env=0x0000000106016000) at node_main_instance.cc:141:9 [opt]
    frame #10: 0x00000001000f9904 node`node::NodeMainInstance::Run(this=<unavailable>) at node_main_instance.cc:132:3 [opt]
    frame #11: 0x000000010008a1a8 node`node::LoadSnapshotDataAndRun(snapshot_data_ptr=<unavailable>, result=0x0000600002909f10) at node.cc:1190:29 [opt]
    frame #12: 0x000000010008a428 node`node::Start(int, char**) [inlined] node::StartInternal(argc=<unavailable>, argv=<unavailable>) at node.cc:1236:10 [opt]
    frame #13: 0x000000010008a214 node`node::Start(argc=<unavailable>, argv=<unavailable>) at node.cc:1240:27 [opt]
    frame #14: 0x000000010525908c dyld`start + 520

@tony-go
Copy link
Member Author

tony-go commented Nov 8, 2022

So it seems that the isolate is available in the open callback (OpenFileAfterOpen) but not in the AfterStat one.

src/node_file.cc Outdated Show resolved Hide resolved
src/node_file.cc Outdated
@@ -2619,6 +2680,7 @@ void Initialize(Local<Object> target,
SetMethod(context, target, "access", Access);
SetMethod(context, target, "close", Close);
SetMethod(context, target, "open", Open);
SetMethod(context, target, "openFile", OpenFile);
Copy link
Member

Choose a reason for hiding this comment

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

ditto here – it’s virtually impossible to tell what the difference between open and openFile is without looking at the implementations

Copy link
Member

Choose a reason for hiding this comment

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

(unresolving this conversation since only the C++ name has been changed)

}

char* fd = static_cast<char *>(open_req->data);
fd[0] = result;
Copy link
Member

Choose a reason for hiding this comment

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

char is not a viable type for storing a file descriptor since it has a much smaller range

open_req->fs_type, req_wrap, "result", result)

if (result < 0) {
FSReqAfterScope after(req_wrap, open_req);
Copy link
Member

Choose a reason for hiding this comment

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

This calls uv_fs_req_cleanup() here. Where is that function called if result >= 0, i.e. when this conditional is not hit?

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably misunderstood something, but as we pass req_wrap->req() to the fstat function, I thought that we should use the FSReqAfterScope only if we want to exit:

  • In this case (when there is a problem with open
  • In the fstat callback AfterStat

Am I correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, FSReqAfterScope would destroy the req_wrap object; however, calling uv_fs_req_cleanup() is also an important part of what the function does, and if you don’t call it through FSReqAfterScope, you should call it directly after the libuv call.

src/node_file.cc Outdated Show resolved Hide resolved
req_wrap->req()->data = fd_buffer;

FS_ASYNC_TRACE_BEGIN0(UV_FS_OPEN, req_wrap)
uv_fs_open(env->event_loop(),
Copy link
Member

Choose a reason for hiding this comment

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

These should be using AsyncCall, otherwise be calling req_wrap->Dispatch(), not the libuv function directly (this is probably the cause of the crash you were seeing

Copy link
Member Author

Choose a reason for hiding this comment

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

I already tried AsyncCall, but as he needs args as an argument, it was not convenient to use it in the open callback.

But I'll try it again and yeah, maybe using req_wrap->Dispatch() and req_wrap->Init() is probably a good alternative ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I also remember that I've tried (in another branch) to change AsyncCall to chain multiple fs calls. But as AsyncCall was widely used, I thought using something standalone could reduce the impact on other fs APIs. At least to run a benchmark and see if it is worth it.

@tony-go
Copy link
Member Author

tony-go commented Nov 9, 2022

Thank you so much for your review @addaleax 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants