Bug 64466 – Refactor JSC to replace JSCell::operator new with static create method

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

[Mark Hahnenberg]

This is the first of a series of bugs to refactor JSC and its related DOM bindings to use static create methods when creating heap-allocated JS objects.  The goal of this bug is simply to replace all instances of JSCell::operator new within JSC with the new static create methods.

[Mark Hahnenberg]

Created attachment 100695 [details]
Proposed patch.

[WebKit Review Bot]

Attachment 100695 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1

Source/JavaScriptCore/runtime/StringConstructor.h:32:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpConstructor.h:60:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/NumberConstructor.h:32:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 100695 [details]
Proposed patch.

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

> Source/JavaScriptCore/API/JSCallbackConstructor.h:36
> +        JSCallbackConstructor(JSGlobalObject*, Structure*, JSClassRef, JSObjectCallAsConstructorCallback);

Incorrect indentation here.

> Source/JavaScriptCore/API/JSCallbackConstructor.h:42
> +        JSCallbackConstructor* result = static_cast<JSCallbackConstructor*>(exec->heap()->allocate(sizeof(JSCallbackConstructor)));
> +        result->m_structure.clear();
> +        return new (result) JSCallbackConstructor(globalObject, structure, classRef, callback);

Before we repeat this idiom everywhere, I suggest we:

    1) Make a function that combines the allocation and the clear in a single function call.
    2) Make a function template that takes the type as an argument and does the sizeof and typecasting.

Then this function would be a one-liner:

    return new (allocateJSObject<JSCallbackConstructor>(exec)) JSCallbackConstructor(globalObject, structure, classRef, callback);

[Oliver Hunt]

Comment on attachment 100695 [details]
Proposed patch.

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

In general this patch is correct, there are just a few overall refinements i'd recommend

* You don't need to clear the structure everytime, that's only needed as a workaround for the existing sequencing issues.
* Given you don't need to clear the structure a single function along the lines of template <typename T> void* allocateCell<T>(exec/globalData/heap/whatever) can just be used for all allocations, and the create methods become 
return new (allocateCell<type goes here>(exec/globalData/whatever)) Constructor(...)

That should simplify the creation routines quite a bit.

