Created attachment 295265 [details] getting started

192 failures!

Here's what I'm finding: - The data structures that are used to track LargeAllocation seem like they may have some thread-safety issues. It's clear that the LargeAllocation table is getting corrupted, but it's not clear why. It *seems* like that code should not need locking since the collector only parses that table when the world is stopped and the mutator only shrinks the table when the collector is not running. The mutator may grow the table while the collector is running, but that shouldn't be a problem. - We need some kind of thread safety story in MarkedAllocator and BlockSet. I think that we need a read-write lock there, implemented using two normal locks: one lock for the mutator to read, one lock for the collector to read, and to write you need to grab both locks.

(In reply to comment #3) > Here's what I'm finding: > > - The data structures that are used to track LargeAllocation seem like they > may have some thread-safety issues. It's clear that the LargeAllocation > table is getting corrupted, but it's not clear why. It *seems* like that > code should not need locking since the collector only parses that table when > the world is stopped and the mutator only shrinks the table when the > collector is not running. The mutator may grow the table while the > collector is running, but that shouldn't be a problem. Well that was easy. The LargeAllocation table had a second set of pointers directly into the vector that the GC would set up at the start of a cycle. In fact, it needs to set those pointers up at the start of each STW increment. > > - We need some kind of thread safety story in MarkedAllocator and BlockSet. > I think that we need a read-write lock there, implemented using two normal > locks: one lock for the mutator to read, one lock for the collector to read, > and to write you need to grab both locks. I'm going to work on this next, though it seems that with the above fix, the concurrent GC is a heck of a lot more stable.

Created attachment 295337 [details] fixed LargeAllocation

The problem with MarkedAllocator is that it has those darned bitvectors, and they can resize. Here are the possible things we could do: - Let the mutator stop the collector when resizing the bitvectors. - Use segmented bitvectors to allow lock-free resizing. - Put read-write locks around the bitvectors. - Try to implement a concurrent copying system for bits. - Rely on the fact that marking only messes with those bits inside aboutToMarkSlow and noteMarkedSlow. The former already holds the block lock and the latter easily could. We could resize the bitvectors by locking all of the blocks in the allocator!

I'm going to slow down a bit and run regression tests on the version with the LargeAllocation fix and then I'll run benchmarks. I want to make sure that the fixes so far have not regressed anything. Then, I think I'll try the read-write lock approach first. This means one extra lock acquisition in aboutToMarkSlow and one extra lock acquisition in noteMarkedSlow. On the flipside, they lose the need to CAS atomically. I'll go with that if it's fast enough - otherwise I think I'll try the lock-all-blocks approach to resizing. This is clearly the lowest impact and the bitvectors do not resize in steady state so this ought to be fine.

Looks like the next bug is that CodeBlockSet is failing to delete code blocks at the end of the collection in which they died. This is causing the next batch of nasty crashes.

Created attachment 295340 [details] fixed CodeBlockSet and InferredValue Down to 60 failures in run-javascriptcor-tests --release.

Created attachment 295342 [details] better tests This adds a new dedicated collect-continuously mode. It runs with no-cjit. This also makes the dfg-eager modes collect continuously, as the ftl-eager ones did. Test failure rate is up to 91 because of the new modes.

Created attachment 295387 [details] more fixes Down to 50 failures.

Created attachment 295452 [details] even more fixes collectContinuously indeed found two pre-existing bugs: one in IsFunction constant folding and another in SamplingProfiler.

Created attachment 295463 [details] getting so stable Fixed race conditions in some UnconditionalFinalizers.

(In reply to comment #13) > Created attachment 295463 [details] > getting so stable > > Fixed race conditions in some UnconditionalFinalizers. 49 failures.

Created attachment 295477 [details] fixed another CodeBlock GC bug I'll retest soon...

(In reply to comment #15) > Created attachment 295477 [details] > fixed another CodeBlock GC bug > > I'll retest soon... Only 4 test failures.

Looks like turning on concurrent GC is a perf win. Benchmark report for SunSpider, LongSpider, Octane, Kraken, and AsmBench on murderface (MacBookPro11,5). VMs tested: "TipOfTree" at /Volumes/Data/secondary/OpenSource/WebKitBuild/Release/jsc (r208896) "Things" at /Volumes/Data/tertiary/OpenSource/WebKitBuild/Release/jsc (r208896) Collected 6 samples per benchmark/VM, with 6 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. TipOfTree Things SunSpider: 3d-cube 4.5568+-0.2104 ? 4.6104+-0.1811 ? might be 1.0118x slower 3d-morph 4.8957+-0.3341 4.6603+-0.0744 might be 1.0505x faster 3d-raytrace 4.4856+-0.1515 ? 4.6117+-0.2347 ? might be 1.0281x slower access-binary-trees 1.9371+-0.0715 ? 1.9637+-0.0310 ? might be 1.0137x slower access-fannkuch 4.7087+-0.1779 ? 4.7810+-0.3312 ? might be 1.0153x slower access-nbody 2.4576+-0.1744 2.4170+-0.0542 might be 1.0168x faster access-nsieve 2.8630+-0.0690 2.8418+-0.0362 bitops-3bit-bits-in-byte 1.1185+-0.0548 ? 1.1661+-0.0999 ? might be 1.0426x slower bitops-bits-in-byte 2.7521+-0.1011 2.7282+-0.0234 bitops-bitwise-and 1.9471+-0.0609 ? 1.9580+-0.0467 ? bitops-nsieve-bits 3.0599+-0.1556 ? 3.0605+-0.0924 ? controlflow-recursive 2.2330+-0.0382 ? 2.2377+-0.0499 ? crypto-aes 3.9156+-0.0593 ? 3.9187+-0.0827 ? crypto-md5 2.3680+-0.0535 ? 2.4383+-0.0324 ? might be 1.0297x slower crypto-sha1 2.5566+-0.3177 2.5212+-0.1994 might be 1.0140x faster date-format-tofte 6.8097+-0.1118 ? 6.8496+-0.2203 ? date-format-xparb 4.6393+-0.1944 4.6160+-0.0954 math-cordic 2.7399+-0.1414 2.6518+-0.0339 might be 1.0333x faster math-partial-sums 3.9711+-0.1888 ? 3.9960+-0.2891 ? math-spectral-norm 1.9341+-0.0580 ? 1.9559+-0.0399 ? might be 1.0113x slower regexp-dna 6.0775+-0.1474 ? 6.3765+-0.5609 ? might be 1.0492x slower string-base64 4.4662+-0.0658 ? 4.5625+-0.2903 ? might be 1.0215x slower string-fasta 5.4991+-0.3321 5.3461+-0.2175 might be 1.0286x faster string-tagcloud 8.1984+-0.1135 ? 8.2316+-0.1361 ? string-unpack-code 18.4231+-0.7940 ? 19.3407+-0.8716 ? might be 1.0498x slower string-validate-input 4.1622+-0.0456 ? 4.2281+-0.1240 ? might be 1.0158x slower <arithmetic> 4.3375+-0.0358 ? 4.3873+-0.0448 ? might be 1.0115x slower TipOfTree Things LongSpider: 3d-cube 811.3600+-6.7267 796.9972+-13.3162 might be 1.0180x faster 3d-morph 565.0785+-9.0159 ? 567.5167+-6.9989 ? 3d-raytrace 448.8218+-2.9032 ? 451.7279+-4.3239 ? access-binary-trees 841.5369+-3.2932 827.4131+-11.2758 might be 1.0171x faster access-fannkuch 236.3149+-12.3910 235.7251+-13.0097 access-nbody 520.3270+-9.1828 520.1399+-4.0402 access-nsieve 283.3927+-3.8265 ? 287.1245+-7.4576 ? might be 1.0132x slower bitops-3bit-bits-in-byte 31.8977+-1.0332 31.7256+-0.9814 bitops-bits-in-byte 82.4250+-6.1793 81.0194+-2.4124 might be 1.0173x faster bitops-nsieve-bits 365.6275+-5.2766 ? 369.2638+-10.5014 ? controlflow-recursive 400.2899+-2.2029 ? 403.0983+-5.7452 ? crypto-aes 543.1446+-11.3802 ? 544.1858+-8.1015 ? crypto-md5 508.7051+-9.0968 503.5114+-4.6654 might be 1.0103x faster crypto-sha1 592.3989+-2.2150 587.7568+-3.9554 date-format-tofte 334.7242+-9.0115 ? 337.0022+-5.9575 ? date-format-xparb 603.6682+-2.2729 602.9265+-1.8511 hash-map 137.2998+-4.3694 ? 141.3467+-4.2756 ? might be 1.0295x slower math-cordic 399.7740+-3.8280 ? 400.0544+-4.1806 ? math-partial-sums 282.7010+-2.2918 281.0641+-2.4510 math-spectral-norm 518.3232+-5.1667 517.3634+-5.1537 string-base64 469.0892+-7.4604 ^ 393.6552+-33.9118 ^ definitely 1.1916x faster string-fasta 337.4228+-8.5378 329.4245+-3.2522 might be 1.0243x faster string-tagcloud 168.1153+-1.0714 165.9889+-4.3208 might be 1.0128x faster <geometric> 339.6449+-1.6040 ^ 336.3873+-1.5068 ^ definitely 1.0097x faster TipOfTree Things Octane: encrypt 0.14930+-0.00272 ? 0.14989+-0.00260 ? decrypt 2.53662+-0.01191 ? 2.56198+-0.06151 ? deltablue x2 0.12235+-0.00179 0.12033+-0.00143 might be 1.0168x faster earley 0.24699+-0.00099 0.24624+-0.00177 boyer 4.23707+-0.13908 4.15846+-0.02935 might be 1.0189x faster navier-stokes x2 4.58244+-0.02375 ? 4.60941+-0.01289 ? raytrace x2 0.70329+-0.00553 0.70071+-0.00483 richards x2 0.07868+-0.00067 ? 0.07899+-0.00141 ? splay x2 0.31799+-0.00502 ^ 0.30072+-0.00529 ^ definitely 1.0574x faster regexp x2 16.49159+-0.94131 16.26655+-0.40570 might be 1.0138x faster pdfjs x2 40.79251+-0.70817 39.71938+-0.44233 might be 1.0270x faster mandreel x2 39.83001+-0.36496 ? 40.23303+-0.13319 ? might be 1.0101x slower gbemu x2 28.67573+-0.54130 28.51590+-0.34242 closure 0.46759+-0.00650 0.46220+-0.00168 might be 1.0117x faster jquery 6.24006+-0.02474 ! 6.33973+-0.03538 ! definitely 1.0160x slower box2d x2 8.71716+-0.07835 ? 8.76956+-0.12658 ? zlib x2 338.25633+-10.90844 329.24019+-14.45725 might be 1.0274x faster typescript x2 614.11556+-13.53791 607.00456+-7.46490 might be 1.0117x faster <geometric> 4.76284+-0.01919 ^ 4.71988+-0.00753 ^ definitely 1.0091x faster TipOfTree Things Kraken: ai-astar 88.457+-1.445 ? 90.947+-2.930 ? might be 1.0281x slower audio-beat-detection 35.708+-0.583 35.533+-0.252 audio-dft 97.777+-2.015 95.347+-2.645 might be 1.0255x faster audio-fft 27.363+-0.082 ? 27.432+-0.229 ? audio-oscillator 43.233+-0.640 ? 44.191+-1.636 ? might be 1.0222x slower imaging-darkroom 55.380+-0.301 ? 55.780+-0.237 ? imaging-desaturate 40.670+-0.277 ? 41.172+-0.418 ? might be 1.0124x slower imaging-gaussian-blur 59.087+-2.515 57.705+-4.267 might be 1.0240x faster json-parse-financial 33.768+-0.585 33.763+-1.631 json-stringify-tinderbox 21.422+-1.782 ? 21.639+-1.422 ? might be 1.0101x slower stanford-crypto-aes 35.149+-0.543 ? 35.203+-0.646 ? stanford-crypto-ccm 34.934+-3.005 32.311+-2.280 might be 1.0812x faster stanford-crypto-pbkdf2 89.609+-0.539 ? 91.184+-4.659 ? might be 1.0176x slower stanford-crypto-sha256-iterative 29.954+-0.447 29.663+-0.700 <arithmetic> 49.465+-0.447 49.419+-0.906 might be 1.0009x faster TipOfTree Things AsmBench: bigfib.cpp 408.0430+-4.7006 402.9780+-1.1966 might be 1.0126x faster cray.c 356.6054+-3.9908 356.5528+-3.6843 dry.c 391.7522+-3.4333 ? 394.7277+-9.2834 ? FloatMM.c 609.5984+-3.4004 ? 621.8640+-27.8287 ? might be 1.0201x slower gcc-loops.cpp 3356.0475+-10.7577 ? 3361.3108+-18.6431 ? n-body.c 746.7086+-5.1632 ? 750.3443+-1.2910 ? Quicksort.c 372.1285+-6.1118 369.2996+-11.1960 stepanov_container.cpp 3126.0235+-30.6852 3124.4739+-33.6255 Towers.c 220.9481+-4.5037 ? 222.1495+-4.2271 ? <geometric> 655.5962+-2.5172 ? 656.8712+-2.0530 ? might be 1.0019x slower TipOfTree Things Geomean of preferred means: <scaled-result> 46.9253+-0.0703 46.8657+-0.1812 might be 1.0013x faster

Created attachment 295561 [details] added a nice visual benchmark for GCs

Created attachment 295579 [details] passes JSC tests!

Attachment 295579 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2176: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 5 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 295579 [details] passes JSC tests! Attachment 295579 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2587109 New failing tests: js/slow-stress/Int32Array-alloc-large-long-lived.html js/slow-stress/ArrayBuffer-Int8Array-alloc-large-long-lived-fragmented.html js/slow-stress/ArrayBuffer-Int8Array-alloc-huge-long-lived.html

Created attachment 295586 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 295604 [details] more fixes

Attachment 295604 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2176: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 5 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 295604 [details] more fixes Attachment 295604 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2589818 New failing tests: js/slow-stress/Int32Array-alloc-large-long-lived.html fast/events/form-onchange.html

Created attachment 295615 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 295667 [details] fix race in addNewPropertyTransition

Attachment 295667 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2176: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 295669 [details] with ChangeLog

Attachment 295669 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2176: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 295669 [details] with ChangeLog Attachment 295669 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2591821 New failing tests: js/slow-stress/Int32Array-alloc-large-long-lived.html js/slow-stress/ArrayBuffer-Int8Array-alloc-large-long-lived-fragmented.html js/slow-stress/nested-function-parsing-random.html

Created attachment 295688 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

(In reply to comment #6) > The problem with MarkedAllocator is that it has those darned bitvectors, and > they can resize. > > Here are the possible things we could do: > > - Let the mutator stop the collector when resizing the bitvectors. > > - Use segmented bitvectors to allow lock-free resizing. > > - Put read-write locks around the bitvectors. > > - Try to implement a concurrent copying system for bits. > > - Rely on the fact that marking only messes with those bits inside > aboutToMarkSlow and noteMarkedSlow. The former already holds the block lock > and the latter easily could. We could resize the bitvectors by locking all > of the blocks in the allocator! I've circled back to this now that I have everything else worked out. I realized that having a stopTheCollector mechanism is actually super easy, and probably generally useful. I'm playing with that solution now. I have a test that seems to reliably crash because of the bitvectors getting messed up, probably because of this bug. Hopefully doing this will fix that program.

Created attachment 295704 [details] possible fix for MarkedBlock crashes Still testing this. Not sure if this is really right.

Attachment 295704 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2197: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 295706 [details] better fix

Attachment 295706 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2197: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 295706 [details] better fix Attachment 295706 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2594171 New failing tests: inspector/console/clearMessages.html

Created attachment 295711 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 295746 [details] the patch

(In reply to comment #40) > Created attachment 295746 [details] > the patch I don't know if it's stable enough yet, but it's rapidly getting there.

Attachment 295746 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:353: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2197: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 295746 [details] the patch Attachment 295746 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2598341 New failing tests: inspector/console/clearMessages.html

Created attachment 295808 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 295812 [details] fix one more bug

Attachment 295812 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2210: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 8 in 48 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 295898 [details] audited all visitChildren in JSC

Comment on attachment 295898 [details] audited all visitChildren in JSC View in context: https://bugs.webkit.org/attachment.cgi?id=295898&action=review r=me > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2957 > + codeBlock->jettison(Profiler::JettisonDueToOldAge); I think you should update the comment around "this != replacement()". It is now possible for that condition to be true simply because we were replacement() at time of check but we were no longer replacement() at time of jettison -- that's a perfectly valid outcome, and an important condition to get right.

Created attachment 295987 [details] the patch

Comment on attachment 295987 [details] the patch r=me

Created attachment 296012 [details] more locking Fixed a couple races in WebCore.

Created attachment 296039 [details] the patch Lots of fixes in WebCore

Attachment 296039 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2210: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 296042 [details] the patch More fixes.

Attachment 296042 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2210: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296042 [details] the patch Attachment 296042 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2613104 New failing tests: media/modern-media-controls/media-controller/media-controller-resize.html

Created attachment 296045 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 296054 [details] the patch

Attachment 296054 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2210: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296054 [details] the patch Attachment 296054 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2615085 New failing tests: media/modern-media-controls/media-controller/media-controller-resize.html

Created attachment 296059 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 296054 [details] the patch Attachment 296054 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2615194 New failing tests: media/modern-media-controls/media-controller/media-controller-resize.html

Created attachment 296060 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

(In reply to comment #63) > Created attachment 296060 [details] > Archive of layout-test-results from ews101 for mac-yosemite > > The attached test failures were seen while running run-webkit-tests on the > mac-ews. > Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5 Seems unlikely that this is GC: Thread 21 Crashed:: com.apple.coremedia.player.async 0 libsystem_kernel.dylib 0x00007fff91e03286 __pthread_kill + 10 1 libsystem_c.dylib 0x00007fff8b5239a3 abort + 129 2 libsystem_malloc.dylib 0x00007fff9644dfe2 szone_error + 625 3 libsystem_malloc.dylib 0x00007fff96441d76 tiny_malloc_from_free_list + 1358 4 libsystem_malloc.dylib 0x00007fff964407e4 szone_malloc_should_clear + 317 5 libsystem_malloc.dylib 0x00007fff96440667 malloc_zone_malloc + 71 6 libsystem_malloc.dylib 0x00007fff9643f187 malloc + 42 7 com.apple.CoreMedia 0x00007fff8e4d4deb FigSimpleMutexCreateWithAttr + 20 8 com.apple.CoreMedia 0x00007fff8e4d4f67 FigReentrantMutexCreate + 69 9 com.apple.CoreMedia 0x00007fff8e4d4ff8 FigSemaphoreCreate + 46 10 com.apple.CoreMedia 0x00007fff8e518989 FigReadWriteLockCreate + 71 11 com.apple.CoreMedia 0x00007fff8e4bd2a6 figTimebaseCreate + 125 12 com.apple.CoreMedia 0x00007fff8e4be14b CMTimebaseCreateWithMasterClock + 54 13 com.apple.MediaToolbox 0x00007fff9796a283 FigPlayabilityMonitorCreate + 335 14 com.apple.MediaToolbox 0x00007fff979f02b6 0x7fff97929000 + 815798 15 com.apple.MediaToolbox 0x00007fff979ec305 0x7fff97929000 + 799493 16 com.apple.MediaToolbox 0x00007fff979e1825 0x7fff97929000 + 755749 17 com.apple.MediaToolbox 0x00007fff979e4790 0x7fff97929000 + 767888 18 com.apple.MediaToolbox 0x00007fff97a078aa 0x7fff97929000 + 911530 19 com.apple.MediaToolbox 0x00007fff979e0a59 0x7fff97929000 + 752217 20 com.apple.CoreMedia 0x00007fff8e4d5ef5 figThreadMain + 417 21 libsystem_pthread.dylib 0x00007fff9720205a _pthread_body + 131 22 libsystem_pthread.dylib 0x00007fff97201fd7 _pthread_start + 176 23 libsystem_pthread.dylib 0x00007fff971ff3ed thread_start + 13

Created attachment 296072 [details] last patch to have confirmed good perf Fixed another crash.

Attachment 296072 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2210: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296072 [details] last patch to have confirmed good perf Attachment 296072 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2620190 New failing tests: inspector/unit-tests/url-utilities.html

Created attachment 296080 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 296094 [details] possible new butterfly concurrency protocol Dunno if it even compiles.

Created attachment 296106 [details] new protocol is starting to work

(In reply to comment #70) > Created attachment 296106 [details] > new protocol is starting to work This isn't right - it breaks the compiler's ability to concurrently read objects. It breaks its ability to even do basic things like read object structures. One solution is to change how we nuke the structure ID. We could simply poison some bit in the structureID, which would still result in a useful structureID. Another solution is to have some other field - like a bit somewhere - get used as the thing that indicates badness. That seems harder.

Created attachment 296110 [details] and now more correct than ever

Created attachment 296112 [details] passing more tests

Created attachment 296114 [details] maybe?

Attachment 296114 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2250: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSCInlines.h:55: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 11 in 107 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 296115 [details] more fixes

Attachment 296115 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2250: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSCInlines.h:55: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 11 in 108 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 296116 [details] debug runs splay again

Attachment 296116 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2250: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSCInlines.h:55: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 11 in 108 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 296119 [details] more fixes

Attachment 296119 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2250: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296119 [details] more fixes Attachment 296119 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2624395 Number of test failures exceeded the failure limit.

Created attachment 296125 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 296126 [details] more

Created attachment 296127 [details] it's getting better My previous attempt at a build fix actually introduced a bug. :-( This should fix it.

Attachment 296127 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:354: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2250: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296072 [details] last patch to have confirmed good perf This patch has a bad race in JSObject::visitChildren. To fix it, I implemented things that could be a slow-down. I'm setting this aside for comparison if needed..

Created attachment 296149 [details] more fixes

Attachment 296149 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:356: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2256: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296072 [details] last patch to have confirmed good perf OK - I now have detailed perf data on the new, more correct, version.

Created attachment 296206 [details] with updated changelogs

Created attachment 296208 [details] the patch Rebased.

Created attachment 296209 [details] the patch

Created attachment 296210 [details] the patch For real this time.

Attachment 296210 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:356: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2256: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

My local debug RWT run showed no GC crashes. The Mac bot is red because of a EWS glitch, compile fail in WebCore code I didn't touch.

Created attachment 296294 [details] the patch

Attachment 296294 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:356: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2256: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:219: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 106 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 296294 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=296294&action=review r=me > Source/JavaScriptCore/ChangeLog:191 > + * runtime/Options.h: Change the default max GC parallelism to 8. I don't know why it was still 7. Now that we have good locks, I'm pretty sure we can bump to Inf without penalty. > Source/JavaScriptCore/runtime/JSObject.cpp:166 > + // structure before and after reading the butterfly. For simplicity, lets first consider the case let's

EWS mac-debug seems to be crashing reliably beneath sputnik/Conformance.

(In reply to comment #100) > EWS mac-debug seems to be crashing reliably beneath sputnik/Conformance. Yeah, I wished it crashed like that locally! I've now had two runs in a row that didn't crash.

(In reply to comment #101) > (In reply to comment #100) > > EWS mac-debug seems to be crashing reliably beneath sputnik/Conformance. > > Yeah, I wished it crashed like that locally! > > I've now had two runs in a row that didn't crash. I mean that didn't crash in GC. I get the same non-GC assertions on every run.

Created attachment 296365 [details] possible patch for landing

Created attachment 296376 [details] with fixes to stopAllocating

Created attachment 296379 [details] rebased patch

Attachment 296379 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:356: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2261: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:231: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 114 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 296467 [details] more fixes I found and fixed another race condition.

Attachment 296467 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:84: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/ForwardingHeaders/heap/DeleteAllCodeEffort.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:356: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/heap/Heap.cpp:2261: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:231: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/Structure.cpp:483: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:113: The parameter name "it" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/SparseArrayValueMap.h:114: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/LockDuringMarking.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 10 in 115 files If any of these errors are false positives, please file a bug against check-webkit-style.

Landed in http://trac.webkit.org/changeset/209570

(In reply to comment #109) > Landed in http://trac.webkit.org/changeset/209570 ARM buildfix landed in https://trac.webkit.org/changeset/209600

Comment on attachment 296467 [details] more fixes View in context: https://bugs.webkit.org/attachment.cgi?id=296467&action=review > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:127 > #endif > + } It is misplaced, it should be before the #endif.

(In reply to comment #111) > Comment on attachment 296467 [details] > more fixes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296467&action=review > > > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:127 > > #endif > > + } > > It is misplaced, it should be before the #endif. Fix landed in https://trac.webkit.org/changeset/209602

We don't have a lot of data points yet but this looks like a 2% regression on Dromaeo.

(In reply to comment #113) > We don't have a lot of data points yet but this looks like a 2% regression > on Dromaeo. Possible 3% regression on JSBench as well.

(In reply to comment #113) > We don't have a lot of data points yet but this looks like a 2% regression > on Dromaeo. Related: https://bugs.webkit.org/show_bug.cgi?id=165662

Note that when this re-landed in r209692, it was again a regression on several benchmarks it seems: - 1 to 2% on Dromaeo JSLib - 6% on JSBench