Bug 174811 – GC should be fine with trading blocks between destructor and non-destructor blocks

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 316350 [details]
might be done

[Filip Pizlo]

Created attachment 316351 [details]
the patch

[Build Bot]

Attachment 316351 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedBlock.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 316351 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316351&action=review

> Source/JavaScriptCore/heap/MarkedBlock.cpp:46
> +
> +#include <wtf/Assertions.h>
> +
> +namespace {
> +
> +NO_RETURN_DUE_TO_CRASH NEVER_INLINE void crash()
> +{
> +    CRASH();
> +}
> +
> +#undef RELEASE_ASSERT
> +#define RELEASE_ASSERT(assertion) do { \
> +    if (!(assertion)) { \
> +        WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \
> +        crash(); \
> +    } \
> +} while (0)
> +
> +} // anonymous namespace
> +

I'll remove this.

[Mark Lam]

Comment on attachment 316351 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316351&action=review

r=me

> Source/JavaScriptCore/heap/MarkedBlockInlines.h:330
> +                    specializedSweep<true, NotEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale>(freeList, IsEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale, destroyFunc);
> +                    return true;
> +                case MarksStale:
> +                    specializedSweep<true, NotEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale>(freeList, IsEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale, destroyFunc);
> +                    return true;
> +                }
> +            case SweepToFreeList:
> +                switch (marksMode) {
> +                case MarksNotStale:
> +                    specializedSweep<true, NotEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale>(freeList, IsEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale, destroyFunc);
> +                    return true;
> +                case MarksStale:
> +                    specializedSweep<true, NotEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale>(freeList, IsEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale, destroyFunc);

Behavior-wise, it doesn't matter that you pass "IsEmpty" for the emptyMode (2nd argument) because it will get overridden to NotEmpty in specializedSweep() because the specialize flag is true.  However, for consistency, I think we should pass NotEmpty given that we know that the block's emptyMode is NotEmpty.

[Filip Pizlo]

(In reply to Mark Lam from comment #5)
> Comment on attachment 316351 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316351&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/heap/MarkedBlockInlines.h:330
> > +                    specializedSweep<true, NotEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale>(freeList, IsEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale, destroyFunc);
> > +                    return true;
> > +                case MarksStale:
> > +                    specializedSweep<true, NotEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale>(freeList, IsEmpty, SweepOnly, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale, destroyFunc);
> > +                    return true;
> > +                }
> > +            case SweepToFreeList:
> > +                switch (marksMode) {
> > +                case MarksNotStale:
> > +                    specializedSweep<true, NotEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale>(freeList, IsEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksNotStale, destroyFunc);
> > +                    return true;
> > +                case MarksStale:
> > +                    specializedSweep<true, NotEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale>(freeList, IsEmpty, SweepToFreeList, BlockHasDestructors, DontScribble, DoesNotHaveNewlyAllocated, MarksStale, destroyFunc);
> 
> Behavior-wise, it doesn't matter that you pass "IsEmpty" for the emptyMode
> (2nd argument) because it will get overridden to NotEmpty in
> specializedSweep() because the specialize flag is true.  However, for
> consistency, I think we should pass NotEmpty given that we know that the
> block's emptyMode is NotEmpty.

Fixed!

[Filip Pizlo]

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