[JSC] Do not use Bag<> for DFG / FTL watchpoints
Created attachment 426324 [details] Patch
Created attachment 426325 [details] Patch
Created attachment 426326 [details] Patch
Created attachment 426327 [details] Patch
Comment on attachment 426327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426327&action=review Title should say "for better memory efficiency" or something like that. While I’m not a deep expert on this code, it looks right to me. > Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:43 > +AdaptiveInferredPropertyValueWatchpointBase::AdaptiveInferredPropertyValueWatchpointBase() > + : m_key() > +{ > +} > + Why is this needed? Isn’t this what the default constructor would do if we just wrote "= default"? > Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.h:45 > + CodeBlockJettisoningWatchpoint() > + : CodeBlockJettisoningWatchpoint(nullptr) > + { > + } We could achieve this more economically by just writing "CodeBlock* codeBlock = nullptr" in the other constructor. > Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.cpp:46 > +AdaptiveInferredPropertyValueWatchpoint::AdaptiveInferredPropertyValueWatchpoint() > + : Base() > + , m_codeBlock(nullptr) > +{ > +} It would be more elegant to initialize m_codeBlock to nullptr in the header, and then use "= default" for this constructor. > Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:47 > + , m_codeBlock(nullptr) Can we initialize m_codeBlock in the class definition, and then omit this line? Maybe not because of JSC_WATCHPOINT_FIELD? > Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:48 > + , m_key() Can we omit this line? Pretty sure that this isn’t needed. Maybe not because of JSC_WATCHPOINT_FIELD? > Source/JavaScriptCore/dfg/DFGCommonData.cpp:151 > + for (AdaptiveStructureWatchpoint& watchpoint : m_adaptiveStructureWatchpoints) I think code like this is more readable with auto& rather than writing out a type name. Using "auto&" documents that the type is correct and does not slice or upcast. > Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:49 > + WatchpointCollector() = default; I believe we can simply omit this entirely, and behavior should be the same. > Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:60 > + template<typename Func> > + void addWatchpoint(const Func& func) Could we use a word instead of "func"?
Comment on attachment 426327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426327&action=review Thanks >> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:43 >> + > > Why is this needed? Isn’t this what the default constructor would do if we just wrote "= default"? Yeah, replaced with = default; >> Source/JavaScriptCore/bytecode/CodeBlockJettisoningWatchpoint.h:45 >> + } > > We could achieve this more economically by just writing "CodeBlock* codeBlock = nullptr" in the other constructor. Replaced. >> Source/JavaScriptCore/dfg/DFGAdaptiveInferredPropertyValueWatchpoint.cpp:46 >> +} > > It would be more elegant to initialize m_codeBlock to nullptr in the header, and then use "= default" for this constructor. Replaced. >> Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:47 >> + , m_codeBlock(nullptr) > > Can we initialize m_codeBlock in the class definition, and then omit this line? Maybe not because of JSC_WATCHPOINT_FIELD? Unfortunately it is not since JSC_WATCHPOINT_FIELD. >> Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:48 >> + , m_key() > > Can we omit this line? Pretty sure that this isn’t needed. Maybe not because of JSC_WATCHPOINT_FIELD? Yes, we can omit it. >> Source/JavaScriptCore/dfg/DFGCommonData.cpp:151 >> + for (AdaptiveStructureWatchpoint& watchpoint : m_adaptiveStructureWatchpoints) > > I think code like this is more readable with auto& rather than writing out a type name. Using "auto&" documents that the type is correct and does not slice or upcast. Replaced. >> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:49 >> + WatchpointCollector() = default; > > I believe we can simply omit this entirely, and behavior should be the same. Removed. >> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:60 >> + void addWatchpoint(const Func& func) > > Could we use a word instead of "func"? Replaced with function.
Committed r276226 (236708@main): <https://commits.webkit.org/236708@main>
Comment on attachment 426327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426327&action=review >>> Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:49 >>> + WatchpointCollector() = default; >> >> I believe we can simply omit this entirely, and behavior should be the same. > > Removed. Ah, this was important. If we are using WTF_MAKE_NONCOPYABLE, we need this. Otherwise, this becomes compile error.
Committed r276227 (236709@main): <https://commits.webkit.org/236709@main>
rdar://76810095