Closed Bug 932398 Opened 9 years ago Closed 9 years ago

Add portable version of msync/FlushViewOfFile

Categories

(NSPR :: NSPR, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
4.10.3

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: [games])

Attachments

(3 files, 3 obsolete files)

Attached patch add-pr-flush-mem-to-file (obsolete) — 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)
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)
r? ping.  Getting close to needing this.
r? ping.  This will soon be the only thing blocking bug 929236 which is a major priority for the games project.
Whiteboard: [games]
Sorry, been very backed up on reviews lately. Will review today.
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+
(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.)
Flags: needinfo?(ted)
(My main question is whether I can just land my patch in the tree.)
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)
Great, thanks!  Here's the patch with the .def fix.
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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.10.3
Awesome.  Will that happen in this bug or will we need a second bug?
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+
Status: RESOLVED → REOPENED
Priority: -- → P1
Resolution: FIXED → ---
(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.
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?
Luke: if you could provide a new patch with the requested changes that would be much easier for me.
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)
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-
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).
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)
I attached the wrong patch earlier.
Attachment #8338959 - Attachment is obsolete: true
Attachment #8338959 - Flags: review?(luke)
Attachment #8338964 - Flags: review?(luke)
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 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+
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.