Bug 168257 – Add JSC_sweepSynchronously and fix JSC_useZombieMode options.

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

[Mark Lam]

Patch coming.

[Mark Lam]

<rdar://problem/30451496>

[Mark Lam]

Created attachment 301396 [details]
proposed patch.

Let's get some EWS testing on this.

[Mark Lam]

Comment on attachment 301396 [details]
proposed patch.

Looks ready for a review.

[Michael Saboff]

Comment on attachment 301396 [details]
proposed patch.

r=me

[Mark Lam]

Comment on attachment 301396 [details]
proposed patch.

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

> Source/JavaScriptCore/heap/Heap.cpp:1060
> +        sweepSynchronously();
> +        if (Options::useZombieMode())
> +            zombifyDeadObjects();

Turns out this is not needed because collectSync() above will take care of invoking sweepSynchronously() and zombifyDeadObjects() if needed.  I'll also add some GC logging when zombifying.  Updated patch coming soon.

[Mark Lam]

Comment on attachment 301396 [details]
proposed patch.

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

>> Source/JavaScriptCore/heap/Heap.cpp:1060
>> +            zombifyDeadObjects();
> 
> Turns out this is not needed because collectSync() above will take care of invoking sweepSynchronously() and zombifyDeadObjects() if needed.  I'll also add some GC logging when zombifying.  Updated patch coming soon.

Correction: sweepSyncronously is still needed if neither Options::sweepSynchronously() or Options::useZombieMode() are set because collectSync() does not sweep synchronously by default.  Will fix.

[Mark Lam]

Created attachment 301428 [details]
proposed patch with fixes.

[Saam Barati]

Comment on attachment 301428 [details]
proposed patch with fixes.

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

LGTM

> Source/JavaScriptCore/heap/Heap.cpp:1043
> +        dataLog("=> ", capacity() / 1024, "kb, ", after - before, "ms] ");

Why not put the "\n" here?

> Source/JavaScriptCore/heap/Heap.cpp:1980
> +        dataLog("=> ", capacity() / 1024, "kb, ", after - before, "ms] ");

ditto

[Mark Lam]

Comment on attachment 301428 [details]
proposed patch with fixes.

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

Thanks for the review.

>> Source/JavaScriptCore/heap/Heap.cpp:1043
>> +        dataLog("=> ", capacity() / 1024, "kb, ", after - before, "ms] ");
> 
> Why not put the "\n" here?

Because this function may be called from Heap::finalize() which has its own logging which wraps this line.  Putting a '\n' here will premature terminate the "[GC: finalize ...]" logging in Heap::finalize().

>> Source/JavaScriptCore/heap/Heap.cpp:1980
>> +        dataLog("=> ", capacity() / 1024, "kb, ", after - before, "ms] ");
> 
> ditto

Same as above.

[Mark Lam]

Created attachment 301526 [details]
proposed patch.

One more time: this time, re-define useZombieMode as the combination of sweepSynchronously + scribbleFreeCells.

[Mark Lam]

Thanks for the reviews.  Landed in r212310: <http://trac.webkit.org/r212310>.