Bug 163343

Summary:

The blackening of CellState is a bad way of tracking if the object is being marked for the first time

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, keith_miller, mark.lam, msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

149432

Attachments:

Description	Flags
the patch	mark.lam: review+

Filip Pizlo

Reported 2016-10-12 11:15:41 PDT

I think we need to revert the CellState change in https://trac.webkit.org/changeset/206344.

Attachments
the patch (12.45 KB, patch) 2016-10-12 12:53 PDT, Filip Pizlo	mark.lam: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2016-10-12 12:53:07 PDT

Created attachment 291385 [details] the patch

WebKit Commit Bot

Comment 2 2016-10-12 12:56:05 PDT

Attachment 291385 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/llint/LLIntData.cpp:217: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Mark Lam

Comment 3 2016-10-12 13:07:23 PDT

Comment on attachment 291385 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=291385&action=review r=me > Source/JavaScriptCore/ChangeLog:13 > + new way better: instead of the SlotVisitor rememering the state-before-blackening, we would typo: /rememering/remembering/ > Source/JavaScriptCore/ChangeLog:19 > + Subsequent visits will know that they are not the first. So, we will fail to do the right /will know/will think/? > Source/JavaScriptCore/heap/CellState.h:33 > + // The object is either currently being scanned (anthracite) or it has finished being scalled typo: /scalled/scanned/ > Source/JavaScriptCore/heap/SlotVisitor.cpp:300 > + // There is no race here - the cell state cannot change right now. > + ASSERT(m_oldCellState == CellState::OldGrey || m_oldCellState == CellState::NewGrey); Please explain why the cell state cannot change.

Filip Pizlo

Comment 4 2016-10-12 13:09:06 PDT

(In reply to comment #3) > Comment on attachment 291385 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291385&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:13 > > + new way better: instead of the SlotVisitor rememering the state-before-blackening, we would > > typo: /rememering/remembering/ > > > Source/JavaScriptCore/ChangeLog:19 > > + Subsequent visits will know that they are not the first. So, we will fail to do the right > > /will know/will think/? > > > Source/JavaScriptCore/heap/CellState.h:33 > > + // The object is either currently being scanned (anthracite) or it has finished being scalled > > typo: /scalled/scanned/ > > > Source/JavaScriptCore/heap/SlotVisitor.cpp:300 > > + // There is no race here - the cell state cannot change right now. > > + ASSERT(m_oldCellState == CellState::OldGrey || m_oldCellState == CellState::NewGrey); > > Please explain why the cell state cannot change. All fixed. Now to run all debug tests to make sure I didn't add wrong asserts...

Filip Pizlo

Comment 5 2016-10-12 16:59:39 PDT

Landed in https://trac.webkit.org/changeset/207263

Note You need to log in before you can comment on or make changes to this bug.