Bug 196837 – [bmalloc] IsoHeap should have lower tier using shared IsoPage

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 367261 [details]
Patch

WIP. Still investigating and trying things

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367276 [details]
Patch

WIP

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367277 [details]
Patch

WIP

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367290 [details]
Patch

WIP

[Yusuke Suzuki]

Maybe, bump only thing is enough for this. This simplifies thing a lot.

[EWS Watchlist]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 367295 [details]
Patch

WIP

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367297 [details]
Patch

WIP

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 367305 [details]
Patch

WIP

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 367370 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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

[Yusuke Suzuki]

Created attachment 367574 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367583 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367588 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367603 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367605 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367606 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367607 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 367616 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367665 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367705 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 367710 [details]
Patch

[EWS Watchlist]

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.

[Saam Barati]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r244481: <https://trac.webkit.org/changeset/244481>

[Radar WebKit Bug Importer]

<rdar://problem/50067525>

[Zan Dobersek]

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];

[Philippe Normand]

Looks like a typo indeed.

[Yusuke Suzuki]

(In reply to Philippe Normand from comment #79)
> Looks like a typo indeed.

Yes, this is typo. Thanks, I'll fix it soon.

[Yusuke Suzuki]

Committed r244634: <https://trac.webkit.org/changeset/244634>

[Saam Barati]

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?

[Saam Barati]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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