Bug 139532

Created attachment 243108 [details] Patch

Created attachment 243122 [details] Patch

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.

Created attachment 243178 [details] Patch

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));

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.

Created attachment 243180 [details] Patch

(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 !

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?

Created attachment 243181 [details] Patch

(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 !

Comment on attachment 243181 [details] Patch Clearing flags on attachment: 243181 Committed r177222: <http://trac.webkit.org/changeset/177222>

All reviewed patches have been landed. Closing bug.