If you use the webkit-patch script it will run the style checker before uploading so you can correct any style issues before submitting for review (the commit queue is meant to reject patches with style errors so theoretically wouldn't accept a patch with such errors)

> Source/JavaScriptCore/ChangeLog:6
> +

There needs to be a description of the change here, look for other changelog entries for examples of the level of detail that's expected.  While this is mostly a mechanical change, so doesn't require huge amounts of elaboration, we typically expect a reasonable amount of detail from new-ish contributors.

> Source/JavaScriptCore/API/JSCallbackConstructor.h:41
> +        result->m_structure.clear();

You don't need these calls to clear the structure -- they exist in operator new() to mitigate the problem that this patch is fixing.

> Source/JavaScriptCore/API/JSCallbackObject.h:136
> +    static JSCallbackObject* allocCallbackObj(ExecState* exec)
> +    {
> +        JSCallbackObject* result = static_cast<JSCallbackObject*>(exec->heap()->allocate(sizeof(JSCallbackObject)));
> +        result->m_structure.clear();
> +        return result;
> +    }
> +public:
> +    static JSCallbackObject* create(ExecState* exec, JSGlobalObject* globalObject, Structure* structure, JSClassRef classRef, void* data)
> +    {
> +        return new (allocCallbackObj(exec)) JSCallbackObject(exec, globalObject, structure, classRef, data);
> +    }
> +    static JSCallbackObject* create(JSGlobalData* globalData, JSClassRef classRef, Structure* structure)
> +    {
> +        return new (globalData) JSCallbackObject(*globalData, classRef, structure);
> +    }

i think allocCallbackObj can be private, and you should kill off the new (globalData) usage

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:585
> +    JSArray* resObj = JSArray::create(exec, exec->globalData(), exec->lexicalGlobalObject()->arrayStructure(), deleteCount, CreateCompact);

The additional exec shouldn't be necessary.

> Source/JavaScriptCore/runtime/BooleanObject.h:33
> +        static BooleanObject* create(ExecState* exec, JSGlobalData& globalData, Structure* structure)

You don't need the exec parameter -- just use globalData.heap (exec->heap() is actually doing exec->globalData().heap)

> Source/JavaScriptCore/runtime/DateInstance.h:37
> +        static DateInstance* allocDateInstance(ExecState* exec)

This can probably be private.

> Source/JavaScriptCore/runtime/Error.cpp:172
> +    : InternalFunction(&exec->globalData(), globalObject, structure, exec->globalData().propertyNames->emptyIdentifier)
> +    , m_message(message)

should be indented

> Source/JavaScriptCore/runtime/Executable.h:327
> +            ProgramExecutable* result = static_cast<ProgramExecutable*>(exec->heap()->allocate(sizeof(ProgramExecutable)));

The more i look at this pattern the more i wonder if we want to add a void* allocate<T>() { return allocate(sizeof(T)); } style function to heap.

Also these casts are unnecessary, void* is fine (as you don't need to clear the structure)

> Source/JavaScriptCore/runtime/GetterSetter.h:43
> +        : JSCell(exec->globalData(), exec->globalData().getterSetterStructure.get())
> +        {
> +        }

indentation

> Source/JavaScriptCore/runtime/JSCell.h:89
>      protected:
>          enum VPtrStealingHackType { VPtrStealingHack };
> -
> +        
>      private:
>          explicit JSCell(VPtrStealingHackType) { }
>          JSCell(JSGlobalData&, Structure*);
>          JSCell(JSGlobalData&, Structure*, CreatingEarlyCellTag);
>          virtual ~JSCell();
>          static const ClassInfo s_dummyCellInfo;
> -
> +        
> +        

Random whitespace changes == badness

> Source/JavaScriptCore/runtime/JSCell.h:-168
> +        WriteBarrier<Structure> m_structure;
>  
>      private:
>          // Base implementation; for non-object classes implements getPropertySlot.
>          virtual bool getOwnPropertySlot(ExecState*, const Identifier& propertyName, PropertySlot&);
>          virtual bool getOwnPropertySlot(ExecState*, unsigned propertyName, PropertySlot&);
>          
> -        WriteBarrier<Structure> m_structure;

This should be unnecessary once you remove the extraneous calls to clear()

[Mark Hahnenberg]

Created attachment 100754 [details]
Patch

[WebKit Review Bot]

Attachment 100754 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1

Source/JavaScriptCore/runtime/StringConstructor.h:32:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpConstructor.h:60:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/NumberConstructor.h:32:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

On a side note, I don't think that I added the additional parameters that the style bot is talking about.  Superficially it does appear that they aren't used, but I didn't want to mess with it right now.

[Mark Hahnenberg]

Never mind, upon closer scrutiny I guess I did accidentally add the parameter name.

[Mark Hahnenberg]

Created attachment 100755 [details]
Patch

[Early Warning System Bot]

Comment on attachment 100755 [details]
Patch

Attachment 100755 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9019859

[Darin Adler]

Comment on attachment 100755 [details]
Patch

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

This looks good.

There are some calls to new inside WebCore/bridge/qt/qt_runtime.cpp that need to be changed to use create instead.

review+ but you’ll need to at least fix the Qt build issue.

> Source/JavaScriptCore/jsc.cpp:147
> +protected:
>      GlobalObject(JSGlobalData&, Structure*, const Vector<UString>& arguments);

I suggest you make these constructors private rather than protected in classes where there is no derived class. Generally speaking we would like to use private more often than protected.

> Source/JavaScriptCore/jsc.cpp:152
> +        return new (allocateCell<GlobalObject>(&(globalData.heap))) GlobalObject(globalData, structure, arguments);

This has one extra set of parentheses, around globalData.heap, and I think it would read better without them. There are similar extra parentheses in other places in the patch.

> Source/JavaScriptCore/API/JSCallbackObject.h:123
> +    // pseudo-placement version of operator new
> +    void* operator new(size_t, void* ptr) { return ptr; }

Why is this needed? What about the real placement new? Why can’t we use that?

This comment is a “what” comment, and instead we want our comments to be “why” comments. Also we normally want our comments to be sentence style with a first capital letter and a period.

> Source/JavaScriptCore/runtime/JSCell.h:379
> +    template <typename T> void* allocateCell(Heap* heap)

I think we should use a reference to a heap here instead of a pointer to a heap.

[Mark Hahnenberg]

Created attachment 100855 [details]
Patch

[Darin Adler]

Comment on attachment 100855 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1738
> +    JSActivation* activation = JSActivation::create(&(callFrame->globalData()), callFrame, static_cast<FunctionExecutable*>(ownerExecutable()));

This still has extra parentheses.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:5070
> +        JSValue arguments = JSValue(Arguments::create(&(callFrame->globalData()), functionCallFrame));

This still has extra parentheses.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:5076
> +    Arguments* arguments = Arguments::create(&(functionCallFrame->globalData()), functionCallFrame);

This still has extra parentheses.

> Source/JavaScriptCore/jit/JITStubs.cpp:2065
> +    JSActivation* activation = JSActivation::create(&(stackFrame.callFrame->globalData()), stackFrame.callFrame, static_cast<FunctionExecutable*>(stackFrame.callFrame->codeBlock()->ownerExecutable()));

This still has extra parentheses.

> Source/JavaScriptCore/runtime/Arguments.h:72
> +        static Arguments* create(JSGlobalData* globalData, CallFrame* callFrame, NoParametersType noParams)
> +        {
> +            return new (allocateCell<Arguments>(globalData->heap)) Arguments(callFrame, noParams);
> +        }

For "new" we need to use an extra argument because it’s a way to make a named constructor.

But for a create function, we can actually name the create function in a way that makes clear what these two different types of creation are and thus we don’t need to use the “extra argument” technique.

> Source/JavaScriptCore/runtime/JSActivation.cpp:223
> +    JSValue arguments = JSValue(Arguments::create(&(callFrame->globalData()), callFrame));

This still has extra parentheses.

> Source/JavaScriptCore/runtime/JSString.h:351
> +        static JSString* create(JSGlobalData* globalData, const UString& value, HasOtherOwnerType hasOtherOwnerType)
> +        {
> +            return new (allocateCell<JSString>(globalData->heap)) JSString(globalData, value, hasOtherOwnerType);
> +        }
> +        static JSString* create(JSGlobalData* globalData, PassRefPtr<StringImpl> value, HasOtherOwnerType hasOtherOwnerType)
> +        {
> +            return new (allocateCell<JSString>(globalData->heap)) JSString(globalData, value, hasOtherOwnerType);
> +        }

Same comment here as above. In a create function we can use a sensible function name rather than an extra parameter as the way to implement alternate constructors.

This lets us limit the awkward “extra parameter” technique to local use inside a class, and the type used for that technique can even be private to the class.

> Source/JavaScriptCore/runtime/NumberObject.h:30
>          explicit NumberObject(JSGlobalData&, Structure*);

It makes no sense to have “explicit” on a constructor with multiple arguments, so we really ought to remove this.

[Mark Hahnenberg]

I thought the main reason for the additional argument to operator new was to intercept the allocation request so that we can allocate the memory for the new object on our own GC-managed heap?

[Mark Hahnenberg]

Created attachment 100909 [details]
Patch

[Darin Adler]

Comment on attachment 100909 [details]
Patch

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

Sorry to drive you crazy with new comments each time. I noticed some other things I missed last time.

> Source/JavaScriptCore/jsc.cpp:146
> -public:
>      GlobalObject(JSGlobalData&, Structure*, const Vector<UString>& arguments);

We are normally explicit about private even at the top of a class.

> Source/JavaScriptCore/API/JSCallbackObject.h:123
> +    // This version of operator new is necessary because we can't tell statically 
> +    // that Base will inherit the correct placement version of operator new from JSCell.

The wording here is confusing. I think what you mean to say is that we would like to use the placement version of operator new in JSCell, but can’t because Base is a template argument.

> Source/JavaScriptCore/API/JSContextRef.cpp:100
> +    JSGlobalObject* globalObject = JSCallbackObject<JSGlobalObject>::create(*globalData.get(), globalObjectClass, JSCallbackObject<JSGlobalObject>::createStructure(*globalData, jsNull()));

I am surprised that this .get() is needed. Are you sure you needed to add it?

> Source/JavaScriptCore/runtime/ArrayConstructor.h:31
> -    public:
>          ArrayConstructor(ExecState*, JSGlobalObject*, Structure*, ArrayPrototype*);

Same issue here, where in this project we would normally make private explicit rather than depending on the "first things in a class are private" feature.

> Source/JavaScriptCore/runtime/ArrayPrototype.h:-30
> -    public:

Here too.

> Source/JavaScriptCore/runtime/BooleanConstructor.h:-31
> -    public:

Again. I won’t call out the other cases.

> Source/JavaScriptCore/runtime/RegExp.h:82
> +        // Normally this method would be called create, but we have to avoid a potential conflict with the 
> +        // normal public create method that handles caching as well.
> +        static RegExp* allocateAndInit(JSGlobalData&, const UString&, RegExpFlags);

Given that description I would name it createWithoutCaching or something like that.

[Mark Hahnenberg]

Created attachment 100914 [details]
Patch

[Darin Adler]

(In reply to comment #14)
> I thought the main reason for the additional argument to operator new was to intercept the allocation request so that we can allocate the memory for the new object on our own GC-managed heap?

The reason for the argument is to tell that we want it on our own heap. The clearest way to do that would be to name the function to state that we want it this way. The use of an argument instead is an alternative that’s used because it’s a constructor and we don’t have the option of renaming it.

[Darin Adler]

Comment on attachment 100914 [details]
Patch

Windows build is failing:


2>   Creating library C:\cygwin\home\buildbot\WebKitBuild\Debug\lib\JavaScriptCore.lib and object C:\cygwin\home\buildbot\WebKitBuild\Debug\lib\JavaScriptCore.exp
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: __thiscall JSC::DateInstance::DateInstance(class JSC::ExecState *,class JSC::Structure *,double)" (??0DateInstance@JSC@@QAE@PAVExecState@1@PAVStructure@1@N@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: __thiscall JSC::JSArray::JSArray(class JSC::JSGlobalData &,class JSC::Structure *)" (??0JSArray@JSC@@QAE@AAVJSGlobalData@1@PAVStructure@1@@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: __thiscall JSC::JSArray::JSArray(class JSC::JSGlobalData &,class JSC::Structure *,class JSC::ArgList const &)" (??0JSArray@JSC@@QAE@AAVJSGlobalData@1@PAVStructure@1@ABVArgList@1@@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: __thiscall JSC::JSFunction::JSFunction(class JSC::ExecState *,class JSC::JSGlobalObject *,class JSC::Structure *,int,class JSC::Identifier const &,__int64 (__fastcall*)(class JSC::ExecState *))" (??0JSFunction@JSC@@QAE@PAVExecState@1@PAVJSGlobalObject@1@PAVStructure@1@HABVIdentifier@1@P6I_J0@Z@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: __thiscall JSC::RegExpObject::RegExpObject(class JSC::JSGlobalObject *,class JSC::Structure *,class JSC::RegExp *)" (??0RegExpObject@JSC@@QAE@PAVJSGlobalObject@1@PAVStructure@1@PAVRegExp@1@@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: __thiscall JSC::StringObject::StringObject(class JSC::ExecState *,class JSC::Structure *,class JSC::UString const &)" (??0StringObject@JSC@@QAE@PAVExecState@1@PAVStructure@1@ABVUString@1@@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: static void * __cdecl JSC::JSGlobalObject::operator new(unsigned int,class JSC::JSGlobalData *)" (??2JSGlobalObject@JSC@@SAPAXIPAVJSGlobalData@1@@Z)
2>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: static class JSC::RegExp * __cdecl JSC::RegExp::create(class JSC::JSGlobalData *,class JSC::UString const &,enum JSC::RegExpFlags)" (?create@RegExp@JSC@@SAPAV12@PAVJSGlobalData@2@ABVUString@2@W4RegExpFlags@2@@Z)

[Darin Adler]

Comment on attachment 100914 [details]
Patch

You’ll need to update the Windows export file to fix the Windows build.

[Mark Hahnenberg]

Created attachment 101025 [details]
Patch

[Mark Hahnenberg]

Ignore the last patch, review is unnecessary.  There's more link errors on Windows.  I'm just using the build bots to give me copy & paste-able text.  The Windows try bot is rather painful to use for this purpose.

[Mark Hahnenberg]

(In reply to comment #18)
> (In reply to comment #14)
> > I thought the main reason for the additional argument to operator new was to intercept the allocation request so that we can allocate the memory for the new object on our own GC-managed heap?
> 
> The reason for the argument is to tell that we want it on our own heap. The clearest way to do that would be to name the function to state that we want it this way. The use of an argument instead is an alternative that’s used because it’s a constructor and we don’t have the option of renaming it.

So I should rename all of the create functions to createInGCHeap (or something similar) or individually name each create function (e.g. JSString::createFromUString(...) vs. JSString::createFromRope(...))?

[Mark Hahnenberg]

Created attachment 101031 [details]
Patch

[Mark Hahnenberg]

Again, ignore the patch, still testing builds until I get my Windows VM setup :-)

[Mark Hahnenberg]

Created attachment 101166 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101166 [details]
Patch

Rejecting attachment 101166 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 1

Last 500 characters of output:
g does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9114381

[Oliver Hunt]

Landing manually

[Oliver Hunt]

Committed r91194