Created attachment 343258 [details]
WIP
Going to run some memory tests before landing this.
Attachment 343258 [details] did not pass style-queue:
ERROR: Source/WebCore/page/FrameView.cpp:4134: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 343258 [details] WIP Attachment 343258 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8280282 New failing tests: editing/selection/navigation-clears-editor-state.html performance-api/performance-observer-no-document-leak.html fast/misc/test-observegc.html Created attachment 343266 [details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343258 [details] WIP Attachment 343258 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8280261 New failing tests: editing/selection/navigation-clears-editor-state.html performance-api/performance-observer-no-document-leak.html Created attachment 343267 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343258 [details] WIP Attachment 343258 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8280606 New failing tests: editing/selection/navigation-clears-editor-state.html performance-api/performance-observer-no-document-leak.html plugins/refcount-leaks.html fast/misc/test-observegc.html Created attachment 343274 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343258 [details] WIP Attachment 343258 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8280520 New failing tests: editing/selection/navigation-clears-editor-state.html performance-api/performance-observer-no-document-leak.html fast/misc/test-observegc.html Created attachment 343275 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 343258 [details] WIP Attachment 343258 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8281106 New failing tests: fast/dom/reference-cycle-leaks.html performance-api/performance-observer-no-document-leak.html fast/misc/test-observegc.html Created attachment 343277 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Seems like this is causing tests to fail that measure how many objects are left around. So it's probably an indication that the more conservative scan is keeping more around. I'm not sure how much we care about these layout test results. I'm going to look more at these tests. They seem like they are already prone to being flaky since some assert things about GC collecting objects. Created attachment 343368 [details]
patch
Comment on attachment 343368 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=343368&action=review r=me > Source/JavaScriptCore/ChangeLog:14 > + are Auxiliary. This means that if the stack were the only thing pointing to a /the stack were/the stack is/. "was" also works. > Source/JavaScriptCore/heap/HeapUtil.h:107 > + func(pointer, candidate->handle().cellKind()); Why re-compute the cellKind() every time here? The candidate doesn't change. Hence, the cellKind() shouldn't change either, no? Oh, I see, cellKind() is only used in here. I noticed that candidate->handle() is computed and used multiple times. Can you pre-compute and cache it above instead? Comment on attachment 343368 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=343368&action=review >> Source/JavaScriptCore/heap/HeapUtil.h:107 >> + func(pointer, candidate->handle().cellKind()); > > Why re-compute the cellKind() every time here? The candidate doesn't change. Hence, the cellKind() shouldn't change either, no? > > Oh, I see, cellKind() is only used in here. I noticed that candidate->handle() is computed and used multiple times. Can you pre-compute and cache it above instead? nit: Oh wait, we still use this lambda in more than one place. I think we can still precache the cellKind. Comment on attachment 343368 [details] patch Attachment 343368 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8296440 New failing tests: editing/selection/navigation-clears-editor-state.html Created attachment 343385 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343368 [details] patch Attachment 343368 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8296513 New failing tests: editing/selection/navigation-clears-editor-state.html Created attachment 343386 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343368 [details] patch Attachment 343368 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8296256 New failing tests: editing/selection/navigation-clears-editor-state.html plugins/refcount-leaks.html Created attachment 343388 [details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
(In reply to Build Bot from comment #20) > Comment on attachment 343368 [details] > patch > > Attachment 343368 [details] did not pass mac-ews (mac): > Output: https://webkit-queues.webkit.org/results/8296513 > > New failing tests: > editing/selection/navigation-clears-editor-state.html These tests are vulnerable to conservative roots skewing them Comment on attachment 343368 [details] patch Attachment 343368 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8296375 New failing tests: editing/selection/navigation-clears-editor-state.html Created attachment 343391 [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.13.4
Comment on attachment 343368 [details] patch Attachment 343368 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8299011 New failing tests: fast/dom/reference-cycle-leaks.html Created attachment 343412 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 343457 [details]
patch for landing if EWS passes
Comment on attachment 343457 [details] patch for landing if EWS passes Attachment 343457 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8308669 New failing tests: plugins/refcount-leaks.html Created attachment 343461 [details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343457 [details] patch for landing if EWS passes Attachment 343457 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8309118 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html Created attachment 343464 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 343477 [details]
patch for landing if EWS passes #2
Comment on attachment 343477 [details] patch for landing if EWS passes #2 Attachment 343477 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8320540 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 343484 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 343477 [details] patch for landing if EWS passes #2 Attachment 343477 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8325913 New failing tests: plugins/refcount-leaks.html Created attachment 343489 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 343523 [details]
patch for landing attempt #3
Created attachment 343546 [details]
patch for landing
Comment on attachment 343546 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=343546&action=review > LayoutTests/editing/selection/navigation-clears-editor-state.html:62 > + if (delta > 60) Maybe we should do count * 0.3 instead? (In reply to Ryosuke Niwa from comment #41) > Comment on attachment 343546 [details] > patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343546&action=review > > > LayoutTests/editing/selection/navigation-clears-editor-state.html:62 > > + if (delta > 60) > > Maybe we should do count * 0.3 instead? Otherwise the change to this test looks good. (In reply to Ryosuke Niwa from comment #42) > (In reply to Ryosuke Niwa from comment #41) > > Comment on attachment 343546 [details] > > patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=343546&action=review > > > > > LayoutTests/editing/selection/navigation-clears-editor-state.html:62 > > > + if (delta > 60) > > > > Maybe we should do count * 0.3 instead? > > Otherwise the change to this test looks good. Sounds good. I'll make this change. I'm also going to bump the timeout up a bit. Created attachment 343552 [details]
patch for landing
Comment on attachment 343552 [details] patch for landing Clearing flags on attachment: 343552 Committed r233184: <https://trac.webkit.org/changeset/233184> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 187059 Seems like I need to use our other approach of introducing a new HeapCell::Kind called something like JSCellWithInteriorPointers Created attachment 343645 [details]
WIP
want to test it on EWS
Created attachment 343653 [details]
patch
Comment on attachment 343653 [details] patch Clearing flags on attachment: 343653 Committed r233236: <https://trac.webkit.org/changeset/233236> All reviewed patches have been landed. Closing bug. |
This breaks our conservative marking of the object. Check out this code: HeapUtil::findGCObjectPointersForMarking( m_heap, markingVersion, newlyAllocatedVersion, filter, p, [&] (void* p, HeapCell::Kind cellKind) { if (cellKind == HeapCell::JSCell) markHook.markKnownJSCell(static_cast<JSCell*>(p)); if (m_size == m_capacity) grow(); m_roots[m_size++] = bitwise_cast<HeapCell*>(p); }); I think I'll also need to extend findGCObjectPointersForMarking a bit. It does some stuff conditionally on Auxiliary for butterflies, but here, we'll have a kind of Cell for JSImmutableButterfly. I wonder if we should change up how we use JSImmutableButterfly to act more like other butterflies, and obviate this having to be different than other butterflies.