Bug 196837

Summary: [bmalloc] IsoHeap should have lower tier using shared IsoPage
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: bmallocAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, fpizlo, ggaren, pnormand, rniwa, saam, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 196959    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews116 for mac-highsierra
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch fpizlo: review+

Description Yusuke Suzuki 2019-04-11 16:27:06 PDT
...
Comment 1 Yusuke Suzuki 2019-04-11 16:28:33 PDT
Created attachment 367261 [details]
Patch

WIP. Still investigating and trying things
Comment 2 EWS Watchlist 2019-04-11 16:30:40 PDT
Attachment 367261 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:56:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 10 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Yusuke Suzuki 2019-04-11 18:41:08 PDT
Created attachment 367276 [details]
Patch

WIP
Comment 4 EWS Watchlist 2019-04-11 18:42:55 PDT
Attachment 367276 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:56:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 10 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Yusuke Suzuki 2019-04-11 18:53:26 PDT
Created attachment 367277 [details]
Patch

WIP
Comment 6 EWS Watchlist 2019-04-11 18:56:42 PDT
Attachment 367277 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:56:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 10 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yusuke Suzuki 2019-04-11 22:17:03 PDT
Created attachment 367290 [details]
Patch

WIP
Comment 8 Yusuke Suzuki 2019-04-11 22:18:00 PDT
Maybe, bump only thing is enough for this. This simplifies thing a lot.
Comment 9 EWS Watchlist 2019-04-11 22:21:00 PDT
Attachment 367290 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Yusuke Suzuki 2019-04-11 22:59:47 PDT
Rather than allocating each cells independently from a shared page, allocate bucket (8 cells) from shared page and use it is better? (it is logically introducing smaller page for slow allocations). Anyway, continue trying, and measuring...
Comment 11 Yusuke Suzuki 2019-04-11 23:13:02 PDT
Created attachment 367295 [details]
Patch

WIP
Comment 12 EWS Watchlist 2019-04-11 23:16:02 PDT
Attachment 367295 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Yusuke Suzuki 2019-04-11 23:44:33 PDT
Created attachment 367297 [details]
Patch

WIP
Comment 14 EWS Watchlist 2019-04-11 23:46:58 PDT
Attachment 367297 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 EWS Watchlist 2019-04-12 00:41:13 PDT
Comment on attachment 367297 [details]
Patch

Attachment 367297 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11851544

Number of test failures exceeded the failure limit.
Comment 16 EWS Watchlist 2019-04-12 00:41:14 PDT
Created attachment 367299 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 17 EWS Watchlist 2019-04-12 00:43:04 PDT
Comment on attachment 367297 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 18 EWS Watchlist 2019-04-12 00:43:05 PDT
Created attachment 367300 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 19 EWS Watchlist 2019-04-12 00:49:58 PDT
Comment on attachment 367297 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 20 EWS Watchlist 2019-04-12 00:49:59 PDT
Created attachment 367301 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 21 EWS Watchlist 2019-04-12 01:06:10 PDT
Comment on attachment 367297 [details]
Patch

Attachment 367297 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11851589

Number of test failures exceeded the failure limit.
Comment 22 EWS Watchlist 2019-04-12 01:06:12 PDT
Created attachment 367303 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 23 Yusuke Suzuki 2019-04-12 02:16:48 PDT
Created attachment 367305 [details]
Patch

WIP
Comment 24 EWS Watchlist 2019-04-12 02:19:36 PDT
Attachment 367305 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 EWS Watchlist 2019-04-12 03:15:13 PDT
Comment on attachment 367305 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 26 EWS Watchlist 2019-04-12 03:15:15 PDT
Created attachment 367308 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 27 EWS Watchlist 2019-04-12 03:20:02 PDT
Comment on attachment 367305 [details]
Patch

Attachment 367305 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11852539

Number of test failures exceeded the failure limit.
Comment 28 EWS Watchlist 2019-04-12 03:20:03 PDT
Created attachment 367309 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 29 EWS Watchlist 2019-04-12 03:27:39 PDT
Comment on attachment 367305 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 30 EWS Watchlist 2019-04-12 03:27:40 PDT
Created attachment 367310 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 31 EWS Watchlist 2019-04-12 03:38:01 PDT
Comment on attachment 367305 [details]
Patch

Attachment 367305 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11852584

