Bug 66827

Summary: Unzip initialization lists and constructors in JSCell hierarchy (1/7)
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 65288    
Bug Blocks: 66567, 66957    
Attachments:
Description Flags
Patch
none
Patch
none
Change constructorBody to finishCreation
none
Fixing review issues none

Description Mark Hahnenberg 2011-08-23 17:27:22 PDT
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.
Comment 1 Mark Hahnenberg 2011-08-24 19:04:19 PDT
Created attachment 105118 [details]
Patch
Comment 2 Mark Hahnenberg 2011-08-25 10:00:07 PDT
Created attachment 105204 [details]
Patch
Comment 3 Darin Adler 2011-08-25 10:58:28 PDT
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?
Comment 4 Mark Hahnenberg 2011-08-25 11:27:58 PDT
(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?
Comment 5 Geoffrey Garen 2011-08-25 11:50:04 PDT
> 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.
Comment 6 Geoffrey Garen 2011-08-25 11:50:34 PDT
Comment on attachment 105204 [details]
Patch

r- since there are some minor modifications to make here.
Comment 7 Mark Hahnenberg 2011-08-25 11:58:35 PDT
> 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.
Comment 8 Mark Hahnenberg 2011-08-25 12:22:48 PDT
Created attachment 105231 [details]
Change constructorBody to finishCreation
Comment 9 Darin Adler 2011-08-25 14:06:22 PDT
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.
Comment 10 Mark Hahnenberg 2011-08-25 14:36:35 PDT
Created attachment 105246 [details]
Fixing review issues
Comment 11 Geoffrey Garen 2011-08-25 15:23:24 PDT
Comment on attachment 105246 [details]
Fixing review issues

r=me
Comment 12 WebKit Review Bot 2011-08-25 16:30:27 PDT
Comment on attachment 105246 [details]
Fixing review issues

Clearing flags on attachment: 105246

Committed r93835: <http://trac.webkit.org/changeset/93835>
Comment 13 WebKit Review Bot 2011-08-25 16:30:33 PDT
All reviewed patches have been landed.  Closing bug.