| Summary: | Replace tryLargeMemalignVirtual with tryLargeZeroedMemalignVirtual and use it to allocate large zeroed memory in Wasm | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||||||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aperez, benjamin, cdumez, clopez, cmarcelo, commit-queue, dbates, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, jfbastien, keith_miller, mark.lam, mcatanzaro, msaboff, rmorisset, rniwa, ticaiolima, webkit-bug-importer, ysuzuki, zan | ||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 182146 | ||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||
|
Description
Saam Barati
2018-01-24 14:36:22 PST
Created attachment 332223 [details]
patch
Attachment 332223 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ZeroFillPages.cpp:64: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 332223 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332223&action=review > Source/WTF/wtf/ZeroFillPages.cpp:57 > +#if !OS(DARWIN) > + // Reading through the linux kernel code, they don't set any flag > + // besides INCORE. This code assumes other non-darwin OSs are also > + // less precise. > + flags |= MINCORE_INCORE; > +#endif I'm not at all confident in the soundness of this on linux yet. After reading some of their source code, I'm still not 100% if this is correct for things swapped to disk. Created attachment 332225 [details]
patch
(In reply to Saam Barati from comment #4) > Comment on attachment 332223 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332223&action=review > > > Source/WTF/wtf/ZeroFillPages.cpp:57 > > +#if !OS(DARWIN) > > + // Reading through the linux kernel code, they don't set any flag > > + // besides INCORE. This code assumes other non-darwin OSs are also > > + // less precise. > > + flags |= MINCORE_INCORE; > > +#endif > > I'm not at all confident in the soundness of this on linux yet. After > reading some of their source code, I'm still not 100% if this is correct for > things swapped to disk. The linux kernel essentially sets these bits to "1" if the page is "up to date". I'm unsure of what that means exactly for something that's swapped to disk. If any linux folks know, or know somebody who we can ping, that'd be great. Created attachment 332226 [details]
patch
Attachment 332226 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ZeroFillPages.cpp:64: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style. I guess the Linux API is this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/vm/pagemap.txt. Comment on attachment 332226 [details] patch Attachment 332226 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6203650 New failing tests: workers/wasm-hashset.html Created attachment 332231 [details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 332226 [details] patch Attachment 332226 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6204229 New failing tests: wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.default-wasm wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-cjit-yes-tls-context Comment on attachment 332226 [details] patch Attachment 332226 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6203943 New failing tests: wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/trap-from-start.js.default-wasm wasm.yaml/wasm/function-tests/trap-from-start-async.js.default-wasm wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-call-ic wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-cjit-yes-tls-context Wow, this is complicated! I've CCed some more of developers who I bet know way more about memory performance than I do. And I see Yusuke is CCed already. (In reply to Saam Barati from comment #6) > The linux kernel essentially sets these bits to "1" if the page is "up to > date". I'm unsure of what that means exactly for something that's swapped to > disk. If any linux folks know, or know somebody who we can ping, that'd be > great. I don't think relying on the kernel source code is a good idea, because this is a glibc interface and the C library doesn't always use the syscall directly. See http://man7.org/linux/man-pages/man2/mincore.2.html for the glibc's mincore documentation. Key points there: a) "mincore() returns a vector that indicates whether pages of the calling process's virtual memory are resident in core (RAM), and so will not cause a disk access (page fault) if referenced." And: "the least significant bit of each byte will be set if the corresponding page is currently resident in memory." So the value should be 0 for a page that's swapped to disk, because it's not resident. b) Looks like whether the page is resident is the only thing you can determine from glibc's mincore(). There's no equivalent of MINCORE_MODIFIED or MINCORE_MODIFIED_OTHER, so you can't use mincore() to determine if a page has been modified. (The /proc API that Geoffrey found would be one way to determine that.) Hence I think all the else clause under "So if we haven't modified this virtual page, it must be zero" should be inside the #if OS(DARWIN) conditional. c) "The vec argument must point to an array containing at least (length+PAGE_SIZE-1) / PAGE_SIZE bytes." But you allocate an array that is only length / PAGE_SIZE bytes. I *think* that *should* be OK, because of the precondition "It's required that 'base' is page aligned and 'bytes' is a multiple of the system page size" which should ensure that the extra bytes are not needed... but it seems a bit bold to use a smaller array than the documented requirement, even if it should be fine.... d) "[T]he contents of vec may already be stale by the time this call returns." I'm going to guess that is not OK. So the following ASSERT would be wrong on Linux at least, unless zeroFillPages. I found the FreeBSD documentation https://www.unix.com/man-page/FreeBSD/2/mincore/, which says: "The information returned by mincore() may be out of date by the time the system call returns. The only way to ensure that a page is resident is to lock it into memory with the mlock(2) system call." Are you confident that ASSERT is safe, even on Darwin? It looks like mincore() should probably only be used on locked pages. e) I don't think it matters, but the type of the third argument is unsigned char* on Linux, not char*. I'm not sure if we'll see a compiler warning about that or not. f) Some of our braver customers use alternative C libraries instead of glibc. It's not really advisable, because I'm sure WebKit has many glibc assumptions that we don't know about, but it would be good to not rely on the glibc documentation at all. Unfortunately, mincore is not in POSIX, which is probably why the glibc interface is so different from Darwin's. So #OS(LINUX) is not the right guard to use here; it should be #if defined(__GLIBC__). And the perf improvement will likely be most important when an alternate C library is used, because usually only very small embedded systems use other C libraries (musl, ulibc, or bionic).... Now I have a newfound appreciation of POSIX. :P In conclusion: thanks for trying to bring this speedup to Linux! But it might be best to stick with the slow patch (the simple memset) and file a new bug for improving this on Linux. Not sure how confident you are after reading the above.... (In reply to Michael Catanzaro from comment #14) > but it seems a bit bold to use a smaller array than the > documented requirement, even if it should be fine.... (The consequence of getting this wrong would be buffer overflow.) (In reply to Michael Catanzaro from comment #14) > d) "[T]he contents of vec may already be stale by the time this call > returns." I'm going to guess that is not OK. So the following ASSERT would > be wrong on Linux at least, unless zeroFillPages. unless zeroFillPages is only called on locked memory. But that is not a documented precondition. (In reply to Michael Catanzaro from comment #14) > But it might be best to stick with the slow patch (the simple memset) I meant: "the slow path" > In conclusion: thanks for trying to bring this speedup to Linux! But it
> might be best to stick with the slow patch (the simple memset) and file a
> new bug for improving this on Linux. Not sure how confident you are after
> reading the above....
I'm sympathetic to this logic for most performance issues -- but, this particular performance issue is so significant that I would argue that a WASM implementation is incomplete / incorrect without it.
But maybe this isn't a concern on Linux because Linux always gets fast memory, which doesn't do the memset thing?
(In reply to Geoffrey Garen from comment #18) > But maybe this isn't a concern on Linux because Linux always gets fast > memory, which doesn't do the memset thing? Fast memory still does memset(0). Wasm memory must be zero'd when first initialized, unless requested by the instance itself. (In reply to Keith Miller from comment #19) > (In reply to Geoffrey Garen from comment #18) > > But maybe this isn't a concern on Linux because Linux always gets fast > > memory, which doesn't do the memset thing? > > Fast memory still does memset(0). Wasm memory must be zero'd when first > initialized, unless requested by the instance itself. Not on linux. On linux, we rely on madvise(..., MADV_DONTNEED) to zero the mmapped fast memory. So linux is probably never calling this path in most Wasm programs, at least on x86, not sure about ARM linuxes. The only reason it wouldn't call through to this path is if some WebAssembly program allocated many memories and exceeded our fast memory cap. It looks like even on Darwin, this patch has some issues. Going to test locally now. When I ran debug JSC wasm tests yesterday, they all passed. Maybe something is happening in release. (In reply to Michael Catanzaro from comment #14) > Wow, this is complicated! I've CCed some more of developers who I bet know > way more about memory performance than I do. And I see Yusuke is CCed > already. > > (In reply to Saam Barati from comment #6) > > The linux kernel essentially sets these bits to "1" if the page is "up to > > date". I'm unsure of what that means exactly for something that's swapped to > > disk. If any linux folks know, or know somebody who we can ping, that'd be > > great. > > I don't think relying on the kernel source code is a good idea, because this > is a glibc interface and the C library doesn't always use the syscall > directly. See http://man7.org/linux/man-pages/man2/mincore.2.html for the > glibc's mincore documentation. Key points there: > > a) "mincore() returns a vector that indicates whether pages of the calling > process's virtual memory are resident in core (RAM), and so will not cause a > disk access (page fault) if referenced." And: "the least significant bit of > each byte will be set if the corresponding page is currently resident in > memory." So the value should be 0 for a page that's swapped to disk, because > it's not resident. > > b) Looks like whether the page is resident is the only thing you can > determine from glibc's mincore(). There's no equivalent of MINCORE_MODIFIED > or MINCORE_MODIFIED_OTHER, so you can't use mincore() to determine if a page > has been modified. (The /proc API that Geoffrey found would be one way to > determine that.) Hence I think all the else clause under "So if we haven't > modified this virtual page, it must be zero" should be inside the #if > OS(DARWIN) conditional. Yeah, this makes mincore a nonstarter for linux. > > c) "The vec argument must point to an array containing at least > (length+PAGE_SIZE-1) / PAGE_SIZE bytes." But you allocate an array that is > only length / PAGE_SIZE bytes. I *think* that *should* be OK, because of the > precondition "It's required that 'base' is page aligned and 'bytes' is a > multiple of the system page size" which should ensure that the extra bytes > are not needed... but it seems a bit bold to use a smaller array than the > documented requirement, even if it should be fine.... The asserts I have guarantees these numbers are equal... > > d) "[T]he contents of vec may already be stale by the time this call > returns." I'm going to guess that is not OK. So the following ASSERT would > be wrong on Linux at least, unless zeroFillPages. I found the FreeBSD > documentation https://www.unix.com/man-page/FreeBSD/2/mincore/, which says: > "The information returned by mincore() may be out of date by the time the > system call returns. The only way to ensure that a page is resident is to > lock it into memory with the mlock(2) system call." Are you confident that > ASSERT is safe, even on Darwin? It looks like mincore() should probably only > be used on locked pages. This function relies on the fact that some other thread isn't modifying the memory we're querying. If that's not true, all bets are off. > > e) I don't think it matters, but the type of the third argument is unsigned > char* on Linux, not char*. I'm not sure if we'll see a compiler warning > about that or not. > > f) Some of our braver customers use alternative C libraries instead of > glibc. It's not really advisable, because I'm sure WebKit has many glibc > assumptions that we don't know about, but it would be good to not rely on > the glibc documentation at all. Unfortunately, mincore is not in POSIX, > which is probably why the glibc interface is so different from Darwin's. So > #OS(LINUX) is not the right guard to use here; it should be #if > defined(__GLIBC__). And the perf improvement will likely be most important > when an alternate C library is used, because usually only very small > embedded systems use other C libraries (musl, ulibc, or bionic).... > > Now I have a newfound appreciation of POSIX. :P > > In conclusion: thanks for trying to bring this speedup to Linux! But it > might be best to stick with the slow patch (the simple memset) and file a > new bug for improving this on Linux. Not sure how confident you are after > reading the above.... (In reply to Build Bot from comment #13) > Comment on attachment 332226 [details] > patch > > Attachment 332226 [details] did not pass jsc-ews (mac): > Output: http://webkit-queues.webkit.org/results/6203943 > > New failing tests: > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.default-wasm > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-tls-context > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-call-ic > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-eager-jettison > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-cjit-yes-tls- > context > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-eager-jettison > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-cjit-yes-tls-context > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-eager-jettison > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-eager-jettison > wasm.yaml/wasm/function-tests/trap-from-start.js.default-wasm > wasm.yaml/wasm/function-tests/trap-from-start-async.js.default-wasm > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes- > tls-context > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-tls-context > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-call-ic > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-call-ic > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-tls-context > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-cjit-yes-tls- > context I can't reproduce this locally. Maybe the bots are running an older OS that has a broken mincore? (In reply to Saam Barati from comment #23) > (In reply to Build Bot from comment #13) > > Comment on attachment 332226 [details] > > patch > > > > Attachment 332226 [details] did not pass jsc-ews (mac): > > Output: http://webkit-queues.webkit.org/results/6203943 > > > > New failing tests: > > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.default-wasm > > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-tls-context > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic > > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-call-ic > > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-eager-jettison > > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-cjit-yes-tls- > > context > > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-eager-jettison > > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-cjit-yes-tls-context > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-eager-jettison > > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-eager-jettison > > wasm.yaml/wasm/function-tests/trap-from-start.js.default-wasm > > wasm.yaml/wasm/function-tests/trap-from-start-async.js.default-wasm > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes- > > tls-context > > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-tls-context > > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-call-ic > > wasm.yaml/wasm/function-tests/trap-from-start-async.js.wasm-no-call-ic > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context > > wasm.yaml/wasm/function-tests/trap-from-start.js.wasm-no-tls-context > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm > > wasm.yaml/wasm/js-api/dont-mmap-zero-byte-memory.js.wasm-no-cjit-yes-tls- > > context > > I can't reproduce this locally. Maybe the bots are running an older OS that > has a broken mincore? Yeah, they are. Working on a new version that chunks out mincore calls. Created attachment 332314 [details]
patch
Attachment 332314 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/ZeroFillPages.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332314 [details] patch Attachment 332314 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6213591 New failing tests: workers/wasm-hashset.html Created attachment 332326 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
r=me
mac-debug bot looks angry. You should check it before landing.
Regressions: Unexpected crashes (1)
workers/wasm-hashset.html [ Crash ]
> Source/WTF/wtf/ZeroFillPages.cpp:45
> + size_t systemPageSize = pageSize();
I like to write this as "size_t pageSize = WTF::pageSize();". Otherwise, it reads like you made a subtle change to the value when you transformed it from "page size" to "system page size".
Comment on attachment 332314 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332314&action=review r=me mac-debug bot looks angry. You should check it before landing. > Source/WTF/wtf/ZeroFillPages.cpp:45 > + size_t systemPageSize = pageSize(); I like to write this as "size_t pageSize = WTF::pageSize();". Otherwise, it reads like you made a subtle change to the value when you transformed it from "page size" to "system page size". Created attachment 332333 [details]
test fix on EWS
Attachment 332333 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/ZeroFillPages.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 332342 [details]
maybe patch for landing
Attachment 332342 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/ZeroFillPages.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 332445 [details]
WIP
We're now using mmap to zero memory.
Attachment 332445 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Gigacage.h:123: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:133: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:138: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:100: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:100: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 332567 [details]
patch
Attachment 332567 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:74: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:74: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 4 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332567 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332567&action=review Maybe I misunderstanding, but it seems like we want to tell the allocator: please virtually allocate "requested" size (whatever "initial" was in WebAssembly.Memory's constructor), but also virtually allocate a bunch of uncommitted pages at the end. Basically, Memory::fastMappedBytes() is 4GiB + 128MiB, and I don't expect most calls to ever try to get close to that entire amount. > Source/WTF/wtf/Gigacage.cpp:48 > + memset(result, 0, size); This commits them physically though? It's kinda misleading to call it zeroed virtual pages since it's committed. > Source/bmalloc/bmalloc/VMAllocate.h:134 > + void* result = mmap(p, vmSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON | MAP_FIXED | BMALLOC_NORESERVE, BMALLOC_VM_TAG, 0); Should we use the WebAssembly VM tag again? My original code did this and the kernel utilities know about it so they show up as different VM tags. Comment on attachment 332567 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332567&action=review >> Source/WTF/wtf/Gigacage.cpp:48 >> + memset(result, 0, size); > > This commits them physically though? It's kinda misleading to call it zeroed virtual pages since it's committed. This is for non-bmalloc builds. I can open a FIXME here, but I won't fix it in this patch. >> Source/bmalloc/bmalloc/VMAllocate.h:134 >> + void* result = mmap(p, vmSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON | MAP_FIXED | BMALLOC_NORESERVE, BMALLOC_VM_TAG, 0); > > Should we use the WebAssembly VM tag again? My original code did this and the kernel utilities know about it so they show up as different VM tags. Maybe we can pipe this information later on down into bmalloc. But we haven't used the Wasm tag in a while. I won't bring it back in this patch. Comment on attachment 332567 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332567&action=review >>> Source/WTF/wtf/Gigacage.cpp:48 >>> + memset(result, 0, size); >> >> This commits them physically though? It's kinda misleading to call it zeroed virtual pages since it's committed. > > This is for non-bmalloc builds. I can open a FIXME here, but I won't fix it in this patch. at least for posix, it looks like this gives us zeroed memory. Idk if Windows does or not. Comment on attachment 332567 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332567&action=review >>>> Source/WTF/wtf/Gigacage.cpp:48 >>>> + memset(result, 0, size); >>> >>> This commits them physically though? It's kinda misleading to call it zeroed virtual pages since it's committed. >> >> This is for non-bmalloc builds. I can open a FIXME here, but I won't fix it in this patch. > > at least for posix, it looks like this gives us zeroed memory. Idk if Windows does or not. Actually, it looks like my memset may be unnecessary here. Windows uses VirtualAlloc, which gives us zeroed: https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx The Posix OSAllocator uses mmap(... MAP_ANON ...) Created attachment 332591 [details]
patch
Attachment 332591 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.cpp:50: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:74: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:74: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332591 [details]
patch
Talked offline, I have reservation, but I think this does the right thing for now so r=me
Comment on attachment 332591 [details]
patch
This patch ends up serving up memory that was left in a state after calling madvise(FREE_REUSABLE), which is wrong.
Created attachment 332621 [details]
patch
Attachment 332621 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.cpp:50: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/bmalloc/ChangeLog:18: Line contains tab character. [whitespace/tab] [5]
Total errors found: 6 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332621 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332621&action=review Looks mostly right but I see an issue here. > Source/WTF/wtf/Gigacage.cpp:50 > + for (size_t i = 0; i < size; ++i) > + ASSERT(static_cast<char*>(result)[i] == 0); If you change char* to uintptr_t* and change size to size / sizeof(uintptr_t), you can do this ASSERT a word at a time, since you know that allocation requests must be word-aligned. That will make debug builds 8X faster at this assert. > Source/bmalloc/bmalloc/VMAllocate.h:132 > +inline void vmZeroAndPurgePhysical(void* p, size_t vmSize) > +{ > + vmValidatePhysical(p, vmSize); See comment below about physical pages. Physical pages should only be used for madvise. > Source/bmalloc/bmalloc/VMAllocate.h:133 > + // MAP_ANON guarantees the memory is zeroed. This also almost certainly You can remove "almost certainly". > Source/bmalloc/bmalloc/bmalloc.cpp:54 > + char* end = base + size; > + size_t range = end - pageAlignedBase; > + size_t numPagesToZero = range / physicalPageSize; > + size_t bytesToZero = numPagesToZero * physicalPageSize; > + vmZeroAndPurgePhysical(pageAlignedBase, numPagesToZero * physicalPageSize); I don't think it's correct to do this in terms of physical pages. You're invoking mmap, so you should use virtual pages. Technically, I don't think this will cause an observable bug on any system we use. But it's logically wrong. The physical page size concept is an accommodation for the fact that on Darwin OS's mmap and madvise see memory differently. mmap uses virtual pages and madvise uses physical pages. But I don't think we want any of this rounding business. We just want to take our range [base, base + size) and nuke it, and that's a virtual range. > Source/bmalloc/bmalloc/bmalloc.cpp:63 > + Heap& heap = PerProcess<PerHeapKind<Heap>>::get()->at(kind); Why don't you need the lock around this get() or at()? Comment on attachment 332621 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332621&action=review >> Source/WTF/wtf/Gigacage.cpp:50 >> + ASSERT(static_cast<char*>(result)[i] == 0); > > If you change char* to uintptr_t* and change size to size / sizeof(uintptr_t), you can do this ASSERT a word at a time, since you know that allocation requests must be word-aligned. That will make debug builds 8X faster at this assert. You're too kind to the windows folks ;-) I will fix. >> Source/bmalloc/bmalloc/bmalloc.cpp:54 >> + vmZeroAndPurgePhysical(pageAlignedBase, numPagesToZero * physicalPageSize); > > I don't think it's correct to do this in terms of physical pages. You're invoking mmap, so you should use virtual pages. > > Technically, I don't think this will cause an observable bug on any system we use. But it's logically wrong. > > The physical page size concept is an accommodation for the fact that on Darwin OS's mmap and madvise see memory differently. mmap uses virtual pages and madvise uses physical pages. > > But I don't think we want any of this rounding business. We just want to take our range [base, base + size) and nuke it, and that's a virtual range. Nice catch. Will fix. >> Source/bmalloc/bmalloc/bmalloc.cpp:63 >> + Heap& heap = PerProcess<PerHeapKind<Heap>>::get()->at(kind); > > Why don't you need the lock around this get() or at()? This is how the code was before. Reading it PerProcess, it does its own synchronization for for ::get(). Reading PerHeapKind, it always initializes numHeaps worth of T (Heap in this situation) in its constructor. at() just loads from memory that's already initialized in PerHeapKind constructor. Comment on attachment 332621 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332621&action=review >>> Source/bmalloc/bmalloc/bmalloc.cpp:54 >>> + vmZeroAndPurgePhysical(pageAlignedBase, numPagesToZero * physicalPageSize); >> >> I don't think it's correct to do this in terms of physical pages. You're invoking mmap, so you should use virtual pages. >> >> Technically, I don't think this will cause an observable bug on any system we use. But it's logically wrong. >> >> The physical page size concept is an accommodation for the fact that on Darwin OS's mmap and madvise see memory differently. mmap uses virtual pages and madvise uses physical pages. >> >> But I don't think we want any of this rounding business. We just want to take our range [base, base + size) and nuke it, and that's a virtual range. > > Nice catch. Will fix. Given that size is user input, I think we need to either: - assert that size here is aligned to virtual page size - do the rounding at the end, and memset(0) the remaining bytes at the end Given alignment is also input to this function, I still think the right thing to do is allow for alignments that are different than virtual page size. So I think we really do want the rounding at the beginning and end here. If we don't do it at the beginning, mmap could fail. If we don't do it at the end, mmap might overwrite extra bytes at the last page that we don't want zeroed. Comment on attachment 332621 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332621&action=review >>>> Source/bmalloc/bmalloc/bmalloc.cpp:54 >>>> + vmZeroAndPurgePhysical(pageAlignedBase, numPagesToZero * physicalPageSize); >>> >>> I don't think it's correct to do this in terms of physical pages. You're invoking mmap, so you should use virtual pages. >>> >>> Technically, I don't think this will cause an observable bug on any system we use. But it's logically wrong. >>> >>> The physical page size concept is an accommodation for the fact that on Darwin OS's mmap and madvise see memory differently. mmap uses virtual pages and madvise uses physical pages. >>> >>> But I don't think we want any of this rounding business. We just want to take our range [base, base + size) and nuke it, and that's a virtual range. >> >> Nice catch. Will fix. > > Given that size is user input, I think we need to either: > - assert that size here is aligned to virtual page size > - do the rounding at the end, and memset(0) the remaining bytes at the end > > Given alignment is also input to this function, I still think the right thing to do is allow for alignments that are different than virtual page size. > > So I think we really do want the rounding at the beginning and end here. If we don't do it at the beginning, mmap could fail. If we don't do it at the end, mmap might overwrite extra bytes at the last page that we don't want zeroed. Good point that size is user specified. The strategy I like in cases like this is to round up the alignment and size to a multiple of the page size. It simplifies the edge cases a lot, and we don't have reason to believe that people will call this function with small values that will care that we rounded up. Created attachment 332683 [details]
patch
Created attachment 332684 [details]
patch
Attachment 332684 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.cpp:51: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 5 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 332685 [details]
patch
Created attachment 332686 [details]
patch
Attachment 332686 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.cpp:51: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 5 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 332687 [details]
patch
Attachment 332687 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.cpp:51: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 5 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332687 [details] patch Attachment 332687 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6265076 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/navigation-redirect.https.html Created attachment 332708 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to Build Bot from comment #61) > Comment on attachment 332687 [details] > patch > > Attachment 332687 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/6265076 > > New failing tests: > imported/w3c/web-platform-tests/service-workers/service-worker/navigation- > redirect.https.html This isn't my patch. Comment on attachment 332687 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=332687&action=review r=me > Source/bmalloc/bmalloc/VMAllocate.h:145 > +inline size_t vmPageSizeVirtual() > +{ > + static size_t cached; > + if (!cached) > + cached = getpagesize(); > + BASSERT(isPowerOfTwo(cached)); > + return cached; > +} > + > +inline void vmValidateVirtual(size_t vmSize) > +{ > + UNUSED(vmSize); > + BASSERT(vmSize); > + BASSERT(vmSize == roundUpToMultipleOf(vmPageSizeVirtual(), vmSize)); > +} > + > +inline void vmValidateVirtual(void* p, size_t vmSize) > +{ > + vmValidateVirtual(vmSize); > + > + UNUSED(p); > + BASSERT(p); > + BASSERT(p == mask(p, ~(vmPageSizeVirtual() - 1))); > +} > + vmPageSize() and vmValidate() operate on virtual pages. You can use them. If you prefer the "Virtual" suffix, I'm cool with that. You'll just have to update more call sites. Created attachment 332838 [details]
patch for landing
Attachment 332838 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:76: The parameter name "kind" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:122: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.h:136: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/Gigacage.cpp:51: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 5 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332838 [details] patch for landing Clearing flags on attachment: 332838 Committed r227951: <https://trac.webkit.org/changeset/227951> All reviewed patches have been landed. Closing bug. |