Number of test failures exceeded the failure limit.
Comment 32 EWS Watchlist 2019-04-12 03:38:02 PDT
Created attachment 367311 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 33 Yusuke Suzuki 2019-04-12 19:02:46 PDT
Created attachment 367370 [details]
Patch
Comment 34 EWS Watchlist 2019-04-12 19:05:01 PDT
Attachment 367370 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:68:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 EWS Watchlist 2019-04-12 21:14:09 PDT
Comment on attachment 367370 [details]
Patch

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

New failing tests:
media/modern-media-controls/start-support/start-support-audio.html
http/tests/media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-live-broadcast.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/activeCues.html
http/tests/security/local-video-poster-from-remote.html
media/track/w3c/interfaces/TextTrack/cues.html
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-track.html
accessibility/mac/media-emits-object-replacement.html
fast/forms/input-user-modify.html
http/tests/media/clearkey/clear-key-hls-aes128.html
contentfiltering/allow-media-document.html
http/tests/preload/download_resources.html
fast/table/before-child-non-table-section-add-table-crash.html
fast/block/float/list-marker-is-float-crash.html
Comment 36 EWS Watchlist 2019-04-12 21:14:11 PDT
Created attachment 367378 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 37 EWS Watchlist 2019-04-13 00:02:08 PDT
Comment on attachment 367370 [details]
Patch

Attachment 367370 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11861174

Number of test failures exceeded the failure limit.
Comment 38 EWS Watchlist 2019-04-13 00:02:10 PDT
Created attachment 367380 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 39 EWS Watchlist 2019-04-13 02:14:27 PDT
Comment on attachment 367370 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 40 EWS Watchlist 2019-04-13 02:14:29 PDT
Created attachment 367381 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 41 Yusuke Suzuki 2019-04-15 21:41:33 PDT
DOMMatrix has TransformationMatrix as its member, and TransformationMatrix requires 16 byte alignment, and it is critical since TransformationMatrix uses SSE2 with strict alignment requirement. I think it was already broken... FastMalloc does not have any guarantee that it allocates memory with 16byte alignment.
Comment 42 Yusuke Suzuki 2019-04-15 23:53:48 PDT
(In reply to Yusuke Suzuki from comment #41)
> DOMMatrix has TransformationMatrix as its member, and TransformationMatrix
> requires 16 byte alignment, and it is critical since TransformationMatrix
> uses SSE2 with strict alignment requirement. I think it was already
> broken... FastMalloc does not have any guarantee that it allocates memory
> with 16byte alignment.

https://bugs.webkit.org/show_bug.cgi?id=196959
Comment 43 Yusuke Suzuki 2019-04-16 14:41:16 PDT
Created attachment 367574 [details]
Patch
Comment 44 EWS Watchlist 2019-04-16 14:45:35 PDT
Attachment 367574 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 Yusuke Suzuki 2019-04-16 15:42:54 PDT
Created attachment 367583 [details]
Patch
Comment 46 EWS Watchlist 2019-04-16 15:46:47 PDT
Attachment 367583 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 47 Yusuke Suzuki 2019-04-16 16:01:29 PDT
Created attachment 367588 [details]
Patch
Comment 48 EWS Watchlist 2019-04-16 16:03:45 PDT
Attachment 367588 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Yusuke Suzuki 2019-04-16 20:05:37 PDT
Created attachment 367603 [details]
Patch
Comment 50 EWS Watchlist 2019-04-16 20:07:13 PDT
Attachment 367603 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 51 Yusuke Suzuki 2019-04-16 20:30:52 PDT
Created attachment 367605 [details]
Patch
Comment 52 EWS Watchlist 2019-04-16 20:32:33 PDT
Attachment 367605 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:126:  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/IsoSharedHeap.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: 7 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 Yusuke Suzuki 2019-04-16 20:34:45 PDT
Created attachment 367606 [details]
Patch
Comment 54 EWS Watchlist 2019-04-16 20:37:57 PDT
Attachment 367606 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:126:  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/IsoSharedHeap.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: 7 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 55 Yusuke Suzuki 2019-04-16 21:09:06 PDT
Created attachment 367607 [details]
Patch
Comment 56 EWS Watchlist 2019-04-16 21:10:42 PDT
Attachment 367607 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 57 EWS Watchlist 2019-04-16 22:18:21 PDT
Comment on attachment 367607 [details]
Patch

Attachment 367607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11895736

Number of test failures exceeded the failure limit.
Comment 58 EWS Watchlist 2019-04-16 22:18:23 PDT
Created attachment 367609 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 59 EWS Watchlist 2019-04-16 22:19:00 PDT
Comment on attachment 367607 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 60 EWS Watchlist 2019-04-16 22:19:01 PDT
Created attachment 367610 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 61 EWS Watchlist 2019-04-16 22:22:02 PDT
Comment on attachment 367607 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 62 EWS Watchlist 2019-04-16 22:22:04 PDT
Created attachment 367611 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 63 EWS Watchlist 2019-04-16 22:48:22 PDT
Comment on attachment 367607 [details]
Patch

Attachment 367607 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11895791

Number of test failures exceeded the failure limit.
Comment 64 EWS Watchlist 2019-04-16 22:48:31 PDT
Created attachment 367615 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 65 Yusuke Suzuki 2019-04-16 23:01:51 PDT
Created attachment 367616 [details]
Patch
Comment 66 EWS Watchlist 2019-04-16 23:03:09 PDT
Attachment 367616 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Yusuke Suzuki 2019-04-17 12:47:30 PDT
Created attachment 367665 [details]
Patch
Comment 68 EWS Watchlist 2019-04-17 12:51:35 PDT
Attachment 367665 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 69 Yusuke Suzuki 2019-04-17 18:02:40 PDT
Created attachment 367705 [details]
Patch
Comment 70 EWS Watchlist 2019-04-17 18:04:47 PDT
Attachment 367705 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 71 Yusuke Suzuki 2019-04-17 18:56:39 PDT
Created attachment 367710 [details]
Patch
Comment 72 EWS Watchlist 2019-04-17 19:00:12 PDT
Attachment 367710 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/IsoHeapImplInlines.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoSharedPage.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]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:77:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:62:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:64:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/BCompiler.h:66:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/bmalloc/bmalloc/IsoSharedHeap.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: 7 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 73 Saam Barati 2019-04-17 22:30:27 PDT
Comment on attachment 367710 [details]
Patch

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

> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:62
> +        std::lock_guard<Mutex> locker(*m_lock);

How does this lock actually protect freeing from the page? Can’t multiple threads be freeing from the same page using different deallocators?
Comment 74 Yusuke Suzuki 2019-04-17 22:54:38 PDT
Comment on attachment 367710 [details]
Patch

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

>> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:62
>> +        std::lock_guard<Mutex> locker(*m_lock);
> 
> How does this lock actually protect freeing from the page? Can’t multiple threads be freeing from the same page using different deallocators?

"Can’t multiple threads be freeing from the same page using different deallocators?",

No, we can do that. Multiple threads cannot free shared cells with the same sizeof(Cell). This operation is serialized by this lock.

When freeing objects from the shared page, we do not need to modify the data in shared page. We are modifying the data in IsoHeapImpl of types.
This lock protects IsoHeapImpl. This `m_lock` comes from `PerProcess<IsoTLSDeallocatorEntry<Config>>` and IsoHeapImpl::lock is the reference to this `m_lock` too.
Comment 75 Yusuke Suzuki 2019-04-17 22:56:30 PDT
Comment on attachment 367710 [details]
Patch

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

>>> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:62
>>> +        std::lock_guard<Mutex> locker(*m_lock);
>> 
>> How does this lock actually protect freeing from the page? Can’t multiple threads be freeing from the same page using different deallocators?
> 
> "Can’t multiple threads be freeing from the same page using different deallocators?",
> 
> No, we can do that. Multiple threads cannot free shared cells with the same sizeof(Cell). This operation is serialized by this lock.
> 
> When freeing objects from the shared page, we do not need to modify the data in shared page. We are modifying the data in IsoHeapImpl of types.
> This lock protects IsoHeapImpl. This `m_lock` comes from `PerProcess<IsoTLSDeallocatorEntry<Config>>` and IsoHeapImpl::lock is the reference to this `m_lock` too.

BTW, we can make this locking fine-grained by having per IsoHeapImpl lock instead of using per-object-size lock.
Comment 76 Yusuke Suzuki 2019-04-19 20:29:46 PDT
Committed r244481: <https://trac.webkit.org/changeset/244481>
Comment 77 Radar WebKit Bug Importer 2019-04-19 20:31:42 PDT
<rdar://problem/50067525>
Comment 78 Zan Dobersek 2019-04-22 22:50:47 PDT
Comment on attachment 367710 [details]
Patch

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

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287
> +    void* result = result = m_sharedCells[index];

