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.

Sign up for GitHub

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

Jump to bottom

src,lib: expose memory file mapping flag #29260

Closed
wants to merge 6 commits into from
Closed

src,lib: expose memory file mapping flag #29260

wants to merge 6 commits into from

Conversation

joaocgreis
Copy link
Member

Support for UV_FS_O_FILEMAP was added in libuv in version 1.31.0. This exposes the flag in fs.constants and adds the prefix 'm' for text flags.

New functionality is added, so this is semver minor.

cc @nodejs/platform-windows

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

@joaocgreis
src,lib: expose memory file mapping flag
2002dd1 
Support for UV_FS_O_FILEMAP was added in libuv in version 1.31.0.
This exposes the flag in fs.constants and adds the prefix 'm' for
text flags.
@joaocgreis joaocgreis added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 22, 2019
@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. labels Aug 22, 2019
@Trott
Copy link
Member

Trott commented Aug 22, 2019

@nodejs/fs @nodejs/libuv

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

mscdex
mscdex reviewed Aug 22, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 22, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
bnoordhuis
bnoordhuis approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with suggestions.

Question: is e.g. 'ma' (memory-mapped, append-only) a mode that makes sense?

src/node_constants.cc Outdated Show resolved Hide resolved
test/parallel/test-fs-open-flags.js Outdated Show resolved Hide resolved
@joaocgreis
Copy link
Member Author

Thanks for the reviews! Updated.

Question: is e.g. 'ma' (memory-mapped, append-only) a mode that makes sense?

I tried to keep the libuv API as compatible as possible. This included supporting positional writes and keeping track of the file position. On top of this, supporting append makes sense to me, and it was not that complex:

node/deps/uv/src/win/fs.c

Lines 934 to 935 in 775048d

if (force_append) {
pos = fd_info->size;

Making multiple appends to the same file is much slower than without using a file mapping though, because the mapping has to be re-created to enlarge the file every time.

@bnoordhuis
Copy link
Member

I did some research and it seems mmap() on an O_APPEND file descriptor fails with EACCES on most Unices (which makes sense to me.)

I think we should revisit the libuv behavior and drop the 'ma' flags from this pull request.

@joaocgreis joaocgreis mentioned this pull request Sep 2, 2019
Closed
@joaocgreis
Copy link
Member Author

@bnoordhuis removed the 'm' flags from this PR. Opened libuv/libuv#2448 to discuss the libuv side.

bnoordhuis
bnoordhuis approved these changes Sep 3, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. The 'mr' and 'mw' flags would still have been alright but those can always be added later.

jasnell
jasnell reviewed Sep 3, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
@joaocgreis
Copy link
Member Author

@jasnell updated, PTAL.

@joaocgreis
Copy link
Member Author

@jasnell any objection for landing this?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 11, 2019 •
edited by joaocgreis
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/25432/ ✔️

joaocgreis added a commit that referenced this pull request Sep 11, 2019
@joaocgreis
src,lib: expose memory file mapping flag
902c9fa 
Support for UV_FS_O_FILEMAP was added in libuv in version 1.31.0.
This exposes the flag in fs.constants.

Refs: libuv/libuv#2295
PR-URL: #29260
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joaocgreis
Copy link
Member Author

Landed in 902c9fa

@joaocgreis joaocgreis closed this Sep 11, 2019
@joaocgreis joaocgreis mentioned this pull request Sep 12, 2019
Closed
@joaocgreis joaocgreis mentioned this pull request Sep 12, 2019
Closed
targos pushed a commit that referenced this pull request Sep 20, 2019
@joaocgreis @targos
src,lib: expose memory file mapping flag
465bc66 
Support for UV_FS_O_FILEMAP was added in libuv in version 1.31.0.
This exposes the flag in fs.constants.

Refs: libuv/libuv#2295
PR-URL: #29260
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit that referenced this pull request Sep 24, 2019
@BridgeAR
2019-09-24, Version 12.11.0 (Current)
21e8f36 
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Sep 24, 2019
@joaocgreis
src,lib: expose memory file mapping flag
6f40aa9 
Support for UV_FS_O_FILEMAP was added in libuv in version 1.31.0.
This exposes the flag in fs.constants.

Refs: libuv/libuv#2295
PR-URL: nodejs#29260
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@joaocgreis @BridgeAR
src,lib: expose memory file mapping flag
78af92d 
Support for UV_FS_O_FILEMAP was added in libuv in version 1.31.0.
This exposes the flag in fs.constants.

Refs: libuv/libuv#2295
PR-URL: #29260
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit that referenced this pull request Sep 25, 2019
@BridgeAR
2019-09-24, Version 12.11.0 (Current)
b50c01c 
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
BridgeAR added a commit that referenced this pull request Sep 25, 2019
@BridgeAR
2019-09-24, Version 12.11.0 (Current)
d36b6f8 
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@Trott Trott Trott left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@ronkorving ronkorving ronkorving approved these changes

Assignees
No one assigned
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joaocgreis @Trott @nodejs-github-bot @bnoordhuis @mscdex @jasnell @ronkorving