Bug 224713 - [JSC] Make more DFG/FTL data FixedVector/Vector
Summary: [JSC] Make more DFG/FTL data FixedVector/Vector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-16 22:11 PDT by Yusuke Suzuki
Modified: 2021-04-19 22:08 PDT (History)
8 users (show)

See Also:


Attachments
Patch (33.22 KB, patch)
2021-04-16 22:24 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (33.80 KB, patch)
2021-04-16 22:29 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-04-16 22:11:21 PDT
[JSC] Make more DFG/FTL data FixedVector/Vector
Comment 1 Yusuke Suzuki 2021-04-16 22:24:43 PDT
Created attachment 426321 [details]
Patch
Comment 2 Yusuke Suzuki 2021-04-16 22:29:06 PDT
Created attachment 426322 [details]
Patch
Comment 3 Darin Adler 2021-04-17 21:50:15 PDT
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 4 Yusuke Suzuki 2021-04-18 00:09:50 PDT
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.
Comment 5 Yusuke Suzuki 2021-04-18 00:14:27 PDT
Committed r276224 (236706@main): <https://commits.webkit.org/236706@main>
Comment 6 Darin Adler 2021-04-18 09:06:42 PDT
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.
Comment 7 Ryan Haddad 2021-04-19 22:08:26 PDT
rdar://76809579