Bug 187160 - Disable IsoHeaps when Gigacage is off
Summary: Disable IsoHeaps when Gigacage is off
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
: 187387 (view as bug list)
Depends on: 187609 187497
Blocks: 201061
  Show dependency treegraph
 
Reported: 2018-06-28 15:01 PDT by Michael Saboff
Modified: 2019-08-22 18:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.80 KB, patch)
2018-06-28 15:12 PDT, Michael Saboff
saam: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (322.38 KB, application/zip)
2018-06-28 18:19 PDT, EWS Watchlist
no flags Details
Updated patch to work around compiler issue. (2.67 KB, patch)
2018-07-03 16:49 PDT, Michael Saboff
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.78 MB, application/zip)
2018-07-03 20:27 PDT, EWS Watchlist
no flags Details
Updated patch (9.90 KB, patch)
2018-07-05 14:09 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 (3.24 MB, application/zip)
2019-08-22 14:30 PDT, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2018-06-28 15:01:13 PDT
If Gigacage is disabled, it may be due to lack of address space.  Therefore we should also turn off IsoHeaps.
Comment 1 Michael Saboff 2018-06-28 15:02:00 PDT
<rdar://problem/36029149>
Comment 2 Michael Saboff 2018-06-28 15:12:05 PDT
Created attachment 343858 [details]
Patch
Comment 3 Saam Barati 2018-06-28 16:03:19 PDT
Comment on attachment 343858 [details]
Patch

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

> Source/bmalloc/ChangeLog:7
> +

