Bug 170825 - WebAssembly: limit slow memories
Summary: WebAssembly: limit slow memories
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on: 170628
Blocks: 159775
  Show dependency treegraph
 
Reported: 2017-04-13 13:42 PDT by JF Bastien
Modified: 2017-04-19 12:38 PDT (History)
8 users (show)

See Also:


Attachments
patch (3.91 KB, patch)
2017-04-19 01:35 PDT, JF Bastien
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (6.18 KB, patch)
2017-04-19 10:31 PDT, JF Bastien
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (6.06 KB, patch)
2017-04-19 10:39 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-04-13 13:42:16 PDT
Bug #170628 limits the number of fast memories, partly because ASLR.

The code then falls back to slow memories. It first tries to virtually allocated any declared maximum (and in there, physically the initial), and if that fails it tries to physically allocate the initial without any extra.

This can still be used to cause a bunch of virtual allocation. We should probably impose a soft limit on slow memories as well. I think that limit should be against whole-process sum of virtually allocated slow memory (don't forget to include calls to grow!).

I'm not sure what the number should be, but I'll guess something around what the maximum for fast memory virtual allocation is.

Anything exceeding that limit would cause allocation/grow to fail.
Comment 1 JF Bastien 2017-04-19 01:35:15 PDT
Created attachment 307473 [details]
patch
Comment 2 Saam Barati 2017-04-19 09:49:49 PDT
Comment on attachment 307473 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307473&action=review

r=me. I'd like to at least see a test we run on Mac, and if it's flaky, we can consider what to do next. But let's land it before assuming it'll be flaky.

> Source/JavaScriptCore/ChangeLog:21
> +        I haven't added a test because the bots will likely be unhappy /

What about at least making it run on Mac?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:203
> +    size_t maximum = fastMemoryAllocationSoftLimit * Memory::fastMappedBytes();

I would really add a new variable for this, even if initially it's the same value. That way we can change them independent of one another.
Comment 3 JF Bastien 2017-04-19 10:31:31 PDT
Created attachment 307488 [details]
patch

Add test and factor out function as suggested.
Comment 4 WebKit Commit Bot 2017-04-19 10:33:30 PDT
Comment on attachment 307488 [details]
patch

Rejecting attachment 307488 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 307488, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 succeeded at 1 with fuzz 3.
patching file JSTests/wasm.yaml
Hunk #1 FAILED at 1.
Hunk #2 succeeded at 31 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file JSTests/wasm.yaml.rej
patching file JSTests/wasm/stress/oom.js
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/wasm/WasmMemory.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/3564516
Comment 5 JF Bastien 2017-04-19 10:39:01 PDT
Created attachment 307489 [details]
patch

Rebase. I self-merge-conflicted.
Comment 6 WebKit Commit Bot 2017-04-19 11:45:41 PDT
The commit-queue encountered the following flaky tests while processing attachment 307489 [details]:

The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2017-04-19 11:45:49 PDT
The commit-queue encountered the following flaky tests while processing attachment 307489 [details]:

media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html bug 168317 (author: graouts@apple.com)
The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2017-04-19 12:38:55 PDT
Comment on attachment 307489 [details]
patch

Clearing flags on attachment: 307489

Committed r215525: <http://trac.webkit.org/changeset/215525>
Comment 9 WebKit Commit Bot 2017-04-19 12:38:57 PDT
All reviewed patches have been landed.  Closing bug.