Follow up from bug 945152.

The lack of this feature currently breaks the b2g Simulator on Windows when the Keyboard loads its dictionary. For now, I'll change the pref "dom.mapped_arraybuffer.enabled" to "false" on Windows. See bug 1038254.

If this is something that's still needed, I would like to look into it.

I don't think anything is currently waiting on this, but it does have one caller in Firefox that *seems* like it ought to be really nice for. I don't claim to really understand what's going on, but it looks like it's used when you're opening up an app:// or jar:// uri over XMLHttpRequest. The way it works now is that it attempts to use a memory-mapped buffer, and falls back to a simpler mechanism if it fails. And it currently will always fail on Windows: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#3845 Anyway, yes, it looks like this would be useful. The main thing to do is to implement AllocateMappedContent and DeallocateMappedContent in js/src/gc/Memory.cpp, and then remove the guards in nsXMLHttpRequest.cpp. It pretty much just needs to match the behavior of the Linux version, http://dxr.mozilla.org/mozilla-central/source/js/src/gc/Memory.cpp#691 (sorry, I know that may not mean all that much if you're only familiar with Windows APIS). It looks like you can just assume you don't need page alignment. I think that was for some earlier memory protection scheme to catch out of bounds accesses.

Thanks, I will try to get at least an initial patch up this weekend.

Just wanted to hear your thoughts on if I'm on the correct path. DeallocateMappedContent is a work-in-progress, but I'm very unsure about how to implement it. MSDN[1] states that you should unmap all of the allocated File Views by using UnmapViewOfFile, before closing the entire file mapping object with CloseHandle. Trouble is, DeallocateMappedContent takes a void* and a length, but UnmapViewOfFile only takes a void*. Also, the handle to the original file mapping is currently lost. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa366532%28v=vs.85%29.aspx

