Bug 182563 - Experiment with alternative implementation of memcpy/memset
Summary: Experiment with alternative implementation of memcpy/memset
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-06 20:04 PST by Filip Pizlo
Modified: 2018-02-16 14:06 PST (History)
14 users (show)

See Also:


Attachments
work in progress (54.06 KB, patch)
2018-02-06 20:05 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (66.31 KB, patch)
2018-02-08 10:08 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (68.98 KB, patch)
2018-02-08 10:20 PST, Filip Pizlo
msaboff: review+
Details | Formatted Diff | Diff
patch for landing (68.72 KB, patch)
2018-02-08 12:57 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-02-06 20:04:17 PST
...
Comment 1 Filip Pizlo 2018-02-06 20:05:22 PST
Created attachment 333260 [details]
work in progress
Comment 2 Filip Pizlo 2018-02-07 15:50:28 PST
Looks like a 1.00882x speed-up on PLT with p = 0.0973514.

Gonna test some more benchmarks.
Comment 3 Filip Pizlo 2018-02-08 10:08:42 PST
Created attachment 333383 [details]
the patch
Comment 4 EWS Watchlist 2018-02-08 10:11:46 PST
Attachment 333383 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Algorithm.h:190:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:203:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:210:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:231:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:238:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:259:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:265:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:301:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:313:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:314:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:314:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:320:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:339:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:340:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:340:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:346:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:365:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:366:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:366:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:371:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:51:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:77:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:78:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:78:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:103:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:104:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:104:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:109:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:80:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:108:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 108 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Filip Pizlo 2018-02-08 10:20:15 PST
Created attachment 333386 [details]
the patch
Comment 6 EWS Watchlist 2018-02-08 10:23:07 PST
Attachment 333386 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Algorithm.h:190:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:203:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:210:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:231:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:238:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:259:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:265:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:301:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:313:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:314:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:314:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:320:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:339:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:340:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:340:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:346:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:365:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:366:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:366:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:371:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:51:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:77:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:78:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:78:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:103:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:104:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:104:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:109:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:80:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:108:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 108 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Michael Saboff 2018-02-08 11:21:32 PST
Comment on attachment 333386 [details]
the patch

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

r=me.  Did you consider implementing only in bmalloc and then a forward reference in WTF?

> Source/WTF/ChangeLog:36
> +        reasoning. But that's not quite right, since then we would lose inlining in the unknonw-size

Spelling *unknonw-size*
Comment 8 Mark Lam 2018-02-08 12:20:30 PST
Comment on attachment 333386 [details]
the patch

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

FYI, I did my review from the bottom up (since the fastCopy and fastZeroFill implementations are located towards the bottom).  It might make more sense to read my comments backwards.

> Source/WTF/wtf/FastCopy.h:33
> +template<typename T>
> +void fastCopy(T* dst, const T* src, size_t length)

I assume this is identical to the bmalloc version below and didn't review this thoroughly.

> Source/WTF/wtf/FastCopy.h:143
> +inline void fastCopyBytes(void* dst, const void* src, size_t bytes)

At first, I was pondering whether we can rename this to just be fastCopy.  After doing the entire review, I am strongly of the opinion that we should NOT rename this to fastCopy.  There's a strong need to distinguish fastCopy and fastCopyBytes apart.

One of the class of errors that can arise from fastCopy() taking a count of T to copy instead of size_t bytes is that if someone edits the code later and innocently changes the type of the pointer, an error may silently creep in.  Consider this scenario:

Initially, let's say we have this contrived code in a class:

       void foo(Stuff* myStuff, Stuff* other, int numberOfStuff) {
           m_myStuff = myStuff;
           m_other = other;
           m_length = numberOfStuff;
           bar(); // where bar contains ... fastCopy(m_myStuff, m_other, m_length);  // <======== this is correct.
       }

       Stuff m_myStuff;
       Stuff m_other;
       size_t m_length;

