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

src,lib: expose memory file mapping flag #29260

Closed

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

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.

@nodejs-github-bot
Copy link
Collaborator

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
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
Copy link
Member Author

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

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.

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

joaocgreis added a commit that referenced this pull request Sep 11, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joaocgreis
Copy link
Member Author

Landed in 902c9fa

targos pushed a commit that referenced this pull request Sep 20, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 24, 2019
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
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Sep 24, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
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 <info@bnoordhuis.nl>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Sep 25, 2019
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
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
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants