Closed
Bug 932398
Opened 11 years ago
Closed 11 years ago
Add portable version of msync/FlushViewOfFile
Categories
(NSPR :: NSPR, defect, P1)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.10.3
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: [games])
Attachments
(3 files, 3 obsolete files)
4.90 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
7.52 KB,
patch
|
wtc
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
4.64 KB,
patch
|
luke
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
NSPR currently has the ability to portably mmap/munmap. Another bit of related, but missing functionality is msync. I'd like to use this in bug 929236 because it's simpler and faster than the alternatives.
Note: on Windows, the patch uses FlushViewOfFile followed by FlushFileBuffers because the MSDN page for FlushViewOfFile says that is what you need to do for a synchronous write to disk.
Attachment #824133 -
Flags: review?(ted)
Assignee | ||
Comment 1•11 years ago
|
||
Oops, missed the decl in _win95.h which is, apparently, quite necessary.
Attachment #824133 -
Attachment is obsolete: true
Attachment #824133 -
Flags: review?(ted)
Attachment #824517 -
Flags: review?(ted)
Assignee | ||
Comment 2•11 years ago
|
||
r? ping. Getting close to needing this.
Assignee | ||
Comment 3•11 years ago
|
||
r? ping. This will soon be the only thing blocking bug 929236 which is a major priority for the games project.
Whiteboard: [games]
Comment 4•11 years ago
|
||
Sorry, been very backed up on reviews lately. Will review today.
Comment 5•11 years ago
|
||
Comment on attachment 824517 [details] [diff] [review]
add-pr-flush-mem-to-file
Review of attachment 824517 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with one change.
::: nsprpub/pr/src/md/windows/ntmisc.c
@@ +1003,5 @@
> + PROsfd osfd;
> +
> + osfd = ( fd == (PRFileDesc*)-1 )? -1 : fd->secret->md.osfd;
> +
> + if (FlushViewOfFile(addr, len) && FlushFileBuffers((HANDLE) osfd)) {
These APIs only exist as of Windows XP. I don't know if that's an issue for NSPR (I seriously hope not.)
::: nsprpub/pr/src/nspr.def
@@ +229,4 @@
> PR_Malloc;
> PR_MemMap;
> PR_MemUnmap;
> + PR_FlushMemToFile;
I assume you changed the name of the function at some point and forgot to update this?
Attachment #824517 -
Flags: review?(ted) → review+
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
Thanks Ted! Where should I land this patch?
> I assume you changed the name of the function at some point and forgot to
> update this?
You're right. (I guess this .def file isn't used in any try configuration.)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(ted)
Assignee | ||
Comment 7•11 years ago
|
||
(My main question is whether I can just land my patch in the tree.)
Comment 8•11 years ago
|
||
If you can put up a finalized patch I'll land it for you in the NSPR repo, and then we can cut a beta release and sync it to m-c.
Flags: needinfo?(ted)
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
Great, thanks! Here's the patch with the .def fix.
Comment 11•11 years ago
|
||
http://hg.mozilla.org/projects/nspr/rev/8638930ae241
We'll need to cut a new NSPR beta (not hard) and sync to m-c to pick this up.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.10.3
Assignee | ||
Comment 12•11 years ago
|
||
Awesome. Will that happen in this bug or will we need a second bug?
Comment 13•11 years ago
|
||
Comment on attachment 8336163 [details] [diff] [review]
add-pr-flush-mem-to-file (finalized patch)
Review of attachment 8336163 [details] [diff] [review]:
-----------------------------------------------------------------
Overall this patch looks good. I suggest a change to the PR_SyncMemMap
function. Since the patch has already been checked in, I marked this
patch review+, but please write a new patch to address my review comments.
::: nsprpub/pr/include/prio.h
@@ +1860,5 @@
> + */
> +NSPR_API(PRStatus) PR_SyncMemMap(
> + PRFileDesc *fd,
> + void *addr,
> + PRUint32 len);
IMPORTANT: based on your Windows implementation of
this function in ntmisc.c, I think this function should
take only two arguments:
void *addr,
PRUint32 len
The FlushFileBuffers part in your Windows implementation
(in ntmisc.c) is already provided by the existing PR_Sync
function.
::: nsprpub/pr/src/md/unix/unix.c
@@ +3644,5 @@
> + PRUint32 len)
> +{
> + if (msync(addr, len, MS_SYNC) == 0) {
> + return PR_SUCCESS;
> + } else {
Omit "else" after a return statement.
@@ +3649,5 @@
> + if (errno == EINVAL) {
> + PR_SetError(PR_INVALID_ARGUMENT_ERROR, errno);
> + } else {
> + PR_SetError(PR_UNKNOWN_ERROR, errno);
> + }
I know you copied this error-mapping code from _MD_MemUnmap.
Please do this instead:
_PR_MD_MAP_DEFAULT_ERROR(errno);
::: nsprpub/pr/src/nspr.def
@@ +228,5 @@
> PR_MakeDir;
> PR_Malloc;
> PR_MemMap;
> PR_MemUnmap;
> + PR_SyncMemMap;
This symbol needs to be added to a new NSPR 4.10.3 section at the end
of this file, like this:
;+NSPR_4.10.3 {
;+ global:
PR_SyncMemMap;
;+} NSPR_4.9.2;
Attachment #8336163 -
Flags: review+
Attachment #8336163 -
Flags: checked-in+
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Priority: -- → P1
Resolution: FIXED → ---
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Wan-Teh Chang from comment #13)
> The FlushFileBuffers part in your Windows implementation
> (in ntmisc.c) is already provided by the existing PR_Sync
> function.
The semantics of PR_SyncMemMap is that, on return, the memory in the specified range has been flushed to disk. As the MSDN documentation explains
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366563%28v=vs.85%29.aspx
FlushViewOfFile is not sufficient to achieve this; FlushFileBuffers is required. If I removed the FlushFileBuffers, then the burden would be on the caller to make an extra call which would end up penalizing the unix path where msync is sufficient.
Assignee | ||
Comment 15•11 years ago
|
||
Ted: since you're the one checking this in: would you like me to post a patch to address the other nits or would you like to just do them yourself directly?
Comment 16•11 years ago
|
||
Luke: if you could provide a new patch with the requested changes that would be much easier for me.
Comment 17•11 years ago
|
||
Luke, Ted: this patch implements all of my suggested changes.
Since you know more about this than I do, I'll let you make
the final call. I am offering this patch for your convenience.
It seems that at least, you need a comment in unix.c to
point out that fsync is not necessary. This will address
the potential confusion when someone compares the
PR_SyncMemMap implementations in ntmisc.c and unix.c.
Also, if the |fd| argument is necessary, it may be better
to replace it with PRFileMap *fmap, which would fit better
would the NSPR file-mapping API. Note that PR_MemMap takes
a PRFileMap *fmap instead of a PRFileDesc *fd argument.
Attachment #8336966 -
Flags: superreview?(ted)
Attachment #8336966 -
Flags: review?(luke)
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 8336966 [details] [diff] [review]
Change PR_SyncMemMap to not sync the file, other cleanups
Thank you for the patch! As previously explained, FlushFileBuffers really is necessary. A comment on msync would be fine.
Attachment #8336966 -
Flags: review?(luke) → review-
Assignee | ||
Comment 19•11 years ago
|
||
Also I do not think it really makes sense to pass the PRFileMap to PR_SyncMemMap: the underlying OS calls on Windows don't require it; on the contrary: the docs specificially indicate that you can close the file-mapping before unmaping the view which means that the client might not even have a PRFileMap (they may have already PR_CloseFileMap'd it).
Comment 20•11 years ago
|
||
The "Sync" in "PR_SyncMemMap" can be interpreted to mean "synchronize" (flush)
or "synchronously". Although you already said "Synchronously flush", it still
confused me, so I added a sentence to reinforce that point. I also explained
the purpose of the "PRFileDesc *fd" argument.
In ntmisc.c, I deleted the potential value of -1 for |osfd|. I found that
(HANDLE)-1 is INVALID_HANDLE_VALUE, which is a valid argument for
CreateFileMapping, but it is not a valid argument for FlushFileBuffers.
Attachment #8336966 -
Attachment is obsolete: true
Attachment #8336966 -
Flags: superreview?(ted)
Attachment #8338959 -
Flags: review?(luke)
Comment 21•11 years ago
|
||
I attached the wrong patch earlier.
Attachment #8338959 -
Attachment is obsolete: true
Attachment #8338959 -
Flags: review?(luke)
Attachment #8338964 -
Flags: review?(luke)
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 8338964 [details] [diff] [review]
[Correct patch] Add and clarify comments, miscellaneous cleanups
Review of attachment 8338964 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8338964 -
Flags: review?(luke) → review+
Comment 23•11 years ago
|
||
Comment on attachment 8338964 [details] [diff] [review]
[Correct patch] Add and clarify comments, miscellaneous cleanups
Review of attachment 8338964 [details] [diff] [review]:
-----------------------------------------------------------------
https://hg.mozilla.org/projects/nspr/rev/4a6c30714ec1
Attachment #8338964 -
Flags: checked-in+
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•