Bug 125472 – ASSERT !heap.vm()->isInitializingObject() when finishing DFG compilation at beginning of GC

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

[Mark Hahnenberg]

We already assume that we might do some allocation during DFG plan finalization, so we just need to flip the proper bits so that the ASSERT doesn't fire in this situation.

[Mark Hahnenberg]

Created attachment 218812 [details]
Patch

[EFL EWS Bot]

Comment on attachment 218812 [details]
Patch

Attachment 218812 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/47238202

[EFL EWS Bot]

Comment on attachment 218812 [details]
Patch

Attachment 218812 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/45888244

[Build Bot]

Comment on attachment 218812 [details]
Patch

Attachment 218812 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/47248248

[Build Bot]

Comment on attachment 218812 [details]
Patch

Attachment 218812 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/47288220

[Build Bot]

Comment on attachment 218812 [details]
Patch

Attachment 218812 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/47228237

[Mark Hahnenberg]

Created attachment 218819 [details]
Patch

[EFL EWS Bot]

Comment on attachment 218819 [details]
Patch

Attachment 218819 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/46278321

[Geoffrey Garen]

Comment on attachment 218819 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/Heap.cpp:759
> +        PretendAllocationOkay pretend(*this);

I don't like the word "pretend" here because it implies that we're only silencing ASSERTs, when in reality we're making material changes that make the difference between allocating being OK or not. How about "RecursiveAllocationScope"?

> Source/JavaScriptCore/heap/Heap.cpp:760
> +        m_vm->prepareToDiscardCode();

Do we need to do this in every GC, or only if we're going to discard code? Seems weird, and unnecessarily costly, to do this if we're not going to discard code.

[Mark Hahnenberg]

Comment on attachment 218819 [details]
Patch

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

>> Source/JavaScriptCore/heap/Heap.cpp:759
>> +        PretendAllocationOkay pretend(*this);
> 
> I don't like the word "pretend" here because it implies that we're only silencing ASSERTs, when in reality we're making material changes that make the difference between allocating being OK or not. How about "RecursiveAllocationScope"?

OK.

>> Source/JavaScriptCore/heap/Heap.cpp:760
>> +        m_vm->prepareToDiscardCode();
> 
> Do we need to do this in every GC, or only if we're going to discard code? Seems weird, and unnecessarily costly, to do this if we're not going to discard code.

I think this is an unfortunately named method. We have an invariant right now that there can be no concurrent compilation going on during garbage collection, which is what "prepareToDiscardCode()" enforces, i.e. it finishes any remaining compilation work which then prevents any further concurrent compilation. I don't think this invariant is only related to discarding code. There are lots of assumptions in how concurrent compilation works that are built on top of the fact that a GC absolutely cannot occur in the middle of a compilation. For example, I believe LUB-ing of value profiles, array profiles, etc. depend on the fact that the object they grab from the profile is not a stale reference to a dead object.

[Mark Hahnenberg]

Committed r160377: <http://trac.webkit.org/changeset/160377>