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

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 291385 [details]
the patch

[WebKit Commit Bot]

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 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]

(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]

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