Some time later, for reasons, someone decides to change type of the pointers like so:

       void* m_myStuff;
       void* m_other;
       size_t m_length;

The person making the change may not notice that bar() relies on typeof m_myStuff and m_other not being void*.  If you can be sure that calling fastCopy() with void* pointers will result in a build error, then this error will not go unnoticed.  I think we're fine, but can you please do a quick test to make sure that this is indeed the case i.e. we should get build errors if fastCopy() is called with void* pointers.

This is also why it is good to name fastCopyBytes() differently than fastCopy().

> Source/WTF/wtf/FastMalloc.cpp:83
> +    fastZeroFill(static_cast<char*>(result), n);

Let's just call fastZeroFillBytes() and do away with the cast instead.

> Source/WTF/wtf/FastMalloc.cpp:100
> +    fastZeroFill(static_cast<char*>(result), n);

Let's just call fastZeroFillBytes() and do away with the cast instead.

> Source/WTF/wtf/FastZeroFill.h:33
> +void fastZeroFill(T* dst, size_t length)

I assume this is identical to the bmalloc version below and didn't review this thoroughly.

> Source/WTF/wtf/FastZeroFill.h:136
> +inline void fastZeroFillBytes(void* dst, size_t bytes)

Ditto: same issue as with fastCopyBytes() above.

> Source/WTF/wtf/MD5.cpp:5
> - * Copyright (C) 2015 Apple Inc. All rights reserved.
> + * Copyright (C) 2015-2018 Apple Inc. All rights reserved.

Why is this needed?

> Source/WTF/wtf/text/StringConcatenate.h:161
> +        fastCopy(destination, m_characters, m_length * sizeof(UChar));

This is wrong.  You should remove the "* sizeof(UChar)".

> Source/bmalloc/bmalloc/Algorithm.h:218
> +            "movq (%%rsi, %%rcx), %%rax\n\t"
> +            "movq %%rax, (%%rdi, %%rcx)\n\t"

Ditto with assumption about alignment being a non-issue (see below).

> Source/bmalloc/bmalloc/Algorithm.h:229
> +            "addq $4, %%rcx\n\t"
> +            "4:\n\t"
> +            "movl (%%rsi, %%rcx), %%eax\n\t"
> +            "movl %%eax, (%%rdi, %%rcx)\n\t"
> +            "subq $4, %%rcx\n\t"
> +            "jae 4b\n\t"

Same as below: no need for addq, subq, and jae.  This reduces to:
    "movl (%%rsi), %%eax\n\t"
    "movl %%eax, (%%rdi)\n\t"

> Source/bmalloc/bmalloc/Algorithm.h:246
> +            "movq (%%rsi, %%rcx), %%rax\n\t"
> +            "movq %%rax, (%%rdi, %%rcx)\n\t"

Ditto with assumption about alignment being a non-issue (see below).

> Source/bmalloc/bmalloc/Algorithm.h:272
> +        "movq (%%rsi, %%rcx), %%rax\n\t"
> +        "movq %%rax, (%%rdi, %%rcx)\n\t"

Ditto with assumption about alignment being a non-issue (see below).

> Source/bmalloc/bmalloc/Algorithm.h:324
> +            "rep stosl\n\t"

I presume the CPU's implementation of stosl will do the equivalent of stosq for the parts that are 8 byte aligned?  Can you confirm?

> Source/bmalloc/bmalloc/Algorithm.h:330
> +            "1:\n\t"
> +            "subq $8, %%rcx\n\t"
> +            "jae 3b\n\t"

It looks like you're assuming that fastZeroFill() will only every be called on a dst that is aligned on a 8 byte boundary because you're not checking for the potential of a leading 4 bytes here.  Or are you relying on x86 support for unaligned access and just taking the hit if it turns out to be unaligned?

> Source/bmalloc/bmalloc/Algorithm.h:337
> +            "addq $4, %%rcx\n\t"
> +            "4:\n\t"
> +            "movl %%eax, (%%rdi, %%rcx)\n\t"
> +            "subq $4, %%rcx\n\t"
> +            "jae 4b\n\t"