Is this here a typo?

GCC 8 warns the following:
../../Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287:11: warning: operation on ‘result’ may be undefined [-Wsequence-point]
     void* result = result = m_sharedCells[index];
Comment 79 Philippe Normand 2019-04-24 03:31:23 PDT
Looks like a typo indeed.
Comment 80 Yusuke Suzuki 2019-04-24 15:49:44 PDT
(In reply to Philippe Normand from comment #79)
> Looks like a typo indeed.

Yes, this is typo. Thanks, I'll fix it soon.
Comment 81 Yusuke Suzuki 2019-04-24 22:10:03 PDT
Committed r244634: <https://trac.webkit.org/changeset/244634>
Comment 82 Saam Barati 2019-04-25 11:57:48 PDT
Comment on attachment 367710 [details]
Patch

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

LGTM too, just a few questions, suggestions, and maybe one issue.

> Source/bmalloc/bmalloc/IsoAllocator.h:44
> +    AllocationMode considerUsingSharedAllocation();
> +

This isn't used.

> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:63
> +        static_cast<IsoSharedPage*>(page)->free<Config>(handle, ptr);

nit: Maybe take const std::lock_guard<Mutext>& as a parameter to free to indicate lock must be held.

> Source/bmalloc/bmalloc/IsoHeapImpl.h:63
> +    std::chrono::steady_clock::time_point m_slowPathTimePoint;

nit: the name of this variable doesn't need the type name in it. Maybe something like "m_lastSlowPathTime" or "m_timeOfLastSlowPath"

> Source/bmalloc/bmalloc/IsoHeapImpl.h:66
> +    unsigned m_usableBits { maxAllocationFromSharedMask };

can we have some kind of static assert that this bitmap is large enough to hold maxAllocationFromShared? Maybe:
static_assert(sizeof(m_usableBits) * 8 >= maxAllocationFromShared)

Also, I'm not a fan of this name. "Usable" is a bit vague. Maybe "m_availableShared" or "m_availableSharedIndices". Since this is only used for shared, I think there should be some description of "shared" here.

> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
> +    AllocationMode m_allocationMode { AllocationMode::Init };

This seems wrong, you never store to this variable besides turning it into Init?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:114
> +    void* allocateFromShared(bool abortOnFailure);

nit: Maybe also take lock_guard ref here to show that this is called while lock is held.

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:259
> +        // If we don't go to the allocation slow path during 1~ seconds, we think the allocation becomes quiescent state.

nit: 1~ => ~1

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:303
> +    m_usableBits &= (~(1U << index));

style nit: You don't need the outer parens.

> Source/bmalloc/bmalloc/IsoSharedConfig.h:32
> +static constexpr unsigned alignmentForIsoSharedAllocation = 16;

is this really needed?

> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:50
> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));

why cast to uintptr_t?

> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:76
> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));

maybe have a function that does this since this is done in a couple places?
Comment 83 Saam Barati 2019-04-25 12:05:11 PDT
Comment on attachment 367710 [details]
Patch

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

> Source/bmalloc/test/testbmalloc.cpp:258
> +static void testIsoMallocAndFreeFast()

I wonder why this works since you never update the variable.
Comment 84 Yusuke Suzuki 2019-04-25 12:26:19 PDT
Comment on attachment 367710 [details]
Patch

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

Thanks! I'll address them in a follow-up patch :D

>> Source/bmalloc/bmalloc/IsoAllocator.h:44
>> +
> 
> This isn't used.

Nice catch! Removed. This was originally here, and I moved it to IsoHeapImpl.

>> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:63
>> +        static_cast<IsoSharedPage*>(page)->free<Config>(handle, ptr);
> 
> nit: Maybe take const std::lock_guard<Mutext>& as a parameter to free to indicate lock must be held.

Fixed.

>> Source/bmalloc/bmalloc/IsoHeapImpl.h:63
>> +    std::chrono::steady_clock::time_point m_slowPathTimePoint;
> 
> nit: the name of this variable doesn't need the type name in it. Maybe something like "m_lastSlowPathTime" or "m_timeOfLastSlowPath"

Sounds nice.

>> Source/bmalloc/bmalloc/IsoHeapImpl.h:66
>> +    unsigned m_usableBits { maxAllocationFromSharedMask };
> 
> can we have some kind of static assert that this bitmap is large enough to hold maxAllocationFromShared? Maybe:
> static_assert(sizeof(m_usableBits) * 8 >= maxAllocationFromShared)
> 
> Also, I'm not a fan of this name. "Usable" is a bit vague. Maybe "m_availableShared" or "m_availableSharedIndices". Since this is only used for shared, I think there should be some description of "shared" here.

