Bug 182064 - Replace tryLargeMemalignVirtual with tryLargeZeroedMemalignVirtual and use it to allocate large zeroed memory in Wasm
Summary: Replace tryLargeMemalignVirtual with tryLargeZeroedMemalignVirtual and use it...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks: 182146
  Show dependency treegraph
 
Reported: 2018-01-24 14:36 PST by Saam Barati
Modified: 2018-01-31 21:36 PST (History)
23 users (show)

See Also:


Attachments
patch (13.62 KB, patch)
2018-01-24 20:07 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (13.65 KB, patch)
2018-01-24 20:18 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (13.63 KB, patch)
2018-01-24 20:20 PST, Saam Barati
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (2.62 MB, application/zip)
2018-01-24 21:50 PST, EWS Watchlist
no flags Details
patch (14.47 KB, patch)
2018-01-25 14:15 PST, Saam Barati
ggaren: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (2.95 MB, application/zip)
2018-01-25 15:43 PST, EWS Watchlist
no flags Details
test fix on EWS (14.41 KB, patch)
2018-01-25 16:23 PST, Saam Barati
no flags Details | Formatted Diff | Diff
maybe patch for landing (14.56 KB, patch)
2018-01-25 18:39 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (12.78 KB, patch)
2018-01-26 18:04 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (15.69 KB, patch)
2018-01-29 12:19 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (16.63 KB, patch)
2018-01-29 15:40 PST, Saam Barati
saam: review-
Details | Formatted Diff | Diff
patch (32.57 KB, patch)
2018-01-29 19:36 PST, Saam Barati
ggaren: review-
Details | Formatted Diff | Diff
patch (32.88 KB, patch)
2018-01-30 12:32 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (32.88 KB, patch)
2018-01-30 12:33 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (33.01 KB, patch)
2018-01-30 12:37 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (33.01 KB, patch)
2018-01-30 12:38 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (33.03 KB, patch)
2018-01-30 12:41 PST, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.57 MB, application/zip)
2018-01-30 14:26 PST, EWS Watchlist
no flags Details
patch for landing (32.22 KB, patch)
2018-01-31 19:02 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-01-24 14:36:22 PST
This allows us to skip a lot of memset(0) on WasmBench, leading to a big speedup on iOS.
Comment 1 Radar WebKit Bug Importer 2018-01-24 14:36:43 PST
<rdar://problem/36840132>
Comment 2 Saam Barati 2018-01-24 20:07:35 PST
Created attachment 332223 [details]
patch
Comment 3 EWS Watchlist 2018-01-24 20:08:59 PST
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 4 Saam Barati 2018-01-24 20:12:18 PST
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.
Comment 5 Saam Barati 2018-01-24 20:18:07 PST
Created attachment 332225 [details]
patch
Comment 6 Saam Barati 2018-01-24 20:19:26 PST
(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.
Comment 7 Saam Barati 2018-01-24 20:20:14 PST
Created attachment 332226 [details]
patch
Comment 8 EWS Watchlist 2018-01-24 20:21:14 PST
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.
Comment 9 Geoffrey Garen 2018-01-24 20:54:37 PST
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 10 EWS Watchlist 2018-01-24 21:50:56 PST
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
Comment 11 EWS Watchlist 2018-01-24 21:50:57 PST
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 12 EWS Watchlist 2018-01-24 23:37:07 PST
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 13 EWS Watchlist 2018-01-25 02:06:47 PST
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
Comment 14 Michael Catanzaro 2018-01-25 09:42:24 PST
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....
Comment 15 Michael Catanzaro 2018-01-25 09:44:12 PST
(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.)
Comment 16 Michael Catanzaro 2018-01-25 09:46:16 PST
(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.
Comment 17 Michael Catanzaro 2018-01-25 09:47:44 PST
(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"
Comment 18 Geoffrey Garen 2018-01-25 11:13:13 PST
> 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?
Comment 19 Keith Miller 2018-01-25 11:18:25 PST
(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.
Comment 20 Saam Barati 2018-01-25 11:21:56 PST
(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.
Comment 21 Saam Barati 2018-01-25 11:22:46 PST
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.
Comment 22 Saam Barati 2018-01-25 11:36:20 PST
(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....
Comment 23 Saam Barati 2018-01-25 11:40:37 PST
(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?
Comment 24 Saam Barati 2018-01-25 13:59:45 PST
(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.
Comment 25 Saam Barati 2018-01-25 14:15:09 PST
Created attachment 332314 [details]
patch
Comment 26 EWS Watchlist 2018-01-25 14:16:24 PST
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 27 EWS Watchlist 2018-01-25 15:43:35 PST
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
Comment 28 EWS Watchlist 2018-01-25 15:43:36 PST
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
Comment 29 Geoffrey Garen 2018-01-25 15:45:14 PST
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 30 Geoffrey Garen 2018-01-25 15:45:22 PST
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".
Comment 31 Saam Barati 2018-01-25 16:23:52 PST
Created attachment 332333 [details]
test fix on EWS
Comment 32 EWS Watchlist 2018-01-25 16:25:23 PST
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.
Comment 33 Saam Barati 2018-01-25 18:39:29 PST
Created attachment 332342 [details]
maybe patch for landing
Comment 34 EWS Watchlist 2018-01-25 18:40:21 PST
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.
Comment 35 Saam Barati 2018-01-26 18:04:36 PST
Created attachment 332445 [details]
WIP

We're now using mmap to zero memory.
Comment 36 EWS Watchlist 2018-01-26 18:07:18 PST
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.
Comment 37 Saam Barati 2018-01-29 12:19:13 PST
Created attachment 332567 [details]
patch
Comment 38 EWS Watchlist 2018-01-29 12:21:20 PST
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 39 JF Bastien 2018-01-29 15:12:08 PST
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 40 Saam Barati 2018-01-29 15:20:40 PST
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 41 Saam Barati 2018-01-29 15:22:46 PST
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 42 Saam Barati 2018-01-29 15:27:17 PST
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 ...)
Comment 43 Saam Barati 2018-01-29 15:40:30 PST
Created attachment 332591 [details]
patch
Comment 44 EWS Watchlist 2018-01-29 15:42:53 PST
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 45 JF Bastien 2018-01-29 17:07:31 PST
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 46 Saam Barati 2018-01-29 18:52:23 PST
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.
Comment 47 Saam Barati 2018-01-29 19:36:08 PST
Created attachment 332621 [details]
patch
Comment 48 EWS Watchlist 2018-01-29 19:37:07 PST
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 49 Geoffrey Garen 2018-01-30 11:00:09 PST
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 50 Saam Barati 2018-01-30 11:31:40 PST
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 51 Saam Barati 2018-01-30 11:38:37 PST
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 52 Geoffrey Garen 2018-01-30 11:58:44 PST
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.
Comment 53 Saam Barati 2018-01-30 12:32:26 PST
Created attachment 332683 [details]
patch
Comment 54 Saam Barati 2018-01-30 12:33:22 PST
Created attachment 332684 [details]
patch
Comment 55 EWS Watchlist 2018-01-30 12:36:51 PST
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.
Comment 56 Saam Barati 2018-01-30 12:37:09 PST
Created attachment 332685 [details]
patch
Comment 57 Saam Barati 2018-01-30 12:38:44 PST
Created attachment 332686 [details]
patch
Comment 58 EWS Watchlist 2018-01-30 12:40:25 PST
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.
Comment 59 Saam Barati 2018-01-30 12:41:09 PST
Created attachment 332687 [details]
patch
Comment 60 EWS Watchlist 2018-01-30 12:43:25 PST
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 61 EWS Watchlist 2018-01-30 14:26:46 PST
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
Comment 62 EWS Watchlist 2018-01-30 14:26:47 PST
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
Comment 63 Saam Barati 2018-01-30 14:28:09 PST
(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 64 Geoffrey Garen 2018-01-31 16:34:21 PST
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.
Comment 65 Saam Barati 2018-01-31 19:02:09 PST
Created attachment 332838 [details]
patch for landing
Comment 66 EWS Watchlist 2018-01-31 19:03:11 PST
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 67 WebKit Commit Bot 2018-01-31 21:36:46 PST
Comment on attachment 332838 [details]
patch for landing

Clearing flags on attachment: 332838

Committed r227951: <https://trac.webkit.org/changeset/227951>
Comment 68 WebKit Commit Bot 2018-01-31 21:36:48 PST
All reviewed patches have been landed.  Closing bug.