| Summary: | [JSC] Make more DFG/FTL data FixedVector/Vector | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2021-04-16 22:11:21 PDT
Created attachment 426321 [details]
Patch
Created attachment 426322 [details]
Patch
Comment on attachment 426322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426322&action=review > Source/JavaScriptCore/bytecode/Operands.h:141 > + template<typename U, typename V> friend class Operands; No need for the "U" and "V" here. > Source/JavaScriptCore/dfg/DFGJITCompiler.h:269 > + unsigned appendOSRExit(OSRExit&& exit) This creates a 2^32 maximum. Does something check that and prevent overflow? > Source/JavaScriptCore/dfg/DFGJITCompiler.h:276 > + unsigned appendSpeculationRecovery(const SpeculationRecovery& recovery) Ditto. > Source/JavaScriptCore/dfg/DFGJITCompiler.h:394 > +public: > + Vector<DFG::OSREntryData> m_osrEntry; > + Vector<DFG::OSRExit> m_osrExit; > + Vector<DFG::SpeculationRecovery> m_speculationRecovery; > +private: This is inelegant. Is there a way to do this without making the vectors public? Even accessor functions that return references would be more elegant than this. But also could use friend maybe? > Source/JavaScriptCore/ftl/FTLOSRExit.cpp:88 > + unsigned index = state.jitCode->m_osrExit.size(); Same 2^32 question. Comment on attachment 426322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426322&action=review >> Source/JavaScriptCore/dfg/DFGJITCompiler.h:269 >> + unsigned appendOSRExit(OSRExit&& exit) > > This creates a 2^32 maximum. Does something check that and prevent overflow? Vector crashes on overflow. And there is no way to achieve that since DFG / FTL has node amount limit (10000) >> Source/JavaScriptCore/dfg/DFGJITCompiler.h:276 >> + unsigned appendSpeculationRecovery(const SpeculationRecovery& recovery) > > Ditto. Ditto. >> Source/JavaScriptCore/dfg/DFGJITCompiler.h:394 >> +private: > > This is inelegant. Is there a way to do this without making the vectors public? Even accessor functions that return references would be more elegant than this. But also could use friend maybe? I don't think accessor function is better since we WTFMove(them). I don't think we expect that when we are using accessor functions. friend class would be better. >> Source/JavaScriptCore/ftl/FTLOSRExit.cpp:88 >> + unsigned index = state.jitCode->m_osrExit.size(); > > Same 2^32 question. Ditto. Committed r276224 (236706@main): <https://commits.webkit.org/236706@main> Comment on attachment 426322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426322&action=review >>> Source/JavaScriptCore/dfg/DFGJITCompiler.h:394 >>> +private: >> >> This is inelegant. Is there a way to do this without making the vectors public? Even accessor functions that return references would be more elegant than this. But also could use friend maybe? > > I don't think accessor function is better since we WTFMove(them). I don't think we expect that when we are using accessor functions. > friend class would be better. If what we are doing is moving vectors out, then the style of access function I would recommend is “take“ which includes the move (or std::exchange) and returns the moved object. This does create better encapsulation than just having public data members. |