Sounds nice, fixed.

>> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
>> +    AllocationMode m_allocationMode { AllocationMode::Init };
> 
> This seems wrong, you never store to this variable besides turning it into Init?

Oops, right. I was doing update in the one previous patch https://bugs.webkit.org/attachment.cgi?id=367705&action=review and I accidentally dropped it with the refactoring.
I fix it.

>> Source/bmalloc/bmalloc/IsoHeapImpl.h:114
>> +    void* allocateFromShared(bool abortOnFailure);
> 
> nit: Maybe also take lock_guard ref here to show that this is called while lock is held.

Sounds good, fixed.

>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:259
>> +        // If we don't go to the allocation slow path during 1~ seconds, we think the allocation becomes quiescent state.
> 
> nit: 1~ => ~1

Fixed.

>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287
>> +    void* result = result = m_sharedCells[index];
> 
> Is this here a typo?
> 
> GCC 8 warns the following:
> ../../Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287:11: warning: operation on ‘result’ may be undefined [-Wsequence-point]
>      void* result = result = m_sharedCells[index];

Fixed.

>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:303
>> +    m_usableBits &= (~(1U << index));
> 
> style nit: You don't need the outer parens.

Fixed.

>> Source/bmalloc/bmalloc/IsoSharedConfig.h:32
>> +static constexpr unsigned alignmentForIsoSharedAllocation = 16;
> 
> is this really needed?

This is super sad but it is actually required (And I added this after I found this issue in EWS).
One of our IsoHeaped class DOMMatrixReadOnly / DOMMatrix / WebKitCSSMatrix has a field typed as TransformationMatrix. And TransformationMatrix requires 16byte alignment. And it is even strictly required in x64 since TransformationMatrix's member functions use this alignment requirement to emit aligned SSE intrinsics. So, if we remove that, we can see the crash in DOMMatrix tests because of unaligned SSE instructions.

It would be possible to change aligned-SSE-intrinsic to unaligned-SSE-intrinsic. First version of unaligned-SSE-intrinsics was bad, but it is now eventually becomes better and I doubt that there are measurable difference right now for our use case.
But maybe, our performance tests do not exercise DOMMatrix etc. well, so it would be a bit hard to measure, watch, and keep the performance.
Another possible idea is placing TransformationMatrix in 16byte aligned storage even if the parent class is 8byte aligned, like, adding a padding. I also have a prototyped patch for that https://bugs.webkit.org/show_bug.cgi?id=196959.

But in this patch, my focus is not changing TransformationMatrix mechanism. So, for now, I took the simplest approach here. Just using 16byte alignment.

>> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:50
>> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));
> 
> why cast to uintptr_t?

roundUpToMultipleOf only accepts uintptr_t right now. We can define 32 width version. But for now, I just reused it.
And we already ensured that passedObjectSize is less than 16KB by using `static_assert(numObjects, "IsoHeap size should allow at least one allocation per page");` in IsoPage.h.

>> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:76
>> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));
> 
> maybe have a function that does this since this is done in a couple places?

Fixed.

>> Source/bmalloc/test/testbmalloc.cpp:258
>> +static void testIsoMallocAndFreeFast()
> 
> I wonder why this works since you never update the variable.

Oops, I tested it with one previous version (and performance measurement is also done in the one previous version).
Comment 85 Yusuke Suzuki 2019-04-25 12:31:47 PDT
Comment on attachment 367710 [details]
Patch

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

Thanks! I'll address them in a follow-up patch :D

>>> Source/bmalloc/bmalloc/IsoAllocator.h:44
>>> +
>> 
>> This isn't used.
> 
> Nice catch! Removed. This was originally here, and I moved it to IsoHeapImpl.

Nice catch! Removed. This was originally here, and I moved it to IsoHeapImpl.

>>> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:63
>>> +        static_cast<IsoSharedPage*>(page)->free<Config>(handle, ptr);
>> 
>> nit: Maybe take const std::lock_guard<Mutext>& as a parameter to free to indicate lock must be held.
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:63
>>> +    std::chrono::steady_clock::time_point m_slowPathTimePoint;
>> 
>> nit: the name of this variable doesn't need the type name in it. Maybe something like "m_lastSlowPathTime" or "m_timeOfLastSlowPath"
> 
> Sounds nice.

