src: add sync trace to fs #19649

chinhuang007 · 2018-03-28T05:19:46Z

Add sync trace to fs operations which
is enabled by the node.fs.sync trace event
category. Also add a general test js file
to verify all operations.

Checklist

[x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
[x ] tests and/or benchmarks are included
[x ] commit message follows commit guidelines

Trott · 2018-03-28T05:39:01Z

@nodejs/fs

joyeecheung · 2018-03-28T05:46:06Z

src/node_file.cc

@@ -776,7 +815,9 @@ void Access(const FunctionCallbackInfo<Value>& args) {
  } else {  // access(path, mode, undefined, ctx)
    CHECK_EQ(argc, 4);
    fs_req_wrap req_wrap;
+    FS_SYNC_TRACE_BEGIN(SYNC_TRACE_ACCESS);
    SyncCall(env, args[3], &req_wrap, "access", uv_fs_access, *path, mode);


Maybe pass this as a parameter to SyncCall and call FS_SYNC_TRACE_BEGIN and FS_SYNC_TRACE_END from there?

Thanks very much for the comment. I also considered adding trace begin and end into SyncCall. These trace functions are flexible to take additional useful tracing data whenever making sense such as number of bytes read for read(). Since SyncCall is already a variadic function, it will be a little messy to introduce arbitrary trace arguments there. It seems cleaner to leave SyncCall API unchanged and call trace begin and end variadic functions separately.

@chinhuang007 Good point, I missed the call below that passes additional arguments to the macros, sorry about that.

joyeecheung · 2018-03-29T03:16:38Z

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

chinhuang007 · 2018-03-30T17:17:45Z

I fixed the conflicts in src/node_file.cc. Can someone start a new CI please?

joyeecheung · 2018-03-30T17:35:51Z

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

yhwang · 2018-04-02T16:29:08Z

New CI: https://ci.nodejs.org/job/node-test-commit/17360/

yhwang · 2018-04-02T21:50:28Z

Another new CI to help the problem investigation: https://ci.nodejs.org/job/node-test-commit/17366/

chinhuang007 · 2018-04-03T00:50:56Z

Thanks. Most CI issues were fixed by avoid using relative paths. Also found and fixed the AIX issues. Please start a new CI.

yhwang · 2018-04-03T01:08:34Z

New CI: https://ci.nodejs.org/job/node-test-commit/17371/

jasnell · 2018-04-03T14:55:54Z

src/node_file.cc

+static const char* SYNC_TRACE_SYMLINK = "fs.symlink";
+static const char* SYNC_TRACE_UNLINK = "fs.unlink";
+static const char* SYNC_TRACE_UTIME = "fs.utimes";
+static const char* SYNC_TRACE_WRITE = "fs.write";


I'd prefer to not have to define these as static strings this way and just build it in to the macro such that, for instance, FS_SYNC_TRACE_BEGIN(access) would expand out to fs.access

chinhuang007 · 2018-04-03T23:03:30Z

I merged with upstream, fixed the lint issue, and changed to use macros instead of static strings as @jasnell commented. Can someone start a CI?

joyeecheung · 2018-04-04T05:14:47Z

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

jasnell · 2018-04-04T19:08:23Z

test/parallel/test-sync-trace-fs.js

@@ -0,0 +1,161 @@
+'use strict';


To group this better with other trace event test cases, please use a file name like test-trace-events-fs-sync.js

chinhuang007 · 2018-04-09T17:33:57Z

@joyeecheung Thanks for the heads-up. I just rebased my master and this branch and passed local tests. Can someone start an CI to see if everything is good?

yhwang · 2018-04-09T19:04:47Z

CI: https://ci.nodejs.org/job/node-test-commit/17570/

chinhuang007 · 2018-04-09T23:10:37Z

CI passed all tests except on linux-linked-debug, failing on test-http-readable-data-event, and on arm-fanned, failing on Waiting for next available executor on pi1-docker. Both seem unrelated to this PR.

mhdawson

LGTM

mhdawson · 2018-04-12T21:53:29Z

New CI since we are trying to have green CI before landing: https://ci.nodejs.org/job/node-test-pull-request/14233/

jasnell · 2018-04-14T19:44:47Z

CI failures appear unrelated.

Add sync trace to fs operations which is enabled by the node.fs.sync trace event category. Also add a general test js file to verify all operations. PR-URL: #19649 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell · 2018-04-14T19:57:42Z

Landed in 09c6346

Add sync trace to fs operations which is enabled by the node.fs.sync trace event category. Also add a general test js file to verify all operations. PR-URL: #19649 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Add the trace category for file system synchronous methods to documentation so the users can enable it when they want to look into file system sync method trace data. Refs: nodejs#19649

Add the trace category for file system synchronous methods to documentation so the users can enable it when they want to look into file system sync method trace data. PR-URL: #20526 Refs: #19649 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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. labels Mar 28, 2018

joyeecheung reviewed Mar 28, 2018

View reviewed changes

chinhuang007 force-pushed the add_fs_sync_trace branch from 9ab8e6a to f77f35e Compare March 28, 2018 17:50

chinhuang007 force-pushed the add_fs_sync_trace branch from f77f35e to 79d3738 Compare March 30, 2018 17:13

chinhuang007 force-pushed the add_fs_sync_trace branch from 79d3738 to a8fd0fc Compare March 30, 2018 22:22

chinhuang007 force-pushed the add_fs_sync_trace branch 2 times, most recently from 08bccce to 812a9c7 Compare April 2, 2018 20:29

chinhuang007 force-pushed the add_fs_sync_trace branch 4 times, most recently from a963af0 to 4f5d1fe Compare April 3, 2018 00:43

chinhuang007 force-pushed the add_fs_sync_trace branch 2 times, most recently from 029097a to 1bd5f51 Compare April 3, 2018 01:18

jasnell reviewed Apr 3, 2018

View reviewed changes

chinhuang007 force-pushed the add_fs_sync_trace branch 4 times, most recently from 8a8c31d to b266ed0 Compare April 3, 2018 22:39

jasnell reviewed Apr 4, 2018

View reviewed changes

jasnell approved these changes Apr 9, 2018

View reviewed changes

joyeecheung approved these changes Apr 10, 2018

View reviewed changes

chinhuang007 mentioned this pull request Apr 12, 2018

[trace_event] adding trace points to core nodejs/diagnostics#82

Closed

2 tasks

mhdawson approved these changes Apr 12, 2018

View reviewed changes

jasnell closed this Apr 14, 2018

chinhuang007 deleted the add_fs_sync_trace branch April 24, 2018 18:56

chinhuang007 mentioned this pull request May 4, 2018

doc: add trace category for fs sync methods #20526

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add sync trace to fs #19649

src: add sync trace to fs #19649

chinhuang007 commented Mar 28, 2018

Trott commented Mar 28, 2018

joyeecheung Mar 28, 2018

chinhuang007 Mar 28, 2018

joyeecheung Mar 29, 2018

joyeecheung commented Mar 29, 2018

chinhuang007 commented Mar 30, 2018

joyeecheung commented Mar 30, 2018 •

edited

Loading

yhwang commented Apr 2, 2018

yhwang commented Apr 2, 2018

chinhuang007 commented Apr 3, 2018

yhwang commented Apr 3, 2018

jasnell Apr 3, 2018

chinhuang007 commented Apr 3, 2018

joyeecheung commented Apr 4, 2018

jasnell Apr 4, 2018

chinhuang007 commented Apr 9, 2018

yhwang commented Apr 9, 2018

chinhuang007 commented Apr 9, 2018

mhdawson left a comment

mhdawson commented Apr 12, 2018

jasnell commented Apr 14, 2018

jasnell commented Apr 14, 2018

src: add sync trace to fs #19649

src: add sync trace to fs #19649

Conversation

chinhuang007 commented Mar 28, 2018

Checklist

Trott commented Mar 28, 2018

joyeecheung Mar 28, 2018

Choose a reason for hiding this comment

chinhuang007 Mar 28, 2018

Choose a reason for hiding this comment

joyeecheung Mar 29, 2018

Choose a reason for hiding this comment

joyeecheung commented Mar 29, 2018

chinhuang007 commented Mar 30, 2018

joyeecheung commented Mar 30, 2018 • edited Loading

yhwang commented Apr 2, 2018

yhwang commented Apr 2, 2018

chinhuang007 commented Apr 3, 2018

yhwang commented Apr 3, 2018

jasnell Apr 3, 2018

Choose a reason for hiding this comment

chinhuang007 commented Apr 3, 2018

joyeecheung commented Apr 4, 2018

jasnell Apr 4, 2018

Choose a reason for hiding this comment

chinhuang007 commented Apr 9, 2018

yhwang commented Apr 9, 2018

chinhuang007 commented Apr 9, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 12, 2018

jasnell commented Apr 14, 2018

jasnell commented Apr 14, 2018

joyeecheung commented Mar 30, 2018 •

edited

Loading