Bug 182064 – Replace tryLargeMemalignVirtual with tryLargeZeroedMemalignVirtual and use it to allocate large zeroed memory in Wasm

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

This allows us to skip a lot of memset(0) on WasmBench, leading to a big speedup on iOS.

[Radar WebKit Bug Importer]

<rdar://problem/36840132>

[Saam Barati]

Created attachment 332223 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 332225 [details]
patch

[Saam Barati]

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

[Saam Barati]

Created attachment 332226 [details]
patch

[EWS Watchlist]

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.

[Geoffrey Garen]

I guess the Linux API is this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/vm/pagemap.txt.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Geoffrey Garen]

> 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?

[Keith Miller]

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

[Saam Barati]

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

[Saam Barati]

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.

[Saam Barati]

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

[Saam Barati]

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

[Saam Barati]

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

[Saam Barati]

Created attachment 332314 [details]
patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Geoffrey Garen]

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

[Geoffrey Garen]

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

[Saam Barati]

Created attachment 332333 [details]
test fix on EWS

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 332342 [details]
maybe patch for landing

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 332445 [details]
WIP

We're now using mmap to zero memory.

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 332567 [details]
patch

[EWS Watchlist]

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.

[JF Bastien]

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.

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 332591 [details]
patch

[EWS Watchlist]

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.

[JF Bastien]

Comment on attachment 332591 [details]
patch

Talked offline, I have reservation, but I think this does the right thing for now so r=me

[Saam Barati]

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.

[Saam Barati]

Created attachment 332621 [details]
patch

[EWS Watchlist]

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.

[Geoffrey Garen]

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

[Saam Barati]

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.

[Saam Barati]

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.

[Geoffrey Garen]

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.

[Saam Barati]

Created attachment 332683 [details]
patch

[Saam Barati]

Created attachment 332684 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 332685 [details]
patch

[Saam Barati]

Created attachment 332686 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 332687 [details]
patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Saam Barati]

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

[Geoffrey Garen]

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.

[Saam Barati]

Created attachment 332838 [details]
patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 332838 [details]
patch for landing

Clearing flags on attachment: 332838

Committed r227951: <https://trac.webkit.org/changeset/227951>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.