Bug 66265 – Move allocation in constructors into separate constructorBody() methods

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

[Mark Hahnenberg]

This is the next small step in the process of refactoring JSC constructors to do no allocation in the middle of the execution of an initialization list.  In this bug, we're going to pull the allocation done in the constructor bodies into separate finishAllocations() methods, which will in turn be called from the constructor bodies.  This will set us up to have those finishAllocations() methods called outside of the constructor, allowing us to avoid allocation during initialization.

[Mark Hahnenberg]

Created attachment 103980 [details]
Patch

[Mark Hahnenberg]

Created attachment 103982 [details]
Patch

[Geoffrey Garen]

Comment on attachment 103982 [details]
Patch

Patch looks good.

I think the simplest idiom to understand and enforce is "no constructor bodies or non-trivial initializers in GC objects." Then, this extra function you've added functions as the constructor body. So, let's call it "constructorBody".

r=me with that change.

[Mark Hahnenberg]

Created attachment 104216 [details]
Change function name

[Mark Hahnenberg]

Created attachment 104419 [details]
Fixing windows build errors

[Oliver Hunt]

Comment on attachment 104419 [details]
Fixing windows build errors

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

> Source/JavaScriptCore/runtime/JSPropertyNameIterator.h:90
> +            PropertyNameArrayData::PropertyNameVector& propertyNameVector = propertyNameArrayData->propertyNameVector();

Out of curiosity, how does JSPropertyNameIterator handle gc during initialisation -- eg. if jsOwnedString triggers a gc?

[Mark Hahnenberg]

> Out of curiosity, how does JSPropertyNameIterator handle gc during initialisation -- eg. if jsOwnedString triggers a gc?

Poorly I think.  visitChildren appends all of the values in m_jsStrings to the visitor, so if you're only halfway through creating the list, I think it has a potential to blow up.

[WebKit Review Bot]

Comment on attachment 104419 [details]
Fixing windows build errors

Clearing flags on attachment: 104419

Committed r93378: <http://trac.webkit.org/changeset/93378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.