Bug 199650

Summary: Keyword lookup can use memcmp to get around unaligned load undefined behavior
Product: WebKit Reporter: Saagar Jha <saagarjha>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: REOPENED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, king_pirate11, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Mac   
OS: Other   
Bug Depends on: 201515    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Saagar Jha
Reported 2019-07-09 18:22:00 PDT
KeywordLookup.h rolls what appears to be its own custom memcmp to do string matches, but it performs unaligned loads, reinterpret_casts, and in general makes UBSan very unhappy. I think we can use just use memcmp instead (someone should check whether I can do this, though: I may be misunderstanding how the character encoding works). I have checked that Clang and GCC produce reasonable-looking assembly for this if we make what we're doing clear enough.
Attachments
Patch (9.45 KB, patch)
2019-07-09 18:34 PDT, Saagar Jha 		no flags
DetailsFormatted DiffDiff
Patch (9.37 KB, patch)
2019-07-09 18:42 PDT, Saagar Jha 		no flags
DetailsFormatted DiffDiff
Patch (9.09 KB, patch)
2019-07-15 15:38 PDT, Saagar Jha 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Saagar Jha
Comment 1 2019-07-09 18:34:56 PDT
Created attachment 373805 [details] Patch
Saagar Jha
Comment 2 2019-07-09 18:42:11 PDT
Created attachment 373806 [details] Patch
Saagar Jha
Comment 3 2019-07-12 09:21:22 PDT
Here's the results of running JetStream2 on my computer; I'm not seeing a huge performance difference (as expected: generated code is pretty much functionally identical until we go past 16-byte comparisons, at which point the memcmp method starts emitting SSE vectorized instructions). It's possible that there's some difference that's statistically significant in here, but I didn't find any… |-------------------------------------|------------------------------------| | Old | New | |-------------------------------------| -----------------------------------| | Starting JetStream2 | Starting JetStream2 | | Running WSL: | Running WSL: | | Stdlib: 0.898 | Stdlib: 0.918 | | Tests: 0.332 | Tests: 0.338 | | Score: 0.546 | Score: 0.557 | | Wall time: 0:20.659 | Wall time: 0:20.270 | | Running UniPoker: | Running UniPoker: | | Startup: 106.383 | Startup: 128.205 | | Worst Case: 162.602 | Worst Case: 165.289 | | Average: 223.265 | Average: 240.307 | | Score: 156.894 | Score: 172.044 | | Wall time: 0:02.716 | Wall time: 0:02.518 | | Running uglify-js-wtb: | Running uglify-js-wtb: | | Startup: 6.766 | Startup: 6.460 | | Worst Case: 11.494 | Worst Case: 12.658 | | Average: 12.415 | Average: 13.459 | | Score: 9.884 | Score: 10.325 | | Wall time: 0:03.334 | Wall time: 0:03.085 | | Running typescript: | Running typescript: | | Startup: 5.107 | Startup: 5.285 | | Worst Case: 8.826 | Worst Case: 9.217 | | Average: 10.779 | Average: 11.182 | | Score: 7.862 | Score: 8.167 | | Wall time: 0:07.512 | Wall time: 0:07.245 | | Running tsf-wasm: | Running tsf-wasm: | | Startup: 73.529 | Startup: 79.365 | | Run time: 1.652 | Run time: 1.653 | | Score: 11.021 | Score: 11.455 | | Running tagcloud-SP: | Running tagcloud-SP: | | Startup: 131.579 | Startup: 135.135 | | Worst Case: 155.039 | Worst Case: 138.889 | | Average: 177.294 | Average: 179.541 | | Score: 153.500 | Score: 149.922 | | Wall time: 0:03.396 | Wall time: 0:03.353 | | Running string-unpack-code-SP: | Running string-unpack-code-SP: | | Startup: 192.308 | Startup: 200 | | Worst Case: 210.526 | Worst Case: 140.845 | | Average: 232.422 | Average: 228.846 | | Score: 211.119 | Score: 186.111 | | Wall time: 0:02.589 | Wall time: 0:02.626 | | Running stanford-crypto-sha256: | Running stanford-crypto-sha256: | | Startup: 200 | Startup: 238.095 | | Worst Case: 350.877 | Worst Case: 333.333 | | Average: 403.390 | Average: 401.214 | | Score: 304.768 | Score: 316.958 | | Wall time: 0:01.503 | Wall time: 0:01.507 | | Running stanford-crypto-pbkdf2: | Running stanford-crypto-pbkdf2: | | Startup: 217.391 | Startup: 238.095 | | Worst Case: 208.333 | Worst Case: 204.082 | | Average: 295.873 | Average: 275.846 | | Score: 237.521 | Score: 237.542 | | Wall time: 0:02.035 | Wall time: 0:02.180 | | Running stanford-crypto-aes: | Running stanford-crypto-aes: | | Startup: 147.059 | Startup: 161.290 | | Worst Case: 204.082 | Worst Case: 273.973 | | Average: 307.653 | Average: 321.274 | | Score: 209.790 | Score: 242.138 | | Wall time: 0:01.990 | Wall time: 0:01.900 | | Running splay: | Running splay: | | Startup: 208.333 | Startup: 217.391 | | Worst Case: 140.845 | Worst Case: 119.048 | | Average: 318.352 | Average: 328.366 | | Score: 210.605 | Score: 204.068 | | Wall time: 0:01.925 | Wall time: 0:01.870 | | Running richards-wasm: | Running richards-wasm: | | Startup: 277.778 | Startup: 238.095 | | Run time: 3.794 | Run time: 4.013 | | Score: 32.462 | Score: 30.910 | | Running richards: | Running richards: | | Startup: 238.095 | Startup: 238.095 | | Worst Case: 625 | Worst Case: 625 | | Average: 683.908 | Average: 697.538 | | Score: 466.885 | Score: 469.966 | | Wall time: 0:00.892 | Wall time: 0:00.875 | | Running regexp: | Running regexp: | | Startup: 200 | Startup: 178.571 | | Worst Case: 176.991 | Worst Case: 168.067 | | Average: 211.443 | Average: 217.949 | | Score: 195.610 | Score: 187.018 | | Wall time: 0:02.893 | Wall time: 0:02.815 | | Running regex-dna-SP: | Running regex-dna-SP: | | Startup: 294.118 | Startup: 263.158 | | Worst Case: 250 | Worst Case: 256.410 | | Average: 288.975 | Average: 299.597 | | Score: 276.975 | Score: 272.414 | | Wall time: 0:02.078 | Wall time: 0:02.007 | | Running raytrace: | Running raytrace: | | Startup: 151.515 | Startup: 147.059 | | Worst Case: 312.500 | Worst Case: 317.460 | | Average: 690.255 | Average: 704.976 | | Score: 319.722 | Score: 320.468 | | Wall time: 0:00.897 | Wall time: 0:00.880 | | Running quicksort-wasm: | Running quicksort-wasm: | | Startup: 357.143 | Startup: 333.333 | | Run time: 125 | Run time: 142.857 | | Score: 211.289 | Score: 218.218 | | Running prepack-wtb: | Running prepack-wtb: | | Startup: 10.965 | Startup: 10.893 | | Worst Case: 19.763 | Worst Case: 16.234 | | Average: 25.126 | Average: 25.641 | | Score: 17.592 | Score: 16.551 | | Wall time: 0:02.118 | Wall time: 0:02.049 | | Running pdfjs: | Running pdfjs: | | Startup: 54.348 | Startup: 53.763 | | Worst Case: 87.336 | Worst Case: 90.090 | | Average: 139.312 | Average: 139.803 | | Score: 87.121 | Score: 87.813 | | Wall time: 0:04.453 | Wall time: 0:04.439 | | Running OfflineAssembler: | Running OfflineAssembler: | | Startup: 38.168 | Startup: 38.760 | | Worst Case: 67.568 | Worst Case: 68.259 | | Average: 79.525 | Average: 84.800 | | Score: 58.972 | Score: 60.764 | | Wall time: 0:05.129 | Wall time: 0:04.817 | | Running octane-zlib: | Running octane-zlib: | | Startup: 7.680 | Startup: 7.886 | | Worst Case: 20.964 | Worst Case: 21.097 | | Average: 21.598 | Average: 21.558 | | Score: 15.151 | Score: 15.308 | | Wall time: 0:03.893 | Wall time: 0:03.883 | | Running octane-code-load: | Running octane-code-load: | | Startup: 384.615 | Startup: 384.615 | | Worst Case: 444.444 | Worst Case: 454.545 | | Average: 500 | Average: 515.598 | | Score: 440.492 | Score: 448.372 | | Wall time: 0:01.204 | Wall time: 0:01.169 | | Running navier-stokes: | Running navier-stokes: | | Startup: 312.500 | Startup: 312.500 | | Worst Case: 666.667 | Worst Case: 666.667 | | Average: 829.847 | Average: 828.691 | | Score: 557.082 | Score: 556.823 | | Wall time: 0:00.736 | Wall time: 0:00.737 | | Running n-body-SP: | Running n-body-SP: | | Startup: 312.500 | Startup: 312.500 | | Worst Case: 408.163 | Worst Case: 408.163 | | Average: 448.380 | Average: 456.639 | | Score: 385.280 | Score: 387.631 | | Wall time: 0:01.344 | Wall time: 0:01.321 | | Running multi-inspector-code-load: | Running multi-inspector-code-load: | | Startup: 128.205 | Startup: 131.579 | | Worst Case: 121.212 | Worst Case: 109.890 | | Average: 384.367 | Average: 384.615 | | Score: 181.440 | Score: 177.170 | | Wall time: 0:01.656 | Wall time: 0:01.645 | | Running ML: | Running ML: | | Startup: 43.103 | Startup: 43.860 | | Worst Case: 51.414 | Worst Case: 51.948 | | Average: 70.055 | Average: 71.619 | | Score: 53.746 | Score: 54.646 | | Wall time: 0:04.341 | Wall time: 0:04.248 | | Running mandreel: | Running mandreel: | | Startup: 29.762 | Startup: 31.056 | | Worst Case: 85.106 | Worst Case: 84.034 | | Average: 105.728 | Average: 108.190 | | Score: 64.457 | Score: 65.604 | | Wall time: 0:04.354 | Wall time: 0:04.251 | | Running lebab-wtb: | Running lebab-wtb: | | Startup: 13.021 | Startup: 13.333 | | Worst Case: 22.624 | Worst Case: 23.810 | | Average: 25.608 | Average: 27.137 | | Score: 19.612 | Score: 20.500 | | Wall time: 0:01.985 | Wall time: 0:01.913 | | Running json-stringify-inspector: | Running json-stringify-inspector: | | Startup: 104.167 | Startup: 106.383 | | Worst Case: 107.527 | Worst Case: 107.527 | | Average: 112.693 | Average: 113.365 | | Score: 108.072 | Score: 109.049 | | Wall time: 0:01.107 | Wall time: 0:01.096 | | Running json-parse-inspector: | Running json-parse-inspector: | | Startup: 104.167 | Startup: 111.111 | | Worst Case: 86.957 | Worst Case: 91.743 | | Average: 103.486 | Average: 110.081 | | Score: 97.867 | Score: 103.916 | | Wall time: 0:01.048 | Wall time: 0:00.989 | | Running jshint-wtb: | Running jshint-wtb: | | Startup: 11.111 | Startup: 11.086 | | Worst Case: 15.106 | Worst Case: 16.026 | | Average: 17.575 | Average: 18.657 | | Score: 14.342 | Score: 14.910 | | Wall time: 0:02.042 | Wall time: 0:01.957 | | Running HashSet-wasm: | Running HashSet-wasm: | | Startup: 28.736 | Startup: 34.014 | | Run time: 5.092 | Run time: 5.102 | | Score: 12.096 | Score: 13.173 | | Running hash-map: | Running hash-map: | | Startup: 151.515 | Startup: 156.250 | | Worst Case: 266.667 | Worst Case: 192.308 | | Average: 337.302 | Average: 333.707 | | Score: 238.862 | Score: 215.639 | | Wall time: 0:01.798 | Wall time: 0:01.817 | | Running gcc-loops-wasm: | Running gcc-loops-wasm: | | Startup: 119.048 | Startup: 81.967 | | Run time: 0.433 | Run time: 0.440 | | Score: 7.178 | Score: 6.006 | | Running gbemu: | Running gbemu: | | Startup: 39.370 | Startup: 38.462 | | Worst Case: 86.580 | Worst Case: 84.746 | | Average: 140.496 | Average: 135.969 | | Score: 78.238 | Score: 76.242 | | Wall time: 0:04.382 | Wall time: 0:04.524 | | Running gaussian-blur: | Running gaussian-blur: | | Startup: 161.290 | Startup: 156.250 | | Worst Case: 235.294 | Worst Case: 224.719 | | Average: 253.084 | Average: 261.538 | | Score: 212.566 | Score: 209.411 | | Wall time: 0:02.393 | Wall time: 0:02.316 | | Running float-mm.c: | Running float-mm.c: | | Startup: 5.513 | Startup: 5.453 | | Worst Case: 5.470 | Worst Case: 5.525 | | Average: 5.652 | Average: 5.724 | | Score: 5.544 | Score: 5.566 | | Wall time: 0:13.298 | Wall time: 0:13.151 | | Running FlightPlanner: | Running FlightPlanner: | | Startup: 161.290 | Startup: 178.571 | | Worst Case: 434.783 | Worst Case: 454.545 | | Average: 557.116 | Average: 563.981 | | Score: 339.319 | Score: 357.727 | | Wall time: 0:01.679 | Wall time: 0:01.614 | | Running first-inspector-code-load: | Running first-inspector-code-load: | | Startup: 121.951 | Startup: 125 | | Worst Case: 117.647 | Worst Case: 117.647 | | Average: 126.731 | Average: 128.399 | | Score: 122.053 | Score: 123.600 | | Wall time: 0:04.769 | Wall time: 0:04.735 | | Running espree-wtb: | Running espree-wtb: | | Startup: 16.077 | Startup: 15.823 | | Worst Case: 25.773 | Worst Case: 24.272 | | Average: 27.510 | Average: 27.322 | | Score: 22.506 | Score: 21.893 | | Wall time: 0:01.484 | Wall time: 0:01.488 | | Running earley-boyer: | Running earley-boyer: | | Startup: 227.273 | Startup: 238.095 | | Worst Case: 400 | Worst Case: 434.783 | | Average: 836.850 | Average: 877.581 | | Score: 423.726 | Score: 449.542 | | Wall time: 0:00.742 | Wall time: 0:00.709 | | Running delta-blue: | Running delta-blue: | | Startup: 185.185 | Startup: 192.308 | | Worst Case: 555.556 | Worst Case: 588.235 | | Average: 1018.836 | Average: 1010.187 | | Score: 471.497 | Score: 485.270 | | Wall time: 0:00.613 | Wall time: 0:00.617 | | Running date-format-xparb-SP: | Running date-format-xparb-SP: | | Startup: 135.135 | Startup: 135.135 | | Worst Case: 143.885 | Worst Case: 136.054 | | Average: 160.291 | Average: 150.595 | | Score: 146.071 | Score: 140.420 | | Wall time: 0:03.754 | Wall time: 0:03.991 | | Running date-format-tofte-SP: | Running date-format-tofte-SP: | | Startup: 100 | Startup: 100 | | Worst Case: 133.333 | Worst Case: 130.719 | | Average: 150.747 | Average: 149.310 | | Score: 126.201 | Score: 124.971 | | Wall time: 0:03.999 | Wall time: 0:04.036 | | Running crypto-sha1-SP: | Running crypto-sha1-SP: | | Startup: 277.778 | Startup: 277.778 | | Worst Case: 392.157 | Worst Case: 425.532 | | Average: 592.040 | Average: 604.675 | | Score: 401.023 | Score: 415.002 | | Wall time: 0:01.024 | Wall time: 0:01.004 | | Running crypto-md5-SP: | Running crypto-md5-SP: | | Startup: 250 | Startup: 263.158 | | Worst Case: 444.444 | Worst Case: 444.444 | | Average: 501.686 | Average: 508.547 | | Score: 382.000 | Score: 390.351 | | Wall time: 0:01.207 | Wall time: 0:01.190 | | Running crypto-aes-SP: | Running crypto-aes-SP: | | Startup: 238.095 | Startup: 238.095 | | Worst Case: 285.714 | Worst Case: 338.983 | | Average: 476.763 | Average: 489.712 | | Score: 318.905 | Score: 340.635 | | Wall time: 0:01.272 | Wall time: 0:01.237 | | Running crypto: | Running crypto: | | Startup: 384.615 | Startup: 384.615 | | Worst Case: 833.333 | Worst Case: 800 | | Average: 1054.965 | Average: 1079.855 | | Score: 696.671 | Score: 692.618 | | Wall time: 0:00.581 | Wall time: 0:00.568 | | Running coffeescript-wtb: | Running coffeescript-wtb: | | Startup: 11.628 | Startup: 11.848 | | Worst Case: 17.241 | Worst Case: 16.722 | | Average: 19.048 | Average: 19.960 | | Score: 15.630 | Score: 15.814 | | Wall time: 0:01.911 | Wall time: 0:01.850 | | Running chai-wtb: | Running chai-wtb: | | Startup: 24.876 | Startup: 25 | | Worst Case: 38.760 | Worst Case: 38.168 | | Average: 42.735 | Average: 42.194 | | Score: 34.539 | Score: 34.274 | | Wall time: 0:01.092 | Wall time: 0:01.082 | | Running cdjs: | Running cdjs: | | Startup: 30.120 | Startup: 31.847 | | Worst Case: 32.328 | Worst Case: 32.397 | | Average: 36.299 | Average: 36.090 | | Score: 32.818 | Score: 33.393 | | Wall time: 0:08.295 | Wall time: 0:08.335 | | Running Box2D: | Running Box2D: | | Startup: 60.241 | Startup: 60.241 | | Worst Case: 259.740 | Worst Case: 259.740 | | Average: 434.307 | Average: 438.144 | | Score: 189.413 | Score: 189.969 | | Wall time: 0:01.467 | Wall time: 0:01.456 | | Running Basic: | Running Basic: | | Startup: 116.279 | Startup: 119.048 | | Worst Case: 238.095 | Worst Case: 289.855 | | Average: 421.986 | Average: 562.914 | | Score: 226.908 | Score: 268.812 | | Wall time: 0:01.455 | Wall time: 0:01.101 | | Running base64-SP: | Running base64-SP: | | Startup: 250 | Startup: 238.095 | | Worst Case: 285.714 | Worst Case: 294.118 | | Average: 409.498 | Average: 415.793 | | Score: 308.111 | Score: 307.645 | | Wall time: 0:01.474 | Wall time: 0:01.453 | | Running babylon-wtb: | Running babylon-wtb: | | Startup: 15.974 | Startup: 16.340 | | Worst Case: 28.902 | Worst Case: 29.070 | | Average: 31.299 | Average: 31.646 | | Score: 24.357 | Score: 24.679 | | Wall time: 0:01.378 | Wall time: 0:01.353 | | Running Babylon: | Running Babylon: | | Startup: 78.125 | Startup: 79.365 | | Worst Case: 303.030 | Worst Case: 227.273 | | Average: 498.325 | Average: 501.686 | | Score: 227.648 | Score: 208.387 | | Wall time: 0:01.268 | Wall time: 0:01.260 | | Running async-fs: | Running async-fs: | | Startup: 59.524 | Startup: 60.241 | | Worst Case: 75.377 | Worst Case: 90.909 | | Average: 100.464 | Average: 102.362 | | Score: 76.674 | Score: 82.454 | | Wall time: 0:02.026 | Wall time: 0:01.990 | | Running Air: | Running Air: | | Startup: 50.505 | Startup: 49.020 | | Worst Case: 186.916 | Worst Case: 168.067 | | Average: 648.855 | Average: 650.273 | | Score: 182.968 | Score: 174.978 | | Wall time: 0:01.028 | Wall time: 0:01.028 | | Running ai-astar: | Running ai-astar: | | Startup: 250 | Startup: 250 | | Worst Case: 240.964 | Worst Case: 303.030 | | Average: 382.883 | Average: 396.667 | | Score: 284.655 | Score: 310.898 | | Wall time: 0:01.642 | Wall time: 0:01.587 | | Running acorn-wtb: | Running acorn-wtb: | | Startup: 15.060 | Startup: 15.432 | | Worst Case: 25.773 | Worst Case: 26.178 | | Average: 29.455 | Average: 28.571 | | Score: 22.528 | Score: 22.599 | | Wall time: 0:01.437 | Wall time: 0:01.430 | | Running 3d-raytrace-SP: | Running 3d-raytrace-SP: | | Startup: 156.250 | Startup: 156.250 | | Worst Case: 198.020 | Worst Case: 253.165 | | Average: 313.819 | Average: 329.093 | | Score: 213.338 | Score: 235.241 | | Wall time: 0:01.930 | Wall time: 0:01.841 | | Running 3d-cube-SP: | Running 3d-cube-SP: | | Startup: 166.667 | Startup: 172.414 | | Worst Case: 303.030 | Worst Case: 285.714 | | Average: 344.130 | Average: 356.929 | | Score: 259.031 | Score: 260.033 | | Wall time: 0:01.760 | Wall time: 0:01.697 | | | | | | | | Stdlib: 0.898 | Stdlib: 0.918 | | MainRun: 0.332 | MainRun: 0.338 | | First: 78.720 | First: 80.064 | | Worst: 121.695 | Worst: 121.508 | | Average: 163.034 | Average: 166.564 | | Startup: 120.068 | Startup: 111.921 | | Runtime: 4.440 | Runtime: 4.630 | | | | | Total Score: 93.426 | Total Score: 94.389 | |-------------------------------------|------------------------------------|
Yusuke Suzuki
Comment 4 2019-07-15 11:27:28 PDT
Comment on attachment 373806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373806&action=review r=me with nit. > Source/JavaScriptCore/KeywordLookupGenerator.py:170 > + needle = "(Char []){" + ", ".join(["'%s'" % (c) for c in test]) + "}" Can you change it to "static_cast" instead of C-style cast?
Saagar Jha
Comment 5 2019-07-15 15:38:08 PDT
Created attachment 374157 [details] Patch
Saagar Jha
Comment 6 2019-07-15 15:42:58 PDT
The declaration you're pointing to is a compound literal, not a cast. In any case, I've replaced it with std::array instead because it optimizes better and allowed me to remove the complexity around specializing Lexer::parseKeyword for each character type. The new patch generates identical assembly and benchmarks the same on my machine.
Yusuke Suzuki
Comment 7 2019-07-15 15:46:36 PDT
Comment on attachment 374157 [details] Patch r=me
WebKit Commit Bot
Comment 8 2019-07-15 17:42:33 PDT
Comment on attachment 374157 [details] Patch Clearing flags on attachment: 374157 Committed r247463: <https://trac.webkit.org/changeset/247463>
WebKit Commit Bot
Comment 9 2019-07-15 17:42:34 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2019-07-15 17:44:39 PDT
<rdar://problem/53131061>
Filip Pizlo
Comment 11 2019-07-17 09:03:36 PDT
(In reply to Saagar Jha from comment #0) > KeywordLookup.h rolls what appears to be its own custom memcmp to do string > matches, but it performs unaligned loads, reinterpret_casts, and in general > makes UBSan very unhappy. I don't think these constitute good reasons for landing changes to WebKit because: - We intentionally use unaligned loads in some parts of the code. - We intentionally use reinterpret_casts a lot. - We intentionally do other things that UBSan doesn't agree with. I don't think we want to change our conventions in regard to those features of the language. > I think we can use just use memcmp instead > (someone should check whether I can do this, though: I may be > misunderstanding how the character encoding works). I have checked that > Clang and GCC produce reasonable-looking assembly for this if we make what > we're doing clear enough.
Saagar Jha
Comment 12 2019-07-17 09:50:00 PDT
(In reply to Filip Pizlo from comment #11) > (In reply to Saagar Jha from comment #0) > > I don't think these constitute good reasons for landing changes to WebKit > because: > > - We intentionally use unaligned loads in some parts of the code. > - We intentionally use reinterpret_casts a lot. > - We intentionally do other things that UBSan doesn't agree with. > > I don't think we want to change our conventions in regard to those features > of the language. Note that the way WebKit was using these was not a "feature of the language": strictly speaking, the code was invalid C++, and this patch ensures that the code cannot be miscompiled in the future. From a practical standpoint I understand that we already have undefined behavior in our code that we will never be able to completely remove (either because we can't find it, or because we need to do something that cannot be legally expressed in the language)–but I don't see why we should be opposed to changes that remove undefined behavior with no regression in execution speed (if JavaScript ever adds a long keyword, I think this change might actually *improve* performance). Especially if those changes replace a hundred lines of specialized and somewhat tedious macros with an easier-to-maintain solution ;)
Filip Pizlo
Comment 13 2019-07-17 10:37:07 PDT
(In reply to Saagar Jha from comment #12) > (In reply to Filip Pizlo from comment #11) > > (In reply to Saagar Jha from comment #0) > > > > I don't think these constitute good reasons for landing changes to WebKit > > because: > > > > - We intentionally use unaligned loads in some parts of the code. > > - We intentionally use reinterpret_casts a lot. > > - We intentionally do other things that UBSan doesn't agree with. > > > > I don't think we want to change our conventions in regard to those features > > of the language. > > Note that the way WebKit was using these was not a "feature of the > language": strictly speaking, the code was invalid C++ Not "was using". Webkit is using these features in ways that we know are outside what the spec allows, because we don't program to the C++-in-the-spec. Instead we program in the C++-that-compilers-implement. > , and this patch > ensures that the code cannot be miscompiled in the future. From a practical > standpoint I understand that we already have undefined behavior in our code > that we will never be able to completely remove (either because we can't > find it, or because we need to do something that cannot be legally expressed > in the language)–but I don't see why we should be opposed to changes that > remove undefined behavior with no regression in execution speed (if > JavaScript ever adds a long keyword, I think this change might actually > *improve* performance). Especially if those changes replace a hundred lines > of specialized and somewhat tedious macros with an easier-to-maintain > solution ;) Because all changes carry probability of regression, sometimes regressions that are only discovered long after the change landed. One of the best ways to ensure code quality is to reject frivolous changes.
Yusuke Suzuki
Comment 14 2019-09-05 13:25:35 PDT
Our performance bots is saying that this causes 5% regression in JetStream2/multi-inspector-code-load, and 2.5% regression in JetStream2/first-inspector-code-load. Until we can fix this regression, I'll roll-out this patch for now.
Filip Pizlo
Comment 15 2019-09-05 13:35:56 PDT
(In reply to Yusuke Suzuki from comment #14) > Our performance bots is saying that this causes 5% regression in > JetStream2/multi-inspector-code-load, and 2.5% regression in > JetStream2/first-inspector-code-load. Until we can fix this regression, I'll > roll-out this patch for now. Yeah let’s roll out. We should avoid these changes like the plague.
WebKit Commit Bot
Comment 16 2019-09-05 14:07:43 PDT
Re-opened since this is blocked by bug 201515
Note You need to log in before you can comment on or make changes to this bug.