Bug 139532 – Final clean up OwnPtr in JSC - runtime, ftl, and tool directories

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

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 243108 [details]
Patch

[Gyuyoung Kim]

Created attachment 243122 [details]
Patch

[Mark Lam]

Comment on attachment 243122 [details]
Patch

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

> Source/JavaScriptCore/runtime/Structure.cpp:40
> +#include <wtf/PassOwnPtr.h>

Why do we need this?

> Source/JavaScriptCore/runtime/StructureIDTable.cpp:49
> -    OwnPtr<StructureOrOffset> newTable = adoptPtr(new StructureOrOffset[newCapacity]);
> +    auto newTable = std::make_unique<StructureOrOffset[]>(newCapacity);

In the code below this, we append the old m_table (swap’ed into newTable) to m_oldTables.  Currently, that code uses release().  Shouldn't we be using WTF::move() instead there?

> Source/JavaScriptCore/runtime/StructureIDTable.h:69
> +    Vector<std::unique_ptr<StructureOrOffset>> m_oldTables;

Shouldn’t this be "Vector<std::unique_ptr<StructureOrOffset[]>> m_oldTables;” instead?

> Source/JavaScriptCore/tools/CodeProfile.h:44
> -            parent->addChild(this);
> +            parent->addChild(std::make_unique<CodeProfile>(source, parent));

This is incorrect.  Previously, the parent retains a pointer to “this”.  Now it retains a pointer to a clone of “this” by constructing another instance, and it looks like this results in infinite recursion too.

[Gyuyoung Kim]

Created attachment 243178 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 243122 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Structure.cpp:40
>> +#include <wtf/PassOwnPtr.h>
> 
> Why do we need this?

Removed.

>> Source/JavaScriptCore/runtime/StructureIDTable.cpp:49
>> +    auto newTable = std::make_unique<StructureOrOffset[]>(newCapacity);
> 
> In the code below this, we append the old m_table (swap’ed into newTable) to m_oldTables.  Currently, that code uses release().  Shouldn't we be using WTF::move() instead there?

Done.

>> Source/JavaScriptCore/runtime/StructureIDTable.h:69
>> +    Vector<std::unique_ptr<StructureOrOffset>> m_oldTables;
> 
> Shouldn’t this be "Vector<std::unique_ptr<StructureOrOffset[]>> m_oldTables;” instead?

Used it.

>> Source/JavaScriptCore/tools/CodeProfile.h:44
>> +            parent->addChild(std::make_unique<CodeProfile>(source, parent));
> 
> This is incorrect.  Previously, the parent retains a pointer to “this”.  Now it retains a pointer to a clone of “this” by constructing another instance, and it looks like this results in infinite recursion too.

I don't know how to cast this pointer to unique_ptr. If you know it, please let me know.

Anyway, I change to add "this" instance to m_children in caller (CodeProfiling::begin())

    bool alreadyProfiling = s_profileStack;
    auto profileStack = std::make_unique<CodeProfile>(source, const_cast<CodeProfile*>(s_profileStack));
    s_profileStack = profileStack.get();
    profileStack->addChild(WTF::move(profileStack));

[Mark Lam]

Comment on attachment 243178 [details]
Patch

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

When you’re done updating your patch, please do a recursive grep in the JavaScriptCore directory to ensure that there are no more uses of OwnPtr and PassOwnPtr?  This will help catch any #include that you may have missed.

> Source/JavaScriptCore/runtime/StructureIDTable.h:68
>      static const size_t s_initialSize = 256;

There’s a #include <OwnPtr.h> at the top of this file that should be removed now.  You had this change in your previous patch.  Why did you undo it?

> Source/JavaScriptCore/tools/CodeProfile.h:-44
> -        if (parent)
> -            parent->addChild(this);

Let’s keep the original design that hides this detail in the CodeProfile class instead of passing this burden (of associating the child with the parent) onto the client.

You can implement this as the following:
    if (parent)
        parent->addChile(std::unique_ptr<CodeProfile>(this));

> Source/JavaScriptCore/tools/CodeProfiling.cpp:139
> -    CodeProfile* profileStack = const_cast<CodeProfile*>(s_profileStack);
> -    bool alreadyProfiling = profileStack;
> -    s_profileStack = profileStack = new CodeProfile(source, profileStack);
> +    bool alreadyProfiling = s_profileStack;
> +    auto profileStack = std::make_unique<CodeProfile>(source, const_cast<CodeProfile*>(s_profileStack));
> +    s_profileStack = profileStack.get();
> +    profileStack->addChild(WTF::move(profileStack));

Let’s undo this.

[Gyuyoung Kim]

Created attachment 243180 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #6)
> Comment on attachment 243178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243178&action=review
> 
> When you’re done updating your patch, please do a recursive grep in the
> JavaScriptCore directory to ensure that there are no more uses of OwnPtr and
> PassOwnPtr?  This will help catch any #include that you may have missed.

> > Source/JavaScriptCore/runtime/StructureIDTable.h:68
> >      static const size_t s_initialSize = 256;
> 
> There’s a #include <OwnPtr.h> at the top of this file that should be removed
> now.  You had this change in your previous patch.  Why did you undo it?

Yes, I have tried to remove useless #include <wtf/OwnPtr.h> / <wtf/PassOwnPtr.h> with clean up OwnPtr,PassOwnPtr. However, build break sometimes occurs because of include chain. In this case, I restored it because of build break in other file, which seems to include StructureIDTable.h. Anyway, now it can be removed. I will also try to remove useless #include <wtf/OwnPtr.h>. 


> > Source/JavaScriptCore/tools/CodeProfile.h:-44
> > -        if (parent)
> > -            parent->addChild(this);
> 
> Let’s keep the original design that hides this detail in the CodeProfile
> class instead of passing this burden (of associating the child with the
> parent) onto the client.
> 
> You can implement this as the following:
>     if (parent)
>         parent->addChile(std::unique_ptr<CodeProfile>(this));

Oh, right. I only thought to use std::make_unique<>. Fixed.
 
> > Source/JavaScriptCore/tools/CodeProfiling.cpp:139
> > -    CodeProfile* profileStack = const_cast<CodeProfile*>(s_profileStack);
> > -    bool alreadyProfiling = profileStack;
> > -    s_profileStack = profileStack = new CodeProfile(source, profileStack);
> > +    bool alreadyProfiling = s_profileStack;
> > +    auto profileStack = std::make_unique<CodeProfile>(source, const_cast<CodeProfile*>(s_profileStack));
> > +    s_profileStack = profileStack.get();
> > +    profileStack->addChild(WTF::move(profileStack));
> 
> Let’s undo this.

Done !

[Mark Lam]

Comment on attachment 243180 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:245
> +    std::unique_ptr< Vector<PropertyOffset>> m_deletedOffsets;

nit: Can you please remove the space between the < and Vector?

[Gyuyoung Kim]

Created attachment 243181 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #9)

> > Source/JavaScriptCore/runtime/PropertyMapHashTable.h:245
> > +    std::unique_ptr< Vector<PropertyOffset>> m_deletedOffsets;
> 
> nit: Can you please remove the space between the < and Vector?

Fixed. Thanks !

[WebKit Commit Bot]

Comment on attachment 243181 [details]
Patch

Clearing flags on attachment: 243181

Committed r177222: <http://trac.webkit.org/changeset/177222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.