Sounds nice.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:66
>>> +    unsigned m_usableBits { maxAllocationFromSharedMask };
>> 
>> can we have some kind of static assert that this bitmap is large enough to hold maxAllocationFromShared? Maybe:
>> static_assert(sizeof(m_usableBits) * 8 >= maxAllocationFromShared)
>> 
>> Also, I'm not a fan of this name. "Usable" is a bit vague. Maybe "m_availableShared" or "m_availableSharedIndices". Since this is only used for shared, I think there should be some description of "shared" here.
> 
> Sounds nice, fixed.

Sounds nice, fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
>>> +    AllocationMode m_allocationMode { AllocationMode::Init };
>> 
>> This seems wrong, you never store to this variable besides turning it into Init?
> 
> Oops, right. I was doing update in the one previous patch https://bugs.webkit.org/attachment.cgi?id=367705&action=review and I accidentally dropped it with the refactoring.
> I fix it.

Oops, right. I was doing update in the one previous patch https://bugs.webkit.org/attachment.cgi?id=367705&action=review and I accidentally dropped it with the refactoring.
I fix it.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:114
>>> +    void* allocateFromShared(bool abortOnFailure);
>> 
>> nit: Maybe also take lock_guard ref here to show that this is called while lock is held.
> 
> Sounds good, fixed.

Sounds good, fixed.

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:113
> +    m_numberOfAllocationsFromSharedInOneCycle = 0;
> +    m_allocationMode = AllocationMode::Init;

I also drop this part, since this update is not related to IsoHeapImpl's scavenge.

>>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:259
>>> +        // If we don't go to the allocation slow path during 1~ seconds, we think the allocation becomes quiescent state.
>> 
>> nit: 1~ => ~1
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287
>>> +    void* result = result = m_sharedCells[index];
>> 
>> Is this here a typo?
>> 
>> GCC 8 warns the following:
>> ../../Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287:11: warning: operation on ‘result’ may be undefined [-Wsequence-point]
>>      void* result = result = m_sharedCells[index];
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:303
>>> +    m_usableBits &= (~(1U << index));
>> 
>> style nit: You don't need the outer parens.
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoSharedConfig.h:32
>>> +static constexpr unsigned alignmentForIsoSharedAllocation = 16;
>> 
>> is this really needed?
> 
> This is super sad but it is actually required (And I added this after I found this issue in EWS).
> One of our IsoHeaped class DOMMatrixReadOnly / DOMMatrix / WebKitCSSMatrix has a field typed as TransformationMatrix. And TransformationMatrix requires 16byte alignment. And it is even strictly required in x64 since TransformationMatrix's member functions use this alignment requirement to emit aligned SSE intrinsics. So, if we remove that, we can see the crash in DOMMatrix tests because of unaligned SSE instructions.
> 
> It would be possible to change aligned-SSE-intrinsic to unaligned-SSE-intrinsic. First version of unaligned-SSE-intrinsics was bad, but it is now eventually becomes better and I doubt that there are measurable difference right now for our use case.
> But maybe, our performance tests do not exercise DOMMatrix etc. well, so it would be a bit hard to measure, watch, and keep the performance.
> Another possible idea is placing TransformationMatrix in 16byte aligned storage even if the parent class is 8byte aligned, like, adding a padding. I also have a prototyped patch for that https://bugs.webkit.org/show_bug.cgi?id=196959.
> 
> But in this patch, my focus is not changing TransformationMatrix mechanism. So, for now, I took the simplest approach here. Just using 16byte alignment.

This is super sad but it is actually required (And I added this after I found this issue in EWS).
One of our IsoHeaped class DOMMatrixReadOnly / DOMMatrix / WebKitCSSMatrix has a field typed as TransformationMatrix. And TransformationMatrix requires 16byte alignment. And it is even strictly required in x64 since TransformationMatrix's member functions use this alignment requirement to emit aligned SSE intrinsics. So, if we remove that, we can see the crash in DOMMatrix tests because of unaligned SSE instructions.

It would be possible to change aligned-SSE-intrinsic to unaligned-SSE-intrinsic. First version of unaligned-SSE-intrinsics was bad, but it is now eventually becomes better and I doubt that there are measurable difference right now for our use case.
But maybe, our performance tests do not exercise DOMMatrix etc. well, so it would be a bit hard to measure, watch, and keep the performance.
Another possible idea is placing TransformationMatrix in 16byte aligned storage even if the parent class is 8byte aligned, like, adding a padding. I also have a prototyped patch for that https://bugs.webkit.org/show_bug.cgi?id=196959.