We already know that sizeof(T) % sizeof(uint32_t) == 0.  Hence, the only way that we can get here (%rcx != -8) is if there's one 4 byte word to write.  I think you can remove this addq, subq, and jae.  This reduces to:
    "movl %%eax, (%%rdi)\n\t"

> Source/bmalloc/bmalloc/Algorithm.h:353
> +            "movq %%rax, (%%rdi, %%rcx)\n\t"

Ditto with assumption about alignment being a non-issue.

> Source/bmalloc/bmalloc/Algorithm.h:377
> +        "movq %%rax, (%%rdi, %%rcx)\n\t"

Ditto with assumption about alignment being a non-issue.
Comment 9 Filip Pizlo 2018-02-08 12:49:50 PST
(In reply to Mark Lam from comment #8)
> Comment on attachment 333386 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333386&action=review
> 
> FYI, I did my review from the bottom up (since the fastCopy and fastZeroFill
> implementations are located towards the bottom).  It might make more sense
> to read my comments backwards.
> 
> > Source/WTF/wtf/FastCopy.h:33
> > +template<typename T>
> > +void fastCopy(T* dst, const T* src, size_t length)
> 
> I assume this is identical to the bmalloc version below and didn't review
> this thoroughly.
> 
> > Source/WTF/wtf/FastCopy.h:143
> > +inline void fastCopyBytes(void* dst, const void* src, size_t bytes)
> 
> At first, I was pondering whether we can rename this to just be fastCopy. 
> After doing the entire review, I am strongly of the opinion that we should
> NOT rename this to fastCopy.  There's a strong need to distinguish fastCopy
> and fastCopyBytes apart.
> 
> One of the class of errors that can arise from fastCopy() taking a count of
> T to copy instead of size_t bytes is that if someone edits the code later
> and innocently changes the type of the pointer, an error may silently creep
> in.  Consider this scenario:
> 
> Initially, let's say we have this contrived code in a class:
> 
>        void foo(Stuff* myStuff, Stuff* other, int numberOfStuff) {
>            m_myStuff = myStuff;
>            m_other = other;
>            m_length = numberOfStuff;
>            bar(); // where bar contains ... fastCopy(m_myStuff, m_other,
> m_length);  // <======== this is correct.
>        }
> 
>        Stuff m_myStuff;
>        Stuff m_other;
>        size_t m_length;
> 
> Some time later, for reasons, someone decides to change type of the pointers
> like so:
> 
>        void* m_myStuff;
>        void* m_other;
>        size_t m_length;
> 
> The person making the change may not notice that bar() relies on typeof
> m_myStuff and m_other not being void*.  If you can be sure that calling
> fastCopy() with void* pointers will result in a build error, then this error
> will not go unnoticed.  I think we're fine, but can you please do a quick
> test to make sure that this is indeed the case i.e. we should get build
> errors if fastCopy() is called with void* pointers.

We do not get build errors if fastCopy() is called with void*.

> 
> This is also why it is good to name fastCopyBytes() differently than
> fastCopy().
> 
> > Source/WTF/wtf/FastMalloc.cpp:83
> > +    fastZeroFill(static_cast<char*>(result), n);
> 
> Let's just call fastZeroFillBytes() and do away with the cast instead.

Fixed.

> 
> > Source/WTF/wtf/FastMalloc.cpp:100
> > +    fastZeroFill(static_cast<char*>(result), n);
> 
> Let's just call fastZeroFillBytes() and do away with the cast instead.

Fixed.

> 
> > Source/WTF/wtf/FastZeroFill.h:33
> > +void fastZeroFill(T* dst, size_t length)
> 
> I assume this is identical to the bmalloc version below and didn't review
> this thoroughly.

Yeah.

> 
> > Source/WTF/wtf/FastZeroFill.h:136
> > +inline void fastZeroFillBytes(void* dst, size_t bytes)
> 
> Ditto: same issue as with fastCopyBytes() above.

