Bug 66827 – Unzip initialization lists and constructors in JSCell hierarchy (1/7)

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

[Mark Hahnenberg]

This is the first level (i.e. all of JSCell's immediately subclasses) of the unzipping process described in https://bugs.webkit.org/show_bug.cgi?id=66567.

[Mark Hahnenberg]

Created attachment 105118 [details]
Patch

[Mark Hahnenberg]

Created attachment 105204 [details]
Patch

[Darin Adler]

Comment on attachment 105204 [details]
Patch

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

I’m not sure constructorBody is a good name for this function if once we are done with this task it won’t be called inside C++ constructors any more. The name “constructor” belongs to the C++ language and has a specific meaning. Could we call these functions “initialize” or “finishInitialization” instead of “constructorBody”?

If someone makes the mistake of not calling the base class’s constructorBody function, is there some way we will quickly detect this at either compile time or runtime?

> Source/JavaScriptCore/runtime/StructureChain.h:72
> +            m_vector[i].clear();

Why is this needed? Wouldn’t that element in the vector already be 0?

[Mark Hahnenberg]

(In reply to comment #3)
> (From update of attachment 105204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105204&action=review
> 
> I’m not sure constructorBody is a good name for this function if once we are done with this task it won’t be called inside C++ constructors any more. The name “constructor” belongs to the C++ language and has a specific meaning. Could we call these functions “initialize” or “finishInitialization” instead of “constructorBody”?
> 

Geoff and I talked about this for a while, and we were never truly happy with the potential names we came up with.  The problem with associating these new functions with initialization is that it somewhat conflicts with the "isInitializing" flag name, since we're technically not doing initialization any more.  Initialization gets the object into a GC-safe state, and after that the constructor body runs.  Another option we considered was "finishAllocations".  We scrapped that one because the function doesn't *just* do allocations--it does whatever the old non-trival constructor body did.  The reason we settled for constructorBody() is because it seemed the closest to what these functions were actually replacing.  I guess we could still discuss it further or brainstorm better names.

> If someone makes the mistake of not calling the base class’s constructorBody function, is there some way we will quickly detect this at either compile time or runtime?

Yes, it will be caught at runtime. JSCell's constructorBody() will not be called if there is a break in the constructorBody() chain, therefore it will not clear the "isInitializing" flag and the next GC allocation will fail.  Unfortunately, it's not guaranteed that the failure will be anywhere near where the break in the constructorBody() chain is.

> > Source/JavaScriptCore/runtime/StructureChain.h:72
> > +            m_vector[i].clear();
> 
> Why is this needed? Wouldn’t that element in the vector already be 0?
 
I think you're right, since the default initialization of WriteBarrier is to set it to null.  I'm just moving the code around right now :-)  Should I file a separate bug on it, or is it worth it to make the intention of the programmer less implicit?

[Geoffrey Garen]

> The problem with associating these new functions with initialization is that it somewhat conflicts with
> the "isInitializing" flag name, since we're technically not doing initialization any more.  

I think a bigger problem with "initialize" as a term is that "initialize" is also a C++ term of art, which we'll continue to take advantage of. We want a C++ constructor to run initializers, putting an object to a GC-safe state from the C++ perspective, after which we want to do non-trivial work to fill in an object to a behaviorally correct state from the JS perspective.

Whatever happens in the function currently named "constructorBody", we want to clearly separate it from a prior step, which we'll call initialization.

Two other options:
    * a notification-style of naming: didConstruct(), didInitialize(), or didAllocate().
    * a command that reflects the caller being "create()": finishCreation().

I'm fine with any of those options. Mark, will you pick one?

> > If someone makes the mistake of not calling the base class’s constructorBody function, is there some way we will quickly detect this at either compile time or runtime?
> 
> Yes, it will be caught at runtime.

Specifically, an ASSERT will fire when the *next* object is allocated. Not perfect, but probably enough to catch any bug this fundamental.

> > > Source/JavaScriptCore/runtime/StructureChain.h:72
> > > +            m_vector[i].clear();
> > 
> > Why is this needed? Wouldn’t that element in the vector already be 0?
> 
> I think you're right, since the default initialization of WriteBarrier is to set it to null.  I'm just moving the code around right now :-)  Should I file a separate bug on it, or is it worth it to make the intention of the programmer less implicit?

Let's just take it out in this patch.

[Geoffrey Garen]

Comment on attachment 105204 [details]
Patch

r- since there are some minor modifications to make here.

[Mark Hahnenberg]

> Two other options:
>     * a notification-style of naming: didConstruct(), didInitialize(), or didAllocate().
>     * a command that reflects the caller being "create()": finishCreation().
> 
> I'm fine with any of those options. Mark, will you pick one?
> 
I think I like finishCreation() better, since the notification-style stuff makes me think more of delegates, and there's not really any delegation going on in this particular instance.

[Mark Hahnenberg]

Created attachment 105231 [details]
Change constructorBody to finishCreation

[Darin Adler]

Comment on attachment 105231 [details]
Change constructorBody to finishCreation

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

> Source/JavaScriptCore/runtime/JSString.h:312
> +        // This constructor constructs a new string by concatenating v1, v2 & v3.

Probably should reword the comment since it’s not a constructor any more. But I don’t want that to hold up this commit.

> Source/JavaScriptCore/runtime/JSString.h:330
> +            appendStringInConstruct(index, u1);

Seems like this function should be named appendStringDuringCreation now. Again, don’t want this to hold up this commit.

[Mark Hahnenberg]

Created attachment 105246 [details]
Fixing review issues

[Geoffrey Garen]

Comment on attachment 105246 [details]
Fixing review issues

r=me

[WebKit Review Bot]

Comment on attachment 105246 [details]
Fixing review issues

Clearing flags on attachment: 105246

Committed r93835: <http://trac.webkit.org/changeset/93835>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.