But in this patch, my focus is not changing TransformationMatrix mechanism. So, for now, I took the simplest approach here. Just using 16byte alignment.

>>> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:50
>>> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));
>> 
>> why cast to uintptr_t?
> 
> roundUpToMultipleOf only accepts uintptr_t right now. We can define 32 width version. But for now, I just reused it.
> And we already ensured that passedObjectSize is less than 16KB by using `static_assert(numObjects, "IsoHeap size should allow at least one allocation per page");` in IsoPage.h.

roundUpToMultipleOf only accepts uintptr_t right now. We can define 32 width version. But for now, I just reused it.
And we already ensured that passedObjectSize is less than 16KB by using `static_assert(numObjects, "IsoHeap size should allow at least one allocation per page");` in IsoPage.h.

>>> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:76
>>> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));
>> 
>> maybe have a function that does this since this is done in a couple places?
> 
> Fixed.

Fixed.

>>> Source/bmalloc/test/testbmalloc.cpp:258
>>> +static void testIsoMallocAndFreeFast()
>> 
>> I wonder why this works since you never update the variable.
> 
> Oops, I tested it with one previous version (and performance measurement is also done in the one previous version).

Oops, I tested it with one previous version (and performance measurement is also done in the one previous version).
Comment 86 Yusuke Suzuki 2019-04-25 12:31:58 PDT
Comment on attachment 367710 [details]
Patch

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

Thanks! I'll address them in a follow-up patch :D

>>> Source/bmalloc/bmalloc/IsoAllocator.h:44
>>> +
>> 
>> This isn't used.
> 
> Nice catch! Removed. This was originally here, and I moved it to IsoHeapImpl.

Nice catch! Removed. This was originally here, and I moved it to IsoHeapImpl.

>>> Source/bmalloc/bmalloc/IsoDeallocatorInlines.h:63
>>> +        static_cast<IsoSharedPage*>(page)->free<Config>(handle, ptr);
>> 
>> nit: Maybe take const std::lock_guard<Mutext>& as a parameter to free to indicate lock must be held.
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:63
>>> +    std::chrono::steady_clock::time_point m_slowPathTimePoint;
>> 
>> nit: the name of this variable doesn't need the type name in it. Maybe something like "m_lastSlowPathTime" or "m_timeOfLastSlowPath"
> 
> Sounds nice.

Sounds nice.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:66
>>> +    unsigned m_usableBits { maxAllocationFromSharedMask };
>> 
>> can we have some kind of static assert that this bitmap is large enough to hold maxAllocationFromShared? Maybe:
>> static_assert(sizeof(m_usableBits) * 8 >= maxAllocationFromShared)
>> 
>> Also, I'm not a fan of this name. "Usable" is a bit vague. Maybe "m_availableShared" or "m_availableSharedIndices". Since this is only used for shared, I think there should be some description of "shared" here.
> 
> Sounds nice, fixed.

Sounds nice, fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
>>> +    AllocationMode m_allocationMode { AllocationMode::Init };
>> 
>> This seems wrong, you never store to this variable besides turning it into Init?
> 
> Oops, right. I was doing update in the one previous patch https://bugs.webkit.org/attachment.cgi?id=367705&action=review and I accidentally dropped it with the refactoring.
> I fix it.

Oops, right. I was doing update in the one previous patch https://bugs.webkit.org/attachment.cgi?id=367705&action=review and I accidentally dropped it with the refactoring.
I fix it.

>>> Source/bmalloc/bmalloc/IsoHeapImpl.h:114
>>> +    void* allocateFromShared(bool abortOnFailure);
>> 
>> nit: Maybe also take lock_guard ref here to show that this is called while lock is held.
> 
> Sounds good, fixed.

Sounds good, fixed.

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:113
> +    m_numberOfAllocationsFromSharedInOneCycle = 0;
> +    m_allocationMode = AllocationMode::Init;

I also drop this part, since this update is not related to IsoHeapImpl's scavenge.

