Description
Filip Pizlo
2016-10-25 14:28:46 PDT
Created attachment 292820 [details]
it begins
Comment on attachment 292820 [details]
it begins
It's nowhere near ready for review.
Attachment 292820 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:36: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 292841 [details]
getting closer
Relevant: https://github.com/tc39/ecmascript_sharedmem/issues/38#issuecomment-163048837 I think that we want to make it harder to unwrap an ArrayBuffer if you haven't made a statement about whether you're happy sharing. Created attachment 292874 [details]
more
Created attachment 292955 [details]
more
Created attachment 292994 [details]
a bit more
Created attachment 293023 [details]
more
I'm renaming all of the functions that give native code access to a typed array's guts. For each toBlahTypedArray()/toArrayBuffer() function, we will now have two:
toUnsharedBlah() -> same as the old toBlah() but asserts that the underlying buffer is unshared, or in the case of versions that dynamic cast, returns zero if the underlying buffer is unshared.
toPossiblySharedBlah() -> same as the old toBlah() but may return something that has memory that is aliased with another thread.
Created attachment 293028 [details]
rebased patch
Created attachment 293094 [details]
more
Created attachment 293113 [details]
it's starting to compile
Created attachment 293165 [details]
closer to compiling
Created attachment 293172 [details]
jsc compiles
Created attachment 293181 [details]
making the DOM happy
Created attachment 293188 [details]
working through more DOM uses of typed arrays
Created attachment 293195 [details]
more of the DOM compiles
Created attachment 293273 [details]
more of the DOM compiles
Created attachment 293299 [details]
the DOM compiles
Created attachment 293307 [details]
it all builds
It builds and SharedArrayBuffer is behind a runtime flag. I haven't tested it yet.
Created attachment 293309 [details]
rebased patch
Attachment 293309 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 69 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293311 [details]
more fixes
Attachment 293311 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 70 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293324 [details]
it seems to work
Created attachment 293325 [details]
rebased patch
Attachment 293325 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:100: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293326 [details]
better changelog
Attachment 293326 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:100: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293326 [details] better changelog Attachment 293326 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2403208 Number of test failures exceeded the failure limit. Created attachment 293329 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 293326 [details] better changelog Attachment 293326 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2403207 Number of test failures exceeded the failure limit. Created attachment 293330 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 293326 [details] better changelog Attachment 293326 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2403205 Number of test failures exceeded the failure limit. Created attachment 293331 [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
Comment on attachment 293326 [details] better changelog Attachment 293326 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2403217 Number of test failures exceeded the failure limit. Created attachment 293333 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Ooops the crashes look like a missing null check. Created attachment 293352 [details]
some fixes
toUnsharedArrayBufferView was missing a null check.
Created attachment 293353 [details]
rebased
Attachment 293353 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293354 [details]
fix gcc build
Attachment 293354 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293355 [details]
the patch
Attachment 293355 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293356 [details]
the patch
Attachment 293356 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293358 [details]
the patch
Attachment 293358 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:184: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Looks like I messed up ArrayBuffer transfer for the non-shared case. Comment on attachment 293358 [details] the patch Attachment 293358 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2406561 New failing tests: js/typed-array-mutated-during-set.html workers/sab/simple.html fast/dom/Window/window-postmessage-args.html fast/canvas/webgl/arraybuffer-transfer-of-control.html js/dom/global-constructors-attributes-dedicated-worker.html js/dom/dfg-byteOffset-neuter.html Created attachment 293362 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 293358 [details] the patch Attachment 293358 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2406562 New failing tests: js/dom/global-constructors-attributes.html workers/sab/simple.html js/dom/dfg-byteOffset-neuter.html fast/canvas/webgl/arraybuffer-transfer-of-control.html js/dom/global-constructors-attributes-dedicated-worker.html fast/dom/Window/window-postmessage-args.html js/typed-array-mutated-during-set.html Created attachment 293364 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 293358 [details] the patch Attachment 293358 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2406565 New failing tests: js/typed-array-mutated-during-set.html workers/sab/simple.html fast/dom/Window/window-postmessage-args.html fast/canvas/webgl/arraybuffer-transfer-of-control.html js/dom/global-constructors-attributes-dedicated-worker.html js/dom/dfg-byteOffset-neuter.html Created attachment 293366 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 293358 [details] the patch Attachment 293358 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2406568 New failing tests: js/dom/dfg-byteOffset-neuter.html fast/dom/Window/window-postmessage-args.html js/dom/global-constructors-attributes-dedicated-worker.html js/typed-array-mutated-during-set.html workers/sab/simple.html Created attachment 293367 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
I'm debugging the crashes. They are subtle. An array buffer view's vector goes null at some point, don't know why yet. (In reply to comment #59) > I'm debugging the crashes. They are subtle. An array buffer view's vector > goes null at some point, don't know why yet. Found it. ArrayBuffer::transferTo() needs to clear() this. Failing to do so creates all manner of problems. Basically, the vector was supposed to be null because of neutering, but our logic for handling neutered array buffers wasn't working right because the ArrayBuffer itself had not been neutered (because the of the missing clear call). Created attachment 293391 [details]
the patch
I fixed the crash in neutering.
Attachment 293391 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 93 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293391 [details] the patch Attachment 293391 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2409241 New failing tests: js/dom/global-constructors-attributes-dedicated-worker.html workers/sab/simple.html Created attachment 293392 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 293391 [details] the patch Attachment 293391 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2409260 New failing tests: js/dom/global-constructors-attributes.html js/dom/global-constructors-attributes-dedicated-worker.html media/modern-media-controls/volume-support/volume-support-click.html workers/sab/simple.html Created attachment 293393 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 293391 [details] the patch Attachment 293391 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2409254 New failing tests: js/dom/global-constructors-attributes-dedicated-worker.html workers/sab/simple.html Created attachment 293394 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 293391 [details] the patch Attachment 293391 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2409276 New failing tests: js/dom/global-constructors-attributes-dedicated-worker.html workers/sab/simple.html Created attachment 293395 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 293423 [details]
the patch
Attachment 293423 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 95 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293426 [details]
the patch
Attachment 293426 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 95 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293426 [details] the patch Attachment 293426 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2411984 New failing tests: js/dom/global-constructors-attributes.html Created attachment 293432 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 293434 [details]
more tests WIP
Comment on attachment 293426 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=293426&action=review Some comments. Mostly looks good. I'll hold off on the final review until the tests are done though. > Source/JavaScriptCore/ChangeLog:20 > + There is also a Atomics object in global scope, which exposes sequentiall consistent atomic typo: a Atomcis => an Atomics. sequentiall => sequentially. > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:109 > + ASSERT(policy == DontInitialize); Is this assert really necessary? > Source/JavaScriptCore/runtime/ArrayBuffer.h:188 > + // FIXME: We probably want to scale this by the shared ref count or something. yikes, this seems unfortunate. > Source/JavaScriptCore/runtime/AtomicsObject.cpp:322 > + // FIXME: Stop using std::chrono. File a bug! :) (In reply to comment #78) > Comment on attachment 293426 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293426&action=review > > Some comments. Mostly looks good. I'll hold off on the final review until > the tests are done though. > > > Source/JavaScriptCore/ChangeLog:20 > > + There is also a Atomics object in global scope, which exposes sequentiall consistent atomic > > typo: a Atomcis => an Atomics. sequentiall => sequentially. Fixed. > > > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:109 > > + ASSERT(policy == DontInitialize); > > Is this assert really necessary? I think it was there before I moved that code from ArrayBuffer.h > > > Source/JavaScriptCore/runtime/ArrayBuffer.h:188 > > + // FIXME: We probably want to scale this by the shared ref count or something. > > yikes, this seems unfortunate. > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:322 > > + // FIXME: Stop using std::chrono. > > File a bug! :) I think we have one. I'll find it. Comment on attachment 293426 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=293426&action=review > Source/JavaScriptCore/ChangeLog:20 > + There is also a Atomics object in global scope, which exposes sequentiall consistent atomic Typo "sequential" > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:95 > +{ Should this set `m_shared = nullptr;` at the beginning? Or assert on it? > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:101 > + m_data = nullptr; It seems like setting `m_sizeInBytes = 0;` on both failure paths would be conservatively good. > Source/JavaScriptCore/runtime/ArrayBufferSharingMode.h:33 > + Default, Why not "Unshared"? That seems more explicit. > Source/JavaScriptCore/runtime/AtomicsObject.cpp:229 > + case 3: case 3 should return false, and case 4 should return true (along with 1 and 2). > Source/JavaScriptCore/runtime/AtomicsObject.cpp:305 > + double timeoutInNanoseconds = timeoutInMilliseconds * 1000; Missing 1000? > Source/JavaScriptCore/runtime/AtomicsObject.cpp:315 > + if (timeoutInNanoseconds == timeoutInNanoseconds) std::isnan > Source/JavaScriptCore/runtime/AtomicsObject.cpp:316 > + timeoutInNanoseconds = std::max(0., timeoutInNanoseconds); This seems to be a spec thing: is -0. a valid thing here? > Source/WTF/wtf/Atomics.h:-96 > - ASSERT(bitwise_cast<std::atomic<T>*>(location)->is_lock_free() && "expected lock-free type"); I think you want to keep these asserts. (In reply to comment #80) > Comment on attachment 293426 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293426&action=review > > > Source/JavaScriptCore/ChangeLog:20 > > + There is also a Atomics object in global scope, which exposes sequentiall consistent atomic > > Typo "sequential" Fixed. > > > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:95 > > +{ > > Should this set `m_shared = nullptr;` at the beginning? Or assert on it? I think it's already certain that this function is only called right after ArrayBufferContents is instantiated. > > > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:101 > > + m_data = nullptr; > > It seems like setting `m_sizeInBytes = 0;` on both failure paths would be > conservatively good. I made it call reset(). > > > Source/JavaScriptCore/runtime/ArrayBufferSharingMode.h:33 > > + Default, > > Why not "Unshared"? That seems more explicit. To reflect the naming of the types. It's ArrayBuffer, not UnsharedArrayBuffer, and "Default" seems to be closer to meaning "the ArrayBuffer that isn't special". > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:229 > > + case 3: > > case 3 should return false, and case 4 should return true (along with 1 and > 2). Oops. > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:305 > > + double timeoutInNanoseconds = timeoutInMilliseconds * 1000; > > Missing 1000? Oops. > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:315 > > + if (timeoutInNanoseconds == timeoutInNanoseconds) > > std::isnan == is a fundamental operator in C++ that already answers this question. I don't think we should mandate using helpers for operations that are already primitive. > > > Source/JavaScriptCore/runtime/AtomicsObject.cpp:316 > > + timeoutInNanoseconds = std::max(0., timeoutInNanoseconds); > > This seems to be a spec thing: is -0. a valid thing here? The spec wants +0 here, but it doesn't matter. > > > Source/WTF/wtf/Atomics.h:-96 > > - ASSERT(bitwise_cast<std::atomic<T>*>(location)->is_lock_free() && "expected lock-free type"); > > I think you want to keep these asserts. I removed them because I didn't want this noise everywhere. If we did want an assertion about the lock freedom of primitive types, we should probably put them somewhere else. Created attachment 293459 [details]
the patch
Attachment 293459 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 105 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 293460 [details]
the patch
Attachment 293460 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 106 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293460 [details] the patch Attachment 293460 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2414059 New failing tests: js/dom/global-constructors-attributes.html Created attachment 293473 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 293478 [details]
the patch
Attachment 293478 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:101: Declaration has space between type name and * in const JSWorkerGlobalScopeBase *thisObject [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:115: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayBuffer.cpp:185: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.cpp:36: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:163: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSArrayBufferView.h:164: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 6 in 108 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 293478 [details]
the patch
r=me. Assuming the bots are happy and you intend on importing more tests since this current testing seems a little light.
This patch passes everything for me locally. I think that EWS is seeing unrelated crashes. But I'll wait a bit before landing. Landed in https://trac.webkit.org/changeset/208209 This patch broke bindings tests: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/1208 Fixing. |