nit: The description you put in bugzilla for this bug should go here.
Comment 4 Saam Barati 2018-06-28 16:03:49 PDT
(In reply to Michael Saboff from comment #0)
> If Gigacage is disabled, it may be due to lack of address space.  Therefore
> we should also turn off IsoHeaps.

Also probably worth stating that we expect IsoHeaps to use more virtual memory than not using IsoHeaps and why
Comment 5 Saam Barati 2018-06-28 16:03:54 PDT
(In reply to Michael Saboff from comment #0)
> If Gigacage is disabled, it may be due to lack of address space.  Therefore
> we should also turn off IsoHeaps.

Also probably worth stating that we expect IsoHeaps to use more virtual memory than not using IsoHeaps and why
Comment 6 EWS Watchlist 2018-06-28 18:19:30 PDT
Comment on attachment 343858 [details]
Patch

Attachment 343858 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8376450

Number of test failures exceeded the failure limit.
Comment 7 EWS Watchlist 2018-06-28 18:19:31 PDT
Created attachment 343879 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 Michael Saboff 2018-06-28 18:40:28 PDT
Committed r233347: <https://trac.webkit.org/changeset/233347>
Comment 9 Ryan Haddad 2018-06-29 08:11:03 PDT
(In reply to Michael Saboff from comment #8)
> Committed r233347: <https://trac.webkit.org/changeset/233347>
This change causes crashes on WK1 (seen with LayoutTests and API tests)

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/builds/4479

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010f7af3e3 bmalloc::PerThread<bmalloc::PerHeapKind<bmalloc::Cache> >::getSlowCase() + 35 (PerThread.h:143)
1   com.apple.JavaScriptCore      	0x000000010f7af384 bmalloc::Cache::tryAllocateSlowCaseNullCache(bmalloc::HeapKind, unsigned long) + 20 (Cache.cpp:53)
2   com.apple.WebCore             	0x000000011dd45c0f bmalloc::Cache::tryAllocate(bmalloc::HeapKind, unsigned long) + 47 (Cache.h:70)
3   com.apple.WebCore             	0x000000011dd45abb bmalloc::api::tryMalloc(unsigned long, bmalloc::HeapKind) + 27 (bmalloc.h:43)
4   com.apple.WebCore             	0x00000001202065fb void* bmalloc::IsoTLS::allocateSlow<bmalloc::IsoConfig<1320u>, WebCore::FrameView>(bmalloc::api::IsoHeap<WebCore::FrameView>&, bool) + 107 (IsoTLSInlines.h:90)
5   com.apple.WebCore             	0x0000000120206545 void* bmalloc::IsoTLS::allocateImpl<bmalloc::IsoConfig<1320u>, WebCore::FrameView>(bmalloc::api::IsoHeap<WebCore::FrameView>&, bool) + 85 (IsoTLSInlines.h:71)
6   com.apple.WebCore             	0x00000001202064e5 void* bmalloc::IsoTLS::allocate<WebCore::FrameView>(bmalloc::api::IsoHeap<WebCore::FrameView>&, bool) + 37 (IsoTLSInlines.h:37)
7   com.apple.WebCore             	0x00000001201db271 bmalloc::api::IsoHeap<WebCore::FrameView>::allocate() + 33 (IsoHeapInlines.h:50)
8   com.apple.WebCore             	0x00000001201db245 WebCore::FrameView::operator new(unsigned long) + 69 (FrameView.cpp:125)
9   com.apple.WebCore             	0x00000001201d681a WebCore::FrameView::create(WebCore::Frame&) + 42 (FrameView.cpp:225)
10  com.apple.WebKitLegacy        	0x000000011a45bfc5 WebFrameLoaderClient::transitionToCommittedForNewPage() + 869 (WebFrameLoaderClient.mm:1476)
11  com.apple.WebCore             	0x000000012001c92f WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) + 1151 (FrameLoader.cpp:2135)
12  com.apple.WebCore             	0x000000012001b979 WebCore::FrameLoader::commitProvisionalLoad() + 2153 (FrameLoader.cpp:1954)
13  com.apple.WebCore             	0x000000011ffc440c WebCore::DocumentLoader::commitIfReady() + 60 (DocumentLoader.cpp:359)
14  com.apple.WebCore             	0x000000011ffc4772 WebCore::DocumentLoader::finishedLoading() + 306 (DocumentLoader.cpp:422)
15  com.apple.WebCore             	0x000000011ffce799 WebCore::DocumentLoader::maybeLoadEmpty() + 1241 (DocumentLoader.cpp:1672)
16  com.apple.WebCore             	0x000000011ffce958 WebCore::DocumentLoader::startLoadingMainResource(WebCore::ShouldContinue) + 360 (DocumentLoader.cpp:1688)
17  com.apple.WebCore             	0x000000012000ea7e WebCore::FrameLoader::init() + 302 (FrameLoader.cpp:312)
18  com.apple.WebCore             	0x00000001201d2284 WebCore::Frame::init() + 36 (Frame.cpp:204)
19  com.apple.WebKitLegacy        	0x000000011a44705e +[WebFrame(WebInternal) _createMainFrameWithPage:frameName:frameView:] + 270 (WebFrame.mm:332)
20  com.apple.WebKitLegacy        	0x000000011a55f50b -[WebView(WebPrivate) _commonInitializationWithFrameName:groupName:] + 6443 (WebView.mm:1511)
21  com.apple.WebKitLegacy        	0x000000011a5608c9 -[WebView(WebPrivate) _initWithFrame:frameName:groupName:] + 297 (WebView.mm:1602)
22  com.apple.WebKitLegacy        	0x000000011a573779 -[WebView initWithFrame:frameName:groupName:] + 313 (WebView.mm:5835)
23  DumpRenderTree                	0x000000010edb39af createWebViewAndOffscreenWindow() + 255 (DumpRenderTree.mm:705)
24  DumpRenderTree                	0x000000010edb6150 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2368 (DumpRenderTree.mm:1950)
25  DumpRenderTree                	0x000000010edb5761 runTestingServerLoop() + 417 (DumpRenderTree.mm:1166)
26  DumpRenderTree                	0x000000010edb4c1e dumpRenderTree(int, char const**) + 1646 (DumpRenderTree.mm:1268)
27  DumpRenderTree                	0x000000010edb7eaf DumpRenderTreeMain(int, char const**) + 111 (DumpRenderTree.mm:1387)
28  DumpRenderTree                	0x000000010ee3d012 main + 34 (DumpRenderTreeMain.mm:34)
29  libdyld.dylib                 	0x00007fff7b4ef015 start + 1
Comment 10 Ryan Haddad 2018-06-29 08:27:57 PDT
Rolled out in https://trac.webkit.org/r233358 to get WK1 tests (and EWS) running again.
Comment 11 Michael Saboff 2018-07-03 16:49:26 PDT
Created attachment 344241 [details]
Updated patch to work around compiler issue.
Comment 12 EWS Watchlist 2018-07-03 18:09:24 PDT
Comment on attachment 344241 [details]
Updated patch to work around compiler issue.

Attachment 344241 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/8430655

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
apiTests
Comment 13 EWS Watchlist 2018-07-03 20:27:09 PDT
Comment on attachment 344241 [details]
Updated patch to work around compiler issue.

Attachment 344241 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8432136

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 14 EWS Watchlist 2018-07-03 20:27:20 PDT
Created attachment 344260 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 15 Saam Barati 2018-07-03 22:13:46 PDT
Comment on attachment 344241 [details]
Updated patch to work around compiler issue.

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

> Source/bmalloc/bmalloc/BCompiler.h:42
> +// Clang versions before 10.00 have a bug where static member variables of an inlined
> +// templated class gets implicitely instantiated in each linked object where used.

Why not just move s_mallocFallbackState out of the class to a global static variable?
Comment 16 Mark Lam 2018-07-04 00:06:03 PDT
Comment on attachment 344241 [details]
Updated patch to work around compiler issue.

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

>> Source/bmalloc/bmalloc/BCompiler.h:42
>> +// templated class gets implicitely instantiated in each linked object where used.
> 
> Why not just move s_mallocFallbackState out of the class to a global static variable?

I agree with Saam.  Also, fwiw, there's a typo: /implicitely/implicitly/.
Comment 17 Michael Saboff 2018-07-05 10:36:10 PDT
(In reply to Saam Barati from comment #15)
> Comment on attachment 344241 [details]
> Updated patch to work around compiler issue.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344241&action=review
> 
> > Source/bmalloc/bmalloc/BCompiler.h:42
> > +// Clang versions before 10.00 have a bug where static member variables of an inlined
> > +// templated class gets implicitely instantiated in each linked object where used.
> 
> Why not just move s_mallocFallbackState out of the class to a global static
> variable?

That is not the variable(s) in question.  The issue is with PerThreadStorage in PerThread.h and its three statics:
    static bool s_didInitialize;
    static pthread_key_t s_key;
    static std::once_flag s_onceFlag;
There needs to be one (and only one) instance of these variables for each specialized instance of the templated class.  There are currently two types used to specialize, PerHeapKind<Cache> and PerHeapKind<Heap>.  It seemed easiest to make the change for clang >= v10.

This bug exists in the current code with clang < v10, but isn't triggered through normal usage.  If you run DumpRenderTree with bmalloc_IsoHeap=false with a clang v9.x Debug build, you'll get the crash.
Comment 18 Michael Saboff 2018-07-05 10:36:34 PDT
(In reply to Mark Lam from comment #16)
> Comment on attachment 344241 [details]
> Updated patch to work around compiler issue.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344241&action=review
> 
> >> Source/bmalloc/bmalloc/BCompiler.h:42
> >> +// templated class gets implicitely instantiated in each linked object where used.
> > 
> > Why not just move s_mallocFallbackState out of the class to a global static variable?
> 
> I agree with Saam.  Also, fwiw, there's a typo: /implicitely/implicitly/.

Fixed locally.
Comment 19 Michael Saboff 2018-07-05 14:09:38 PDT
Created attachment 344365 [details]
Updated patch

Working with Mark, we were able to come up with template magic to instantiate and export one instance of each specialized PerThread class static and ensure that all modules link to those variables.
Comment 20 Saam Barati 2018-07-05 14:11:33 PDT
Comment on attachment 344365 [details]
Updated patch

r=me
Comment 21 EWS Watchlist 2018-07-05 14:12:20 PDT
Attachment 344365 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/PerThread.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Mark Lam 2018-07-05 14:14:24 PDT
Comment on attachment 344365 [details]
Updated patch

r=me too.
Comment 23 WebKit Commit Bot 2018-07-05 16:12:11 PDT
Comment on attachment 344365 [details]
Updated patch

Clearing flags on attachment: 344365

Committed r233547: <https://trac.webkit.org/changeset/233547>
Comment 24 WebKit Commit Bot 2018-07-05 16:12:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 David Kilzer (:ddkilzer) 2018-07-05 17:21:37 PDT
(In reply to WebKit Commit Bot from comment #23)
> Comment on attachment 344365 [details]
> Updated patch
> 
> Clearing flags on attachment: 344365
> 
> Committed r233547: <https://trac.webkit.org/changeset/233547>

Build fix for iOS Simulator Debug (and probably other ports):

Committed r233550: <https://trac.webkit.org/changeset/233550>
Comment 26 WebKit Commit Bot 2018-07-09 17:32:00 PDT
Re-opened since this is blocked by bug 187497
Comment 27 Ryan Haddad 2018-07-10 09:08:17 PDT
*** Bug 187387 has been marked as a duplicate of this bug. ***
Comment 28 Ryan Haddad 2018-07-10 09:09:19 PDT
This change was rolled out because it introduced flakiness with fullscreen media tests on macOS WK1. See https://bugs.webkit.org/show_bug.cgi?id=187387
Comment 29 Michael Saboff 2018-07-12 11:27:28 PDT
Committed r233773: <https://trac.webkit.org/changeset/233773>
Comment 30 Michael Saboff 2018-07-12 11:29:30 PDT
I remanded the patch with the additional change that DumpRenderTree by default uses Gigacages.  This allows the flaky media tests to run without timing out.

I filed <https://bugs.webkit.org/show_bug.cgi?id=187609> to track the issue with DumpRenderTree and these tests that time out.
Comment 31 WebKit Commit Bot 2019-08-22 14:30:03 PDT
The commit-queue just saw media/media-fullscreen-loop-inline.html flake (DumpRenderTree crashed) while processing attachment 377039 [details] on bug 201050.
Bot: webkit-cq-01  Port: <class 'webkitpy.common.config.ports.MacPort'>  Platform: Mac OS X 10.13.6
Comment 32 WebKit Commit Bot 2019-08-22 14:30:04 PDT
Created attachment 377045 [details]
Archive of layout-test-results from webkit-cq-01