Bug 224713 – [JSC] Make more DFG/FTL data FixedVector/Vector

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Make more DFG/FTL data FixedVector/Vector

[Yusuke Suzuki]

Created attachment 426321 [details]
Patch

[Yusuke Suzuki]

Created attachment 426322 [details]
Patch

[Darin Adler]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r276224 (236706@main): <https://commits.webkit.org/236706@main>

[Darin Adler]

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.

[Ryan Haddad]

rdar://76809579