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.

Oops, missed the decl in _win95.h which is, apparently, quite necessary.

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.

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?

(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.)

(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.

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.

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;

(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.

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.

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.

I attached the wrong patch earlier.

Comment on attachment 8338964 [details] [diff] [review] [Correct patch] Add and clarify comments, miscellaneous cleanups Review of attachment 8338964 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

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