Yeah.

> 
> > Source/WTF/wtf/MD5.cpp:5
> > - * Copyright (C) 2015 Apple Inc. All rights reserved.
> > + * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
> 
> Why is this needed?

Reverted.

An earlier version replaced memcpy/memset in this file, but later I noticed it was being used to copy constant-size things, so I reverted.

> 
> > Source/WTF/wtf/text/StringConcatenate.h:161
> > +        fastCopy(destination, m_characters, m_length * sizeof(UChar));
> 
> This is wrong.  You should remove the "* sizeof(UChar)".

Fixed!

> 
> > Source/bmalloc/bmalloc/Algorithm.h:218
> > +            "movq (%%rsi, %%rcx), %%rax\n\t"
> > +            "movq %%rax, (%%rdi, %%rcx)\n\t"
> 
> Ditto with assumption about alignment being a non-issue (see below).

I confirmed that alignment is a non-issue!

It appears that doing a misaligned 64-bit word load/store is always faster than doing 8 8-bit loads/stores.

Also, other memcpy's that I've seen for x86 do the same thing.

> 
> > Source/bmalloc/bmalloc/Algorithm.h:229
> > +            "addq $4, %%rcx\n\t"
> > +            "4:\n\t"
> > +            "movl (%%rsi, %%rcx), %%eax\n\t"
> > +            "movl %%eax, (%%rdi, %%rcx)\n\t"
> > +            "subq $4, %%rcx\n\t"
> > +            "jae 4b\n\t"
> 
> Same as below: no need for addq, subq, and jae.  This reduces to:
>     "movl (%%rsi), %%eax\n\t"
>     "movl %%eax, (%%rdi)\n\t"

Clever.  I'll leave a comment and fix in another patch: https://bugs.webkit.org/show_bug.cgi?id=182617

> 
> > Source/bmalloc/bmalloc/Algorithm.h:246
> > +            "movq (%%rsi, %%rcx), %%rax\n\t"
> > +            "movq %%rax, (%%rdi, %%rcx)\n\t"
> 
> Ditto with assumption about alignment being a non-issue (see below).

Will leave fixme.

> 
> > Source/bmalloc/bmalloc/Algorithm.h:272
> > +        "movq (%%rsi, %%rcx), %%rax\n\t"
> > +        "movq %%rax, (%%rdi, %%rcx)\n\t"
> 
> Ditto with assumption about alignment being a non-issue (see below).

Will leave fixme.

> 
> > Source/bmalloc/bmalloc/Algorithm.h:324
> > +            "rep stosl\n\t"
> 
> I presume the CPU's implementation of stosl will do the equivalent of stosq
> for the parts that are 8 byte aligned?  Can you confirm?
> 
> > Source/bmalloc/bmalloc/Algorithm.h:330
> > +            "1:\n\t"
> > +            "subq $8, %%rcx\n\t"
> > +            "jae 3b\n\t"
> 
> It looks like you're assuming that fastZeroFill() will only every be called
> on a dst that is aligned on a 8 byte boundary because you're not checking
> for the potential of a leading 4 bytes here.  Or are you relying on x86
> support for unaligned access and just taking the hit if it turns out to be
> unaligned?

Relying on x86 to be awesome. ;-)

> 
> > Source/bmalloc/bmalloc/Algorithm.h:337
> > +            "addq $4, %%rcx\n\t"
> > +            "4:\n\t"
> > +            "movl %%eax, (%%rdi, %%rcx)\n\t"
> > +            "subq $4, %%rcx\n\t"
> > +            "jae 4b\n\t"
> 
> We already know that sizeof(T) % sizeof(uint32_t) == 0.  Hence, the only way
> that we can get here (%rcx != -8) is if there's one 4 byte word to write.  I
> think you can remove this addq, subq, and jae.  This reduces to:
>     "movl %%eax, (%%rdi)\n\t"

