Summary: | Heap::isMarked() shouldn't pay the price of concurrent lazy flipping | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, keith_miller, mark.lam, msaboff, ossy, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=161794 | ||||||||||||
Attachments: |
|
Description
Filip Pizlo
2016-09-08 14:07:19 PDT
Created attachment 288323 [details]
the patch
Created attachment 288326 [details]
the patch
Comment on attachment 288326 [details]
the patch
r=me
Comment on attachment 288326 [details] the patch Attachment 288326 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2036936 New failing tests: inspector/heap/tracking.html Created attachment 288345 [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 288348 [details]
better patch
The bots found one more isMarked.
Landed in https://trac.webkit.org/changeset/205683 (In reply to comment #7) > Landed in https://trac.webkit.org/changeset/205683 It made 2 JSC tests fail/timeout/crash/whatever on debug bots: - https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/3643 - https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/4032 - https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/10586 - https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20JSC%20%28Tests%29/builds/6940 The JSC test failures were due to an uninitialized isGCThread. Landed r205740: <http://trac.webkit.org/r205740> to add the null check. Note that there's already precedence for this type of null check in registerGCThread(). That said, I'm not sure that this is the right fix. The real issue is that initializeGCThreads() was never called by the jsc shell. With Fil's patch, is it ok to still allow isGCThread to be uninitialized? I suspect not, but I'll let Fil look into it further. (In reply to comment #9) > The JSC test failures were due to an uninitialized isGCThread. Landed > r205740: <http://trac.webkit.org/r205740> to add the null check. Note that > there's already precedence for this type of null check in registerGCThread(). > > That said, I'm not sure that this is the right fix. The real issue is that > initializeGCThreads() was never called by the jsc shell. With Fil's patch, > is it ok to still allow isGCThread to be uninitialized? I suspect not, but > I'll let Fil look into it further. Interesting. Looks like the problem is that we're only calling initializeMainThread() conditionally in jsc. (In reply to comment #10) > (In reply to comment #9) > > The JSC test failures were due to an uninitialized isGCThread. Landed > > r205740: <http://trac.webkit.org/r205740> to add the null check. Note that > > there's already precedence for this type of null check in registerGCThread(). > > > > That said, I'm not sure that this is the right fix. The real issue is that > > initializeGCThreads() was never called by the jsc shell. With Fil's patch, > > is it ok to still allow isGCThread to be uninitialized? I suspect not, but > > I'll let Fil look into it further. > > Interesting. Looks like the problem is that we're only calling > initializeMainThread() conditionally in jsc. Aha! Actually, I think that the real issue is that because the jsc shell never called initializeGCThreads(), my testing on the shell never caught the crash that the bots caught. That's because if initializeGCThreads() is never called, then isGCThread is null, so we never get to crash on **isGCThread because we'll fall out before we get there. The right fix is to do both what you did, and to call initializeMainThread() (or initializeGCThreads()) in jsc.cpp. (In reply to comment #11) > Aha! Actually, I think that the real issue is that because the jsc shell > never called initializeGCThreads(), my testing on the shell never caught the > crash that the bots caught. That's because if initializeGCThreads() is > never called, then isGCThread is null, so we never get to crash on > **isGCThread because we'll fall out before we get there. > > The right fix is to do both what you did, and to call initializeMainThread() > (or initializeGCThreads()) in jsc.cpp. Fil, based on the new GC changes, is it a requirement that isGCThread be properly initialized (as the GC depends on it for correctness)? The reason I ask is because I think the GTK port does not initialize isGCThread. If it is now a requirement, I would like to give them a heads up. |