FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src,lib: expose memory file mapping flag by joaocgreis · Pull Request #29260 · nodejs/node · GitHub

/ node Public

src,lib: expose memory file mapping flag - #29260

Closed
joaocgreis wants to merge 6 commits into
nodejs:masterfrom
JaneaSystems:joaocgreis-J8L-filemapping
ConversationCommits (6)ChecksFiles changed
Closed

src,lib: expose memory file mapping flag#29260
joaocgreis wants to merge 6 commits into
nodejs:masterfrom
JaneaSystems:joaocgreis-J8L-filemapping

Conversation

joaocgreis commented Aug 22, 2019

Copy link
Copy Markdown
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

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 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 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 commented Aug 22, 2019

Copy link
Copy Markdown
Member

@nodejs/fs @nodejs/libuv

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Aug 22, 2019

Copy link
Copy Markdown
Collaborator

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

mscdex reviewed Aug 22, 2019
View reviewed changes
Comment thread doc/api/fs.md Outdated
Trott reviewed Aug 22, 2019
View reviewed changes
Comment thread doc/api/fs.md Outdated
bnoordhuis approved these changes Aug 22, 2019
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with suggestions.

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

Comment thread src/node_constants.cc Outdated
Comment thread test/parallel/test-fs-open-flags.js

joaocgreis commented Aug 22, 2019

Copy link
Copy Markdown
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 commented Aug 24, 2019

Copy link
Copy Markdown
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 mentioned this pull request Sep 2, 2019
Closed

joaocgreis commented Sep 2, 2019

Copy link
Copy Markdown
Member Author

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

bnoordhuis approved these changes Sep 3, 2019
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell reviewed Sep 3, 2019
View reviewed changes
Comment thread doc/api/fs.md Outdated

joaocgreis commented Sep 4, 2019

Copy link
Copy Markdown
Member Author

@jasnell updated, PTAL.

joaocgreis commented Sep 9, 2019

Copy link
Copy Markdown
Member Author

@jasnell any objection for landing this?

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

Copy link
Copy Markdown
Collaborator

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

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

joaocgreis commented Sep 11, 2019

Copy link
Copy Markdown
Member Author

Landed in 902c9fa

joaocgreis closed this Sep 11, 2019
joaocgreis mentioned this pull request Sep 12, 2019
Closed
joaocgreis mentioned this pull request Sep 12, 2019
Closed
targos pushed a commit that referenced this pull request Sep 20, 2019
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 <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
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 mentioned this pull request Sep 24, 2019
Merged
joaocgreis added a commit to JaneaSystems/node that referenced this pull request Sep 24, 2019
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 <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
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 <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
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
+2 more reviewers
mscdex mscdex left review comments
ronkorving ronkorving approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL