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: add *timeNs properties to BigInt Stats objects #21387

Closed
wants to merge 1 commit into from

Conversation

@joyeecheung
Copy link
Member

commented Jun 18, 2018

  • Extend the aliased buffer for stats objects to contain
    the entire time spec (seconds and nanoseconds) for the time
    values instead of calculating the milliseconds in C++ and
    lose precision.
  • Calculate the nanosecond-precision time values in JS and expose
    them in BigInt Stats objects as *timeNs. The
    millisecond-precision values are now calculated from the
    nanosecond-precision values.
  • Refactor the hierarchy of stats objects and use ES6 classes.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@joyeecheung

This comment has been minimized.

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@Fishrock123
Copy link
Member

left a comment

I like it. One nit.

nsFromTimeSpecBigInt(stats[14 + offset], stats[15 + offset]),
nsFromTimeSpecBigInt(stats[16 + offset], stats[17 + offset])
);
}

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Jun 20, 2018

Member

else would a touch clearer

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jun 27, 2018

Author Member

@Fishrock123 I think we prefer leaving out the else block for early returns? There used to a PR adding a lint rule for that although it didn't land due to the amount of churn.

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

While I'm good with this, changing fs.Stats to a ES6 class is necessarily a semver-major change because fs.Stats() is no longer callable without new.

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

Just a thought... we may want to consider adding a custom toJSON() function to the BitIntStats object to avoid...

> JSON.stringify(fs.statSync('.', { bigint: true }))
TypeError: Do not know how to serialize a BigInt
    at JSON.stringify (<anonymous>)
@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2018

JSON.stringify(fs.statSync('.', { bigint: true }))
TypeError: Do not know how to serialize a BigInt
at JSON.stringify ()

@jasnell hm, thanks for raising that up, but we need to be careful about the format we pick since people may store them into DB or send them to clients that are not implemented in JavaScript...

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

On a second thought, the ES6 classes probably does not worth making this semver major. I have changed it back to the ES5 way of doing things. @jasnell

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

@joyeecheung joyeecheung force-pushed the joyeecheung:bigint-ns branch from 7c8170c to a11d4c7 Jun 27, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

Ping @jasnell does this look good to you now as a non-semver-major?

@jasnell

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

Yes, looks good

@addaleax

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

Previous CI is inaccessible, new CI: https://ci.nodejs.org/job/node-test-pull-request/15855/

@Trott

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

Looks like it broke something related to #13255 on Windows, I'll investigate.

Failures in job https://ci.nodejs.org/job/node-test-pull-request/15920/

debian8-x86

See failures on test-softlayer-debian8-x86-1:
not ok 658 parallel/test-fs-utimes
  ---
  duration_ms: 0.331
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-x86/test/parallel/test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

ubuntu1404-32

See failures on test-digitalocean-ubuntu1404-x86-1:
not ok 601 parallel/test-fs-utimes
  ---
  duration_ms: 0.629
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

ubuntu1604-32

See failures on test-digitalocean-ubuntu1604-x86-2:
not ok 658 parallel/test-fs-utimes
  ---
  duration_ms: 0.171
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1604-32/test/parallel/test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=3

See failures on test-azure_msft-win10-x64-3:
not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.121
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=3

See failures on test-rackspace-win2008r2-x64-3:
not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.198
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=3

See failures on test-azure_msft-win2016-x64-3:
not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.120
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017-x86,RUNNER=win2012r2,RUN_SUBSET=3

See failures on test-rackspace-win2012r2-x64-1:
not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.203
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

@BridgeAR BridgeAR removed the author ready label Jul 18, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2018

Won't be able to be around a machine to look into this this week

@jasnell

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

What's the status on this one?

@jasnell jasnell added the stalled label Sep 10, 2018

@josepharhar

This comment has been minimized.

Copy link

commented Jan 21, 2019

Can we try fixing merge conflicts and running the tests again? I'd be happy to give it a shot in another pull request

@joyeecheung joyeecheung force-pushed the joyeecheung:bigint-ns branch from a11d4c7 to 0410dd2 Jun 10, 2019

fs: add *timeNs properties to BigInt Stats objects
- Extend the aliased buffer for stats objects to contain
  the entire time spec (seconds and nanoseconds) for the time
  values instead of calculating the milliseconds in C++ and
  lose precision there.
- Calculate the nanosecond-precision time values in JS and expose
  them in BigInt Stats objects as `*timeNs`. The
  millisecond-precision values are now calculated from the
  nanosecond-precision values.

@joyeecheung joyeecheung force-pushed the joyeecheung:bigint-ns branch from aa034a6 to 06dff68 Jun 10, 2019

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

@joyeecheung joyeecheung removed the stalled label Jun 11, 2019

stats[3 + offset], stats[4 + offset], stats[5 + offset],
stats[6 + offset], stats[7 + offset], stats[8 + offset],
stats[9 + offset],
msFromTimeSpec(stats[10 + offset], stats[11 + offset]),

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jun 11, 2019

Author Member

We can't pass a time spec into the Stats constructor as that would be a breaking change since Stats is public. We may try using some factory function to construct Stats internally from time specs later, and try adding the Ns properties to normal Stats later, though, but there will be some precision losses without bigint and that's not too different from multiplying the Ms ones with 1000

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

The CI is now green (minus an unrelated failure #27611 )

@jasnell @Fishrock123 can you please take a look again?

Also @devsnek this might be useful for #27850 , whichever lands first..

@@ -726,6 +731,54 @@ added: v8.1.0
The timestamp indicating the creation time of this file expressed in
milliseconds since the POSIX Epoch.

### stats.atimeNs
<!-- YAML
added: REPLACEME

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Jun 12, 2019

This should be changed, right?

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jun 12, 2019

Author Member

That'll only be available when this is released.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Landed in b245257

joyeecheung added a commit that referenced this pull request Jun 13, 2019
fs: add *timeNs properties to BigInt Stats objects
- Extend the aliased buffer for stats objects to contain
  the entire time spec (seconds and nanoseconds) for the time
  values instead of calculating the milliseconds in C++ and
  lose precision there.
- Calculate the nanosecond-precision time values in JS and expose
  them in BigInt Stats objects as `*timeNs`. The
  millisecond-precision values are now calculated from the
  nanosecond-precision values.

PR-URL: #21387
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
fs: add *timeNs properties to BigInt Stats objects
- Extend the aliased buffer for stats objects to contain
  the entire time spec (seconds and nanoseconds) for the time
  values instead of calculating the milliseconds in C++ and
  lose precision there.
- Calculate the nanosecond-precision time values in JS and expose
  them in BigInt Stats objects as `*timeNs`. The
  millisecond-precision values are now calculated from the
  nanosecond-precision values.

PR-URL: #21387
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR BridgeAR referenced this pull request Jun 17, 2019
@targos targos referenced this pull request Aug 19, 2019
2 of 2 tasks complete
BridgeAR added a commit that referenced this pull request Sep 3, 2019
fs: add *timeNs properties to BigInt Stats objects
- Extend the aliased buffer for stats objects to contain
  the entire time spec (seconds and nanoseconds) for the time
  values instead of calculating the milliseconds in C++ and
  lose precision there.
- Calculate the nanosecond-precision time values in JS and expose
  them in BigInt Stats objects as `*timeNs`. The
  millisecond-precision values are now calculated from the
  nanosecond-precision values.

PR-URL: #21387
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR BridgeAR referenced this pull request Sep 3, 2019
BridgeAR added a commit that referenced this pull request Sep 3, 2019
2019-09-04, Version 12.10.0 (Current)
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
BridgeAR added a commit that referenced this pull request Sep 4, 2019
2019-09-04, Version 12.10.0 (Current)
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
BridgeAR added a commit that referenced this pull request Sep 4, 2019
2019-09-04, Version 12.10.0 (Current)
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
2019-09-04, Version 12.10.0 (Current)
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    nodejs#29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    nodejs#29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    nodejs#29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    nodejs#21387
* net:
  * Allow reading data into a static buffer (Brian White)
    nodejs#25436

PR-URL: nodejs#29429
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
2019-09-04, Version 12.10.0 (Current)
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    nodejs#29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    nodejs#29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    nodejs#29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    nodejs#21387
* net:
  * Allow reading data into a static buffer (Brian White)
    nodejs#25436

PR-URL: nodejs#29429
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.