>>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:259
>>> +        // If we don't go to the allocation slow path during 1~ seconds, we think the allocation becomes quiescent state.
>> 
>> nit: 1~ => ~1
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287
>>> +    void* result = result = m_sharedCells[index];
>> 
>> Is this here a typo?
>> 
>> GCC 8 warns the following:
>> ../../Source/bmalloc/bmalloc/IsoHeapImplInlines.h:287:11: warning: operation on ‘result’ may be undefined [-Wsequence-point]
>>      void* result = result = m_sharedCells[index];
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:303
>>> +    m_usableBits &= (~(1U << index));
>> 
>> style nit: You don't need the outer parens.
> 
> Fixed.

Fixed.

>>> Source/bmalloc/bmalloc/IsoSharedConfig.h:32
>>> +static constexpr unsigned alignmentForIsoSharedAllocation = 16;
>> 
>> is this really needed?
> 
> This is super sad but it is actually required (And I added this after I found this issue in EWS).
> One of our IsoHeaped class DOMMatrixReadOnly / DOMMatrix / WebKitCSSMatrix has a field typed as TransformationMatrix. And TransformationMatrix requires 16byte alignment. And it is even strictly required in x64 since TransformationMatrix's member functions use this alignment requirement to emit aligned SSE intrinsics. So, if we remove that, we can see the crash in DOMMatrix tests because of unaligned SSE instructions.
> 
> It would be possible to change aligned-SSE-intrinsic to unaligned-SSE-intrinsic. First version of unaligned-SSE-intrinsics was bad, but it is now eventually becomes better and I doubt that there are measurable difference right now for our use case.
> But maybe, our performance tests do not exercise DOMMatrix etc. well, so it would be a bit hard to measure, watch, and keep the performance.
> Another possible idea is placing TransformationMatrix in 16byte aligned storage even if the parent class is 8byte aligned, like, adding a padding. I also have a prototyped patch for that https://bugs.webkit.org/show_bug.cgi?id=196959.
> 
> But in this patch, my focus is not changing TransformationMatrix mechanism. So, for now, I took the simplest approach here. Just using 16byte alignment.

This is super sad but it is actually required (And I added this after I found this issue in EWS).
One of our IsoHeaped class DOMMatrixReadOnly / DOMMatrix / WebKitCSSMatrix has a field typed as TransformationMatrix. And TransformationMatrix requires 16byte alignment. And it is even strictly required in x64 since TransformationMatrix's member functions use this alignment requirement to emit aligned SSE intrinsics. So, if we remove that, we can see the crash in DOMMatrix tests because of unaligned SSE instructions.

It would be possible to change aligned-SSE-intrinsic to unaligned-SSE-intrinsic. First version of unaligned-SSE-intrinsics was bad, but it is now eventually becomes better and I doubt that there are measurable difference right now for our use case.
But maybe, our performance tests do not exercise DOMMatrix etc. well, so it would be a bit hard to measure, watch, and keep the performance.
Another possible idea is placing TransformationMatrix in 16byte aligned storage even if the parent class is 8byte aligned, like, adding a padding. I also have a prototyped patch for that https://bugs.webkit.org/show_bug.cgi?id=196959.

But in this patch, my focus is not changing TransformationMatrix mechanism. So, for now, I took the simplest approach here. Just using 16byte alignment.

>>> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:50
>>> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));
>> 
>> why cast to uintptr_t?
> 
> roundUpToMultipleOf only accepts uintptr_t right now. We can define 32 width version. But for now, I just reused it.
> And we already ensured that passedObjectSize is less than 16KB by using `static_assert(numObjects, "IsoHeap size should allow at least one allocation per page");` in IsoPage.h.

roundUpToMultipleOf only accepts uintptr_t right now. We can define 32 width version. But for now, I just reused it.
And we already ensured that passedObjectSize is less than 16KB by using `static_assert(numObjects, "IsoHeap size should allow at least one allocation per page");` in IsoPage.h.

>>> Source/bmalloc/bmalloc/IsoSharedHeapInlines.h:76
>>> +    constexpr unsigned objectSize = roundUpToMultipleOf<alignmentForIsoSharedAllocation>(static_cast<uintptr_t>(passedObjectSize));
>> 
>> maybe have a function that does this since this is done in a couple places?
> 
> Fixed.

Fixed.

>>> Source/bmalloc/test/testbmalloc.cpp:258
>>> +static void testIsoMallocAndFreeFast()
>> 
>> I wonder why this works since you never update the variable.
> 
> Oops, I tested it with one previous version (and performance measurement is also done in the one previous version).

Oops, I tested it with one previous version (and performance measurement is also done in the one previous version).