That's a really good observation. https://bugs.webkit.org/show_bug.cgi?id=182617

> 
> > Source/bmalloc/bmalloc/Algorithm.h:353
> > +            "movq %%rax, (%%rdi, %%rcx)\n\t"
> 
> Ditto with assumption about alignment being a non-issue.

Not an issue.

> 
> > Source/bmalloc/bmalloc/Algorithm.h:377
> > +        "movq %%rax, (%%rdi, %%rcx)\n\t"
> 
> Ditto with assumption about alignment being a non-issue.

Not an issue.
Comment 10 Filip Pizlo 2018-02-08 12:52:00 PST
(In reply to Filip Pizlo from comment #9)
> (In reply to Mark Lam from comment #8)
> > Comment on attachment 333386 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333386&action=review
> > 
> > FYI, I did my review from the bottom up (since the fastCopy and fastZeroFill
> > implementations are located towards the bottom).  It might make more sense
> > to read my comments backwards.
> > 
> > > Source/WTF/wtf/FastCopy.h:33
> > > +template<typename T>
> > > +void fastCopy(T* dst, const T* src, size_t length)
> > 
> > I assume this is identical to the bmalloc version below and didn't review
> > this thoroughly.
> > 
> > > Source/WTF/wtf/FastCopy.h:143
> > > +inline void fastCopyBytes(void* dst, const void* src, size_t bytes)
> > 
> > At first, I was pondering whether we can rename this to just be fastCopy. 
> > After doing the entire review, I am strongly of the opinion that we should
> > NOT rename this to fastCopy.  There's a strong need to distinguish fastCopy
> > and fastCopyBytes apart.
> > 
> > One of the class of errors that can arise from fastCopy() taking a count of
> > T to copy instead of size_t bytes is that if someone edits the code later
> > and innocently changes the type of the pointer, an error may silently creep
> > in.  Consider this scenario:
> > 
> > Initially, let's say we have this contrived code in a class:
> > 
> >        void foo(Stuff* myStuff, Stuff* other, int numberOfStuff) {
> >            m_myStuff = myStuff;
> >            m_other = other;
> >            m_length = numberOfStuff;
> >            bar(); // where bar contains ... fastCopy(m_myStuff, m_other,
> > m_length);  // <======== this is correct.
> >        }
> > 
> >        Stuff m_myStuff;
> >        Stuff m_other;
> >        size_t m_length;
> > 
> > Some time later, for reasons, someone decides to change type of the pointers
> > like so:
> > 
> >        void* m_myStuff;
> >        void* m_other;
> >        size_t m_length;
> > 
> > The person making the change may not notice that bar() relies on typeof
> > m_myStuff and m_other not being void*.  If you can be sure that calling
> > fastCopy() with void* pointers will result in a build error, then this error
> > will not go unnoticed.  I think we're fine, but can you please do a quick
> > test to make sure that this is indeed the case i.e. we should get build
> > errors if fastCopy() is called with void* pointers.
> 
> We do not get build errors if fastCopy() is called with void*.

I was wrong.  We totally do, because of sizeof(void).
Comment 11 Filip Pizlo 2018-02-08 12:57:04 PST
Created attachment 333413 [details]
patch for landing
Comment 12 EWS Watchlist 2018-02-08 12:59:56 PST
Attachment 333413 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Algorithm.h:190:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:203:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:210:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:231:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:238:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:259:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:265:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:285:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:290:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:295:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:301:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:313:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:314:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:314:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:320:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:339:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:340:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:340:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:346:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:365:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:366:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:366:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:371:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:389:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:390:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/bmalloc/bmalloc/Algorithm.h:394:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:51:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:77:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:78:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:78:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:103:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:104:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:104:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:109:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:132:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastZeroFill.h:136:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:52:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:80:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:87:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:108:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/FastCopy.h:134:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:139:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/FastCopy.h:143:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 108 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Mark Lam 2018-02-08 13:01:44 PST
(In reply to Filip Pizlo from comment #10)
> (In reply to Filip Pizlo from comment #9)
> > We do not get build errors if fastCopy() is called with void*.
> 
> I was wrong.  We totally do, because of sizeof(void).

Excellent.
Comment 14 Mark Lam 2018-02-08 13:04:52 PST
Comment on attachment 333413 [details]
patch for landing

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

> Source/WTF/wtf/FastCopy.h:101
> +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617

Wrong place.  This comment belongs on the uint32_t case above.

> Source/WTF/wtf/FastZeroFill.h:97
> +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617

Wrong place.  This comment belongs on the uint32_t case above.

> Source/bmalloc/bmalloc/Algorithm.h:252
> +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617

Wrong place.  This comment belongs on the uint32_t case above.

> Source/bmalloc/bmalloc/Algorithm.h:359
> +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617

Wrong place.  This comment belongs on the uint32_t case above.
Comment 15 Filip Pizlo 2018-02-08 18:11:45 PST
(In reply to Mark Lam from comment #14)
> Comment on attachment 333413 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333413&action=review
> 
> > Source/WTF/wtf/FastCopy.h:101
> > +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617
> 
> Wrong place.  This comment belongs on the uint32_t case above.
> 
> > Source/WTF/wtf/FastZeroFill.h:97
> > +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617
> 
> Wrong place.  This comment belongs on the uint32_t case above.
> 
> > Source/bmalloc/bmalloc/Algorithm.h:252
> > +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617
> 
> Wrong place.  This comment belongs on the uint32_t case above.
> 
> > Source/bmalloc/bmalloc/Algorithm.h:359
> > +            "addq $6, %%rcx\n\t" // FIXME: This isn't really a loop. https://bugs.webkit.org/show_bug.cgi?id=182617
> 
> Wrong place.  This comment belongs on the uint32_t case above.

Fixed!
Comment 16 Filip Pizlo 2018-02-08 18:13:45 PST
Landed in http://trac.webkit.org/changeset/228306/webkit

Saam and I will watch the bots to see if this is really a speed-up.  It's hard to tell.  When I retested PLT, I didn't see anything.  But a Speedometer run did see a maybe-speed-up.  So who knows!
Comment 17 Filip Pizlo 2018-02-08 18:14:01 PST
Landed in http://trac.webkit.org/changeset/228306/webkit
Comment 18 Radar WebKit Bug Importer 2018-02-08 18:14:27 PST
<rdar://problem/37374970>
Comment 20 Carlos Alberto Lopez Perez 2018-02-09 05:37:49 PST
Committed r228317: <https://trac.webkit.org/changeset/228317>
Comment 21 Carlos Alberto Lopez Perez 2018-02-09 05:50:47 PST
Committed r228318: <https://trac.webkit.org/changeset/228318>
Comment 22 Michael Catanzaro 2018-02-09 08:07:32 PST
(In reply to Carlos Alberto Lopez Perez from comment #20)
> Committed r228317: <https://trac.webkit.org/changeset/228317>

We should avoid architecture-specific #includes whenever possible. Adding one #include <string.h> is not going to slow down the build very much at all; it's not worth the cost of the include guards, IMO.
Comment 23 Carlos Alberto Lopez Perez 2018-02-09 09:04:31 PST
(In reply to Michael Catanzaro from comment #22)
> (In reply to Carlos Alberto Lopez Perez from comment #20)
> > Committed r228317: <https://trac.webkit.org/changeset/228317>
> 
> We should avoid architecture-specific #includes whenever possible. Adding
> one #include <string.h> is not going to slow down the build very much at
> all; it's not worth the cost of the include guards, IMO.

Thanks for the suggestion. I have uploaded a patch for this to bug 182642
Comment 24 Matt Lewis 2018-02-16 14:06:37 PST
Reverted r228318 for reason:

The patch that this attempted to fix was rolled out already.

Committed r228582: <https://trac.webkit.org/changeset/228582>