(In reply to bd339 from comment #5) > Created attachment 8755210 [details] [diff] [review] > use Windows file mapping API > > Just wanted to hear your thoughts on if I'm on the correct path. > > DeallocateMappedContent is a work-in-progress, but I'm very unsure about how > to implement it. MSDN[1] states that you should unmap all of the allocated > File Views by using UnmapViewOfFile, before closing the entire file mapping > object with CloseHandle. Oh, ugh. So the API really is different. > Trouble is, DeallocateMappedContent takes a void* > and a length, but UnmapViewOfFile only takes a void*. That doesn't sound like a problem. It just means Windows tracks the size for us. On Linux, you can unmap a portion of a mapping, which is never what we actually want here anyway. > Also, the handle to > the original file mapping is currently lost. Yeah, that's a problem. Sounds like we're going to have to keep track of more information for the Windows case. But wait. Wouldn't this all work if AllocateMappedContent did CreateFileMapping, MapViewOfFile, and then immediately CloseHandle on the file mapping object handle? It looks like the file won't actually be closed until the last view is unmapped.

Comment on attachment 8755210 [details] [diff] [review] use Windows file mapping API Review of attachment 8755210 [details] [diff] [review]: ----------------------------------------------------------------- Assuming my hypothesizing in that last comment is correct, this does look like it's on the right track.

I'm really not familiar enough with this API to know if closing the handle will invalidate the views, but I suppose it's worth a try? According to the MSDSN link in comment 5, CloseHandle will succeed even if there are still mapped views, so you could very well be correct. The way I understood your comment about DeallocateMappedContent, we don't really care if the length argument is ignored, because we always deallocate the entire view at once?

If this is what you meant I suppose we can remove the guards in nsXMLHttpRequest.cpp. Do we have a way of testing this?

Comment on attachment 8755990 [details] [diff] [review] don't leak Handle from CreateFileMapping Review of attachment 8755990 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Memory.cpp @@ +365,1 @@ > MOZ_ASSERT(UnmapViewOfFile(p)); MOZ_ASSERT(x) will be compiled out in optimized builds. Use MOZ_ALWAYS_TRUE instead.

(In reply to bd339 from comment #9) > Created attachment 8755990 [details] [diff] [review] > don't leak Handle from CreateFileMapping > > If this is what you meant I suppose we can remove the guards in > nsXMLHttpRequest.cpp. > Do we have a way of testing this? I am assuming existing tests will use this functionality once you remove the guards, but I don't know which ones offhand. I would just push to the try server. Do you have level 1 access to push to try? If not, attach the full patch and I'll push it for you.

Would you push this to try for me? I don't have access.

Pushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27a05ddc707b I also looked harder into how to enable this, and the patch I pushed has several additional changes to enable the feature and turn on tests that rely on it. Hopefully I didn't break anything in the process.

You're clearly serious enough about working on this, and competent enough to get it done, so I'm assigning it to you. Thanks!

I'm unsure of how to interpret the try server results. For instance the Windows XP build reports "busted" and the failure summary is as follows: js/src/tests/style/BadIncludes.h:10: error: +js/src/gc/Memory.cpp:18:19: error: js/src/tests/style/BadIncludesOrder-inl.h:5:6: error: TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output; diff is above Return code: 2 'make -k check' did not run successfully. Please check log for errors. # TBPL FAILURE # # TBPL FAILURE # I think it's telling me to order the includes differently, I just don't know how. Am I supposed to #include <io.h> before #include <psapi.h> because of the includes ought to be ordered alphabetically? If that's the case, then why are only the Windows 7 builds not busted?

Oh wow, what a mess. Yes, your analysis of the BadIncludes stuff is correct. It insists on alphabetical ordering, within various sections. So yes, io.h should come before psapi.h. But that doesn't stop the tests from running. All of the builds in that push (the 'B' ones) are marked as failed (either orange or red). Then you have a whole bunch of test failures -- which are mostly junk. Release engineering is working on getting Windows testing running on AWS, and the clipboard is misbehaving, so we're seeing lots and lots of spurious failures (if the machine name has 'spot' in it, and the test failure has anything to do with pasting or clipboards, it's bogus.) And it's really hard to see what might be real within that mess! One thing you could try -- they were talking about moving all of the clipboard tests into a different test suite so they stop cluttering up the results. I think it has been done now, so perhaps if you rebase on the latest code, you can get another try push without quite as much garbage in it? Err... except I'm the one pushing to try. So all of that is for me to do. Ok, here it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80119d9c8c02

I think at least the two busted SpiderMonkey tests on WinXP and Win8 seem related to what are doing in this bug. The three busted Win7 Jit tests appear to be the same. It looks like the tests were supposed to throw OOM, but didn't?

Ugh. Part of the problem is that the version I rebased on had some breakage that was backed out not long after. This push is a little better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd37a2e952b1e314ca5d2eb8d2989caa9b1a9c0c I haven't looked closely, but it looks like the failures you were worried about are still there. The M(oth) tests have a stack, it looks like. You might be able to reproduce some of these failures locally with ./mach test. (I'll try to fill in more details of how to do that on Tuesday; I'll need to figure it out too.)

https://treeherder.mozilla.org/logviewer.html#?job_id=21619155&repo=try#L15442 is a good failure to look at. The stack is in the log. I suspect you could rerun it locally with ./mach test browser/components/translation/test/unit/test_cld2.js The M(5) failure is from this code too. Actually, a bunch of them are. (You can find the stacks by looking for "PROCESS-CRASH" in the logs.)

test_cld2.js doesn't fail for me. Neither does one of the M(5) (toolkit/components/extensions/test/mochitest/test_ext_i18n.html). I can't even make the VS debugger hit breakpoints in AllocateMappedContent when I run the mochitest with ""mach mochitest --no-autorun toolkit/components/extensions/test/mochitest/test_ext_i18n.html" and then attach the debugger before I start the tests in the Nightly window. I'm running the tests on a Windows 10 machine, with an unoptimized debug x64 Nightly. Is this the correct workflow for reproducing and debugging Try server failures?

That seems right to me, though I don't know the procedure for attaching to the right process on Windows. You could always add an abort() to see if it's getting called at all, I guess. Or maybe it's time for printf debugging via try. I will be your voucher if you follow the instructions at https://www.mozilla.org/en-US/about/governance/policies/commit/ You could also add asserts for pointers to be non-null and things. Just to be sure, since it sounded like you might not be executing the code at all -- are you using your original patch, or the one I pushed to try? I had to add in some preference settings in order to make it even attempt to use this code. If you're missing those, then you wouldn't get any failures.

I totally didn't touch any prefs. I'll try the "dom.mapped_arraybuffer.enabled" mentioned in comment 1. Any others I need to fiddle with?

(In reply to bd339 from comment #22) > I totally didn't touch any prefs. I'll try the > "dom.mapped_arraybuffer.enabled" mentioned in comment 1. Any others I need > to fiddle with? Nvm. I applied your patch from try and while I am still unable to make browser/components/translation/test/unit/test_cld2.js hit any breakpoints (same goes for toolkit/components/extensions/test/mochitest/test_ext_i18n.html), however I'm getting crashes and running into breakpoints as expected when I run /dom/base/test/test_bug945152.html Usually I'm not this excited to see crashes!

Incorporates your changes from the Try push. It turns out the "file descriptor" given to AllocateMappedContent should actually already be a Windows HANDLE for a file. You'll notice a comment about supporting read+write access to the memory mapped file... To pass at least test_bug945152.html we can't unconditionally try to obtain a RW mapping. It seems that test opens its file in read-only mode. About the tests which were causing crashes for me locally: They're passing now! I suppose we can support RW mapping rather easily by first trying to reserve a RW mapping and then if that fails, reserve a read-only mapping before returning nullptr in case of failure. Is that something we want to do? I also have good news regarding the need to track extra information for Windows: I can verify (because I inspected the values in a debugger) that the returned pointer does not point to garbage data even after closing the file mapping handle! (I'm not sure if the data might be overwritten at some later point because it's been marked as available...) The way I read https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537%28v=vs.85%29.aspx it seems that if we continue closing the handle before returning the view, subsequent attempts to map another region of the same file-mapping could entail duplicating the data in memory because Windows wouldn't be able to tell that this mapping has already been made before. If it's the case, is that something we care about?

Also, I applied for access to the try server :) Would you vouch for me in bug 1278065?

Comment on attachment 8759990 [details] [diff] [review] support memory mapped files in Windows builds, 2nd rev. Review of attachment 8759990 [details] [diff] [review]: ----------------------------------------------------------------- Thank you very much for sticking with this and seeing it through! I'm going to request review from ehoogeveen, who is the de facto owner of gc/Memory.cpp, and request feedback from baku, who I *think* may know something about the usage of this in the browser. Or at least, he once said that he had some feedback to provide. ::: js/src/gc/Memory.cpp @@ +356,5 @@ > + > + // Windows requires the offset to be a whole multiple of the > + // allocation granularity. > + size_t alignOffset = offset/allocGranularity * allocGranularity; > + size_t alignLength = offset%allocGranularity + length; I'm not sure this alignment stuff is going to work. If the request is not already aligned, you will return a pointer that is not the beginning of the mapped area, and I'd expect UnmapViewOfFile to fail. For now, I think you'd be better off just failing if offset is not a multiple of allocGranularity.

Sorry, should've put this in the above f+ message. If you keep the alignment stuff, it would be better to use stuff from jsutil.h where applicable (AlignBytes, maybe?)

Comment on attachment 8759990 [details] [diff] [review] support memory mapped files in Windows builds, 2nd rev. Review of attachment 8759990 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! Clearing review for now due to the alignment issue in DeallocateMappedContent; other comments are minor. ::: js/src/gc/Memory.cpp @@ +332,5 @@ > > void* > AllocateMappedContent(int fd, size_t offset, size_t length, size_t alignment) > { > + // Make sure file exists and do sanity check for offset and size. Looks like your editor inserted tabs for indentation - untabify please :) Indentation in SpiderMonkey is 4 spaces per level. @@ +333,5 @@ > void* > AllocateMappedContent(int fd, size_t offset, size_t length, size_t alignment) > { > + // Make sure file exists and do sanity check for offset and size. > + HANDLE fHnd = (HANDLE)fd; Hmm, I'm not sure if this will do the right thing. HANDLE is a typedef for |void*|, but valid handles are all 32-bit for the sake of backward compatibility [1] (so using an |int| to refer to a HANDLE is fine). However, INVALID_HANDLE_VALUE is (HANDLE)(LONG_PTR)-1, so we need to sign-extend back to 64-bit. The following should work: HANDLE fHnd = reinterpret_cast<HANDLE>(intptr_t(fd)); [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203.aspx @@ +338,5 @@ > + if (fHnd == INVALID_HANDLE_VALUE) > + return nullptr; > + > + uint32_t fSizeHgh; > + uint32_t fSizeLow = GetFileSize(fHnd, (LPDWORD)&fSizeHgh); Nit: We generally avoid C-style casts. Constructor-style cast (|LPDWORD(&fSizeHgh)|) or reinterpret-cast should work here. There are a few more instances of this below - I suggest constructor-style casts for brevity. @@ +343,5 @@ > + if (fSizeLow == INVALID_FILE_SIZE) > + return nullptr; > + > + uint64_t fSize = ((uint64_t)fSizeHgh << 32) + fSizeLow; > + if (offset >= (size_t)fSize || length == 0 || length > fSize - offset) Hmm, I feel as though this should be an assertion, but I see that the XP_UNIX implementation does the same thing. @@ +348,5 @@ > + return nullptr; > + > + uint64_t mapSize = length + offset; > + // TODO: Support read-write mappings? > + HANDLE mHnd = CreateFileMapping(fHnd, NULL, PAGE_READONLY, mapSize >> 32, This seems to create an inconsistency between the XP_UNIX implementation and the XP_WIN implementation. Can we just remove PROT_WRITE from the XP_UNIX implementation? If we want read-write access, I think the API should indicate that - allowing for a mismatch between the file handle and the mapping seems like a potential security footgun (even if Windows prevents it). Oh, and a nit: I'm pretty sure you can just pass nullptr here and elsewhere. @@ +349,5 @@ > + > + uint64_t mapSize = length + offset; > + // TODO: Support read-write mappings? > + HANDLE mHnd = CreateFileMapping(fHnd, NULL, PAGE_READONLY, mapSize >> 32, > + mapSize, NULL); Code can use up to 99 columns (comments up to 80) in SpiderMonkey, so this call should (just) fit on a single line. @@ +356,5 @@ > + > + // Windows requires the offset to be a whole multiple of the > + // allocation granularity. > + size_t alignOffset = offset/allocGranularity * allocGranularity; > + size_t alignLength = offset%allocGranularity + length; I think this works, along with the line below to return the offset pointer (which I think Steve missed), but please write it like this for readability: size_t alignOffset = (offset / allocGranularity) * allocGranularity; size_t alignLength = (offset % allocGranularity) + length; @@ +357,5 @@ > + // Windows requires the offset to be a whole multiple of the > + // allocation granularity. > + size_t alignOffset = offset/allocGranularity * allocGranularity; > + size_t alignLength = offset%allocGranularity + length; > + void *view = MapViewOfFile(mHnd, FILE_MAP_READ, 0, alignOffset, alignLength); Nit: the * goes with the type (|void* view|). That's the last style nit I have, but in general you can use https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style as a reference. @@ +362,5 @@ > + CloseHandle(mHnd); > + if (!view) > + return nullptr; > + > + uint8_t *buf = (uint8_t*)view + offset-alignOffset; Nit: This can go on a single line, and let's avoid pointer arithmetic: return reinterpret_cast<void*>(uintptr_t(view) + (offset - alignOffset)); @@ +372,5 @@ > DeallocateMappedContent(void* p, size_t length) > { > + // Windows doesn't support partial unmapping, hence the length is ignored. > + if (p) > + MOZ_ALWAYS_TRUE(UnmapViewOfFile(p)); This should take the alignment stuff into account - right now this could pass an unaligned pointer to UnmapViewOfFile. ::: modules/libpref/init/all.js @@ +5018,5 @@ > // parameter omitted. > pref("dom.voicemail.defaultServiceId", 0); > > +// Enable mapped array buffer by default. > +pref("dom.mapped_arraybuffer.enabled", true); Should be able to remove the B2G-specific override for this too: https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#1023

(In reply to bd339 from comment #29) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=835f3eb54aee Thanks for the feedback, Emanuel! I've addressed most of your feedback in the Try push, but I'll await the results before asking for review.

(In reply to bd339 from comment #29) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=835f3eb54aee Lots of failures and crashes in this one. Trying again with a build that leaks the file-mapping handle to find out whether closing it before unmapping all views is a problem in the long run.

(In reply to bd339 from comment #31) > (In reply to bd339 from comment #29) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=835f3eb54aee > > Lots of failures and crashes in this one. Trying again with a build that > leaks the file-mapping handle to find out whether closing it before > unmapping all views is a problem in the long run. No, the problem with that push is that it looks like returning an unaligned pointer isn't going to work without further changes. The assertion failure looks like someone is storing the return value in a PrivateValue, and PrivateValues must be aligned. Specifically, it happens here: xul.dll!js::ArrayBufferObject::setDataPointer(js::ArrayBufferObject::BufferContents,js::ArrayBufferObject::OwnsState) [ArrayBufferObject.cpp:18b5cd262ccf : 584 So ArrayBuffers only accept aligned pointers for their contents. In my opinion, it would be easier to only support mapping aligned pointers, at least for now. You could file a followup bug to support unaligned pointers, but I'm skeptical that it would ever get used.

(In reply to Steve Fink [:sfink] [:s:] from comment #33) > So ArrayBuffers only accept aligned pointers for their contents. Aha, yes. Then supporting unaligned offsets is fundamentally impossible on Windows without a change to the API. Let's just return nullptr in that case.

I understand. I cancelled the remaining jobs on Try. I'll submit a patch that only supports aligned offsets. But won't there be a bunch of test failures from tests expecting it to work even though they provide unaligned offsets?

I suspect the real question is whether we need to support unaligned offsets in practice. It seems like a niche use-case to me, but I haven't looked at the callers. But if it's just tests, those can be fixed (or simplified).

Oh, hmm, it seems the allocation granularity is 64KiB in practice, so that's a pretty big constraint to put on |offset|. But I don't think setDataPointer needs that kind of granularity; this case in particular only requires that the least significant bit is clear. It seems strange to me that we're requesting an odd offset here - how big is this buffer?

I didn't implement the sanity checks as assertions for Windows nor for Unix, because I wasn't sure if you think it's the right thing to do. Also I don't know if it would be appropriate in this "Windows bug". I would also like your feedback regarding the placement of my implementation of AllocateMappedContent and DeallocateMappedContent. https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537%28v=vs.85%29.aspx seems to indicate that the file mapping api is only available to desktop apps (see "Requirements"). Should this implementation be restricted to the "WINAPI_PARTITION_DESKTOP" ifdef? About further work in this bug... does the "alignment" argument given to AllocateMappedContent not supply the alignment required to satisfy e.g. "setDataPointer"?

Comment on attachment 8761395 [details] [diff] [review] fail if unaligned mapping is requested Review of attachment 8761395 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay. Hmm, if this works it seems fine - do we rely on these allocations succeeding, or do we fall back to normal allocations if they fail? (In reply to bd339 from comment #38) > I would also like your feedback regarding the placement of my implementation > of AllocateMappedContent and DeallocateMappedContent. > https://msdn.microsoft.com/en-us/library/windows/desktop/aa366537%28v=vs. > 85%29.aspx seems to indicate that the file mapping api is only available to > desktop apps (see "Requirements"). Should this implementation be restricted > to the "WINAPI_PARTITION_DESKTOP" ifdef? Yes, you're right. The non-desktop stuff is there for Windows Phone (and the Windows Store, potentially), but we don't support that in any official capacity. Let's place the working version inside a WINAPI_PARTITION_DESKTOP ifdef and leave the previously existing dummy functions for mobile. That will at least compile, and if we ever want to support them officially we can just override the pref again. > About further work in this bug... does the "alignment" argument given to > AllocateMappedContent not supply the alignment required to satisfy e.g. > "setDataPointer"? Hmm, yes. Could you assert that |offset % alignment == 0| and see what failures you get? I'd also like to know how big the buffers we're allocating here are. Since this API allocates virtual pages directly under the hood, and the allocation granularity is 64KiB, we shouldn't be using it for tiny buffers. Maybe Linux is more flexible in this regard, but if we're creating tons of tiny file-backed buffers that still seems like a waste of resources.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #39) > I'd also like to know how big the buffers we're allocating here are. Since > this API allocates virtual pages directly under the hood, and the allocation > granularity is 64KiB, we shouldn't be using it for tiny buffers. Maybe Linux > is more flexible in this regard, but if we're creating tons of tiny > file-backed buffers that still seems like a waste of resources. From what I can tell of the current state, this is going to get used in very restrictive situations, from chrome-only code, where we're doing an XHR to an app:// or jar:// (?) URL. And I think app:// urls might be dead. So I'm guessing this will currently only be used when loading chunks of local data files. In the future, I could envision more uses, but I think we can just say it'll be restricted to what works with it. Other embedders might come up with some good uses for it.

Read something interesting (http://blog.libtorrent.org/2014/11/virtualalloc-pitfall/) and decided to try it out.

(In reply to bd339 from comment #41) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7e88f17c99 What are your thoughts on this run, Steve?

Oops, sorry, I didn't notice this needinfo. (In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #39) > Sorry about the delay. Hmm, if this works it seems fine - do we rely on > these allocations succeeding, or do we fall back to normal allocations if > they fail? The only caller falls back to normal allocations on failure. > I'd also like to know how big the buffers we're allocating here are. Since > this API allocates virtual pages directly under the hood, and the allocation > granularity is 64KiB, we shouldn't be using it for tiny buffers. Maybe Linux > is more flexible in this regard, but if we're creating tons of tiny > file-backed buffers that still seems like a waste of resources. I think we could just put a comment about the allocation granularity in the header file, and can call it good. (In reply to bd339 from comment #43) > (In reply to bd339 from comment #41) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7e88f17c99 > > What are your thoughts on this run, Steve? That looks as green as any push does these days. I'd say make the changes suggested (the header comment and the WINAPI_PARTITION_DESKTOP thing) and request review again. Then also push to try with the alignment assertion ehoogeveen suggested (not for landing, just for finding out whether any existing callers are requesting unaligned buffers), but in my opinion the outcome of that does not need to block landing -- as long as you return false when an unaligned request is made, the caller can (and does) handle it properly by falling back to regular allocations.

I don't know if you or Emanuel should review this, feel free to pass it over to him :) Basically this should prevent returning pointers not aligned according to the given alignment, while still aligning the mapping to Windows' allocation granularity if possible. When I added the comment regarding the allocation granularity on Windows to Memory.h, I noticed this comment[1] for AllocateMappedContent. I will push the test suggested by Emanuel to Try shortly so we can find out which tests are doing this. I would expect the tests requesting unaligned mappings to expect a nullptr in return, to test if AllocateMappedContent is handling said case correctly. Are the tests then supposed to pass the pointer around, causing it to be stored when it mustn't be?

Forgot to add this link to the comment above. [1]: https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Memory.h#38

Comment on attachment 8763275 [details] [diff] [review] support memory mapped files in Windows builds, 3rd rev. Review of attachment 8763275 [details] [diff] [review]: ----------------------------------------------------------------- I found a description of how this is used at https://wiki.mozilla.org/FirefoxOS/MappedArrayBuffer Given how similar they are, it would have been nice to have a single AllocateMappedContent function with internal platform #ifdefs, but there are enough questionable things in the unix AllocateMappedContent that this seems better for now. We could do a further cleanup that deletes the cruft from the unix function and then these could perhaps be merged (to reduce code duplication). I guess I'd still like ehoogeveen to talk a look at this, too. ::: js/src/gc/Memory.cpp @@ +292,5 @@ > + > + // Make sure file exists and do sanity check for offset and size. > + HANDLE hFile = reinterpret_cast<HANDLE>(intptr_t(fd)); > + MOZ_ASSERT(hFile != INVALID_HANDLE_VALUE); > + There's some stray whitespace here. @@ +310,5 @@ > + > + // Windows requires the offset to be a whole multiple of the allocation > + // granularity. > + size_t alignOffset = (offset / allocGranularity) * allocGranularity; > + size_t alignLength = (offset % allocGranularity) + length; Hm. We have two ways of computing the same thing in this file (see the other AllocateMappedContent), but perhaps allocGranularity is not guaranteed to be a power of two? Anyway, I'd prefer alignOffset = offset - (offset % allocGranularity); since you get a common (offset % allocGranularity) subexpression in alignOffset and alignLength, which makes it easier (for me) to understand. Does OffsetFromAligned(offset, allocGranularity) look better than (offset % allocGranularity)? I don't particularly like the OffsetFromAligned helper since it just obscures what's going on, but I noticed you used it in DeallocateMappedContent. Both should either use it or not use it, either is ok with me. @@ +311,5 @@ > + // Windows requires the offset to be a whole multiple of the allocation > + // granularity. > + size_t alignOffset = (offset / allocGranularity) * allocGranularity; > + size_t alignLength = (offset % allocGranularity) + length; > + void* map = MapViewOfFile(hMap, FILE_MAP_READ, 0, alignOffset, alignLength); I just re-read bug 945152 and FILE_MAP_READ will create an ArrayBuffer that is invalid according to the ES spec (as in, if you try to write to it, it'll crash.) You want FILE_MAP_COPY here, so that it is writable but it will not modify the contents of the mapped file. (The above PAGE_READONLY mapping is fine, though; you're allowed to make FILE_MAP_COPY views of PAGE_READONLY mappings.) @@ +315,5 @@ > + void* map = MapViewOfFile(hMap, FILE_MAP_READ, 0, alignOffset, alignLength); > + CloseHandle(hMap); > + if (!map) > + return nullptr; > + The other AllocateMappedMemory zeroes out the unaligned portions of the mapping at the beginning and end. But I *think* that might be a remnant from when ArrayBuffers had a header just before the data, since I can't think of a reason why that would be necessary now. (It is safe because it uses MAP_PRIVATE, the equivalent of FILE_MAP_COPY, so those zeroes don't modify the file.) @@ +327,5 @@ > + if (!p) > + return; > + > + // This assumes length is equal to the length given to the corresponding call > + // to AllocateMappedContent. I would comment out the 'length' parameter name. void DeallocateMappedContent(void* p, size_t /* length */) to make it more clear that you never use the length. (When I first read this comment, I assumed that it meant that this would only work if you passed in the correct length, namely the same length you used for AllocateMappedContent.) With the name commented out, that misinterpretation is not possible. @@ +328,5 @@ > + return; > + > + // This assumes length is equal to the length given to the corresponding call > + // to AllocateMappedContent. > + uintptr_t map = uintptr_t(p) - OffsetFromAligned(p, allocGranularity); As I already mentioned above, this should use the same thing as the alignOffset computation, whatever you choose for that.

(In reply to bd339 from comment #47) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4740efd4a80 The failures there are not as informative as I would have hoped. It just shows that, yes, things do call this with zip files containing unaligned elements. But we don't know what files it was trying to map for. (In reply to bd339 from comment #45) > Created attachment 8763275 [details] [diff] [review] > support memory mapped files in Windows builds, 3rd rev. > > I don't know if you or Emanuel should review this, feel free to pass it over > to him :) I did both. :) > Basically this should prevent returning pointers not aligned according to > the given alignment, while still aligning the mapping to Windows' allocation > granularity if possible. > When I added the comment regarding the allocation granularity on Windows to > Memory.h, I noticed this comment[1] for AllocateMappedContent. I will push > the test suggested by Emanuel to Try shortly so we can find out which tests > are doing this. I would expect the tests requesting unaligned mappings to > expect a nullptr in return, to test if AllocateMappedContent is handling > said case correctly. Are the tests then supposed to pass the pointer around, > causing it to be stored when it mustn't be? No, the assertion was just intended to find out if any existing tests used it with an unaligned pointer. It's ok if they do, since you'd return nullptr and presumably they would know to try something else. So you could print out the name of the file it's accessing. That would be interesting to find out if this function is largely useless as long as it insists on 8-byte alignment. But honestly, I don't really see the point. In my opinion, you can take the assertion back out, make the changes I listed above, make sure ehoogeveen is good with it, and land it.

Fixes the crash if the buffer is written to, as pointed out by Steve in comment 48. Other than that the only difference to the 3rd revision is the cosmetic fixes, per Steve's review. Steve, if you'd like me to push a try run logging the file names would you tell me how to log in the Mozilla code so it will be visible on the try server? Even if we don't need it this time, I still think the knowledge will become useful in the future :)

(In reply to bd339 from comment #50) > Steve, if you'd like me to push a try run logging the file names would you > tell me how to log in the Mozilla code so it will be visible on the try > server? Even if we don't need it this time, I still think the knowledge will > become useful in the future :) I'd say you don't need to do that unless ehoogeveen wants you to. As for how to do it, I actually don't know, but I think you could just fprintf(stderr) file and jarfile at http://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#1943 They are nsAutoCStrings, which sounds like they should be easy to get a char* out of? My guess would be fprintf(stderr, "GREP! mmap failed for reading %s from %s (probably due to unaligned offset)\n", file.BeginReading(), jarfile.BeginReading());

Oops I'm sorry, I noticed some wrong indentation in my previous patch. Looks like I had some tabs in there. Should be fixed now. Was also a fine oppourtunity to remove the now incorrect comment about AllocateMappedContent allocating read-only mappings.

Comment on attachment 8764380 [details] [diff] [review] support memory mapped files in Windows builds, 5th rev. Review of attachment 8764380 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updated patch! Just a few nits, mostly related to comments, but I think this turned out great. Sorry about the review lag! If you have the editbugs permission on Bugzilla, feel free to carry forward r+ with the comments addressed, and set the checkin-needed keyword. ::: js/src/gc/Memory.cpp @@ +282,5 @@ > > +void* > +AllocateMappedContent(int fd, size_t offset, size_t length, size_t alignment) > +{ > + // The alignment must be a whole multiple of the allocation granularity and It seems to me we have the opposite requirement: the allocation granularity must be a whole multiple of the alignment. That's also what you're testing for below. @@ +306,5 @@ > + HANDLE hMap = CreateFileMapping(hFile, nullptr, PAGE_READONLY, mapSize >> 32, mapSize, nullptr); > + if (!hMap) > + return nullptr; > + > + // Windows requires the offset to be a whole multiple of the allocation Nit: Perhaps say |MapViewOfFile| here rather than |Windows|. @@ +310,5 @@ > + // Windows requires the offset to be a whole multiple of the allocation > + // granularity. > + size_t alignOffset = offset - (offset % allocGranularity); > + size_t alignLength = length + (offset % allocGranularity); > + void* map = MapViewOfFile(hMap, FILE_MAP_COPY, 0, alignOffset, alignLength); Thanks for looking into this, Steve. Reading the document you linked, this indeed seems to be the desired behavior. @@ +315,5 @@ > + CloseHandle(hMap); > + if (!map) > + return nullptr; > + > + return reinterpret_cast<void*>(uintptr_t(map) + offset - alignOffset); Nit: I'd prefer this with parentheses around |offset - alignOffset|, to make it clear that we're adding the remainder of the offset. @@ +318,5 @@ > + > + return reinterpret_cast<void*>(uintptr_t(map) + offset - alignOffset); > +} > + > +// Deallocate mapped memory for object. Nit: Don't really need this comment. @@ +326,5 @@ > + if (!p) > + return; > + > + // This assumes length is equal to the length given to the corresponding call > + // to AllocateMappedContent. Does it? I don't think UnmapViewOfFile really cares what happened in between. Instead I'd like a comment here explaining that |p| might be an offset into the view due to the allocation granularity. ::: js/src/gc/Memory.h @@ +15,5 @@ > // Sanity check that our compiled configuration matches the currently > // running instance and initialize any runtime data needed for allocation. > +// On Windows allocations must be aligned to the systems allocation granularity > +// which seems to be 64KiB in practice. As such it may be wasteful to memory > +// map small buffers on Windows. This seems like a weird place for this comment. Perhaps move it to the comment above AllocateMappedContent. Suggested wording: // On Windows the minimum size for a mapping is the allocation granularity // (64KiB in practice), so mapping very small buffers is potentially wasteful.

This should fix the faulty comments pointed out by ehoogeveen.

Comment on attachment 8766034 [details] [diff] [review] support memory mapped files in Windows builds, 6th rev. The checkin flag is for bugs with multiple patches, mostly to indicate which have already landed (and which shouldn't land) :) I've added the checkin-needed keyword for you.

has problems to apply: renamed 988813 -> bug988813_patch11.diff applying bug988813_patch11.diff unable to find 'dom/base/nsXMLHttpRequest.cpp' for patching 3 out of 3 hunks FAILED -- saving rejects to file dom/base/nsXMLHttpRequest.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh bug988813_patch11.diff does this depend on some other bug ?

Ah, while fixing this bug nsXMLHTTPRequest.cpp was renamed. This one should be rebased and good to checkin!

This landed with the wrong bug number. It had bug 988831 rather than bug 988813: Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0be190ee52a support memory mapped files in Windows. r=ehoogeveen

Thanks Mark. A pity none of us caught that, but it's an understandable mistake :( Looks like it made it to central already, so I'm closing this: https://hg.mozilla.org/mozilla-central/rev/c0be190ee52a