Bug 216685 – Move some LLInt globals into JSC::Config.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

rdar://68964544

[Mark Lam]

Created attachment 409103 [details]
proposed patch.

[Mark Lam]

Created attachment 409105 [details]
proposed patch.

[Mark Lam]

Created attachment 409108 [details]
proposed patch.

[Mark Lam]

Created attachment 409109 [details]
proposed patch.

[Mark Lam]

Created attachment 409112 [details]
proposed patch.

[Mark Lam]

Created attachment 409113 [details]
patch for EWS testing.

[Mark Lam]

Created attachment 409114 [details]
EWS test

[Mark Lam]

Created attachment 409115 [details]
EWS test.

[Mark Lam]

Created attachment 409121 [details]
EWS test.

[Mark Lam]

Created attachment 409152 [details]
EWS test.

[Mark Lam]

Created attachment 409155 [details]
EWS test.

[Mark Lam]

Created attachment 409163 [details]
EWS test

[Mark Lam]

Created attachment 409167 [details]
EWS test.

[Myles C. Maxfield]

Created attachment 409194 [details]
LowLevelInterpreterWin.asm without patch

[Myles C. Maxfield]

Created attachment 409195 [details]
LowLevelInterpreterWin.asm with patch

[Mark Lam]

Created attachment 409212 [details]
proposed patch.

[Mark Lam]

Created attachment 409213 [details]
proposed patch.

[Mark Lam]

Comment on attachment 409213 [details]
proposed patch.

Ready for a review now.

[Sam Weinig]

Comment on attachment 409213 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=409213&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Move some LLInt globals into JSC::Config.

What is the goal of doing this?

[Mark Lam]

(In reply to Sam Weinig from comment #19)
> Comment on attachment 409213 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409213&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        Move some LLInt globals into JSC::Config.
> 
> What is the goal of doing this?

The opcodeMaps are supposed to be functionally constant.  They are meant to be initialized once and never changed thereafter.  This change enforces that invariant.

[Sam Weinig]

Comment on attachment 409213 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=409213&action=review

>>> Source/JavaScriptCore/ChangeLog:3
>>> +        Move some LLInt globals into JSC::Config.
>> 
>> What is the goal of doing this?
> 
> The opcodeMaps are supposed to be functionally constant.  They are meant to be initialized once and never changed thereafter.  This change enforces that invariant.

It feels like an oddly named place, "Config", for an opcode map. Perhaps either JSC::Config needs a new name, or these should go somewhere else that can enforce that invariant?

[Mark Lam]

(In reply to Sam Weinig from comment #21)
> Comment on attachment 409213 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409213&action=review
> 
> >>> Source/JavaScriptCore/ChangeLog:3
> >>> +        Move some LLInt globals into JSC::Config.
> >> 
> >> What is the goal of doing this?
> > 
> > The opcodeMaps are supposed to be functionally constant.  They are meant to be initialized once and never changed thereafter.  This change enforces that invariant.
> 
> It feels like an oddly named place, "Config", for an opcode map. Perhaps
> either JSC::Config needs a new name, or these should go somewhere else that
> can enforce that invariant?

It's not that odd.  In a sense, the opcode maps "configures" how the LLInt will behave (though this is a bit of a stretch).  But yeah, if we can come up with a better name for the Config, I would gladly adopt it.  No one has a better proposal yet.  When I first introduced it, I considered ConstData / ROData (for ReadOnlyData) but those are not accurate either (the data is only const/read-only after freezing, and not before).

[Sam Weinig]

(In reply to Mark Lam from comment #22)
> (In reply to Sam Weinig from comment #21)
> > Comment on attachment 409213 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409213&action=review
> > 
> > >>> Source/JavaScriptCore/ChangeLog:3
> > >>> +        Move some LLInt globals into JSC::Config.
> > >> 
> > >> What is the goal of doing this?
> > > 
> > > The opcodeMaps are supposed to be functionally constant.  They are meant to be initialized once and never changed thereafter.  This change enforces that invariant.
> > 
> > It feels like an oddly named place, "Config", for an opcode map. Perhaps
> > either JSC::Config needs a new name, or these should go somewhere else that
> > can enforce that invariant?
> 
> It's not that odd.  In a sense, the opcode maps "configures" how the LLInt
> will behave (though this is a bit of a stretch).  But yeah, if we can come
> up with a better name for the Config, I would gladly adopt it.  No one has a
> better proposal yet.  When I first introduced it, I considered ConstData /
> ROData (for ReadOnlyData) but those are not accurate either (the data is
> only const/read-only after freezing, and not before).

I see, makes sense. We should definitely rename JSC::Config (or make it a member of a new thing) at some point. Not needed for this change.

[Keith Miller]

Comment on attachment 409213 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=409213&action=review

r=me with nits

> Source/JavaScriptCore/ChangeLog:18
> +        2. Add a LLIntInitializeAssertScope to be used in LLInt::initialize() to ensure
> +           that it is only called during primodial initialization time.

It seems like this is unnecessary, since we will crash once the data has been made read only anyway, right?

> Source/JavaScriptCore/llint/LLIntData.cpp:65
> +    LLIntInitializeAssertScope assertScope;

Nit: if you want to keep this assert, make it a ScopeExit, you're only using it once anyway.

> Source/JavaScriptCore/offlineasm/arm.rb:269
> +                        newList << Instruction.new(codeOrigin, "move", [Immediate.new(node.codeOrigin, labelRef.offset), tmp])
> +                        newList << Instruction.new(codeOrigin, "addp", [tmp, node.operands[1]])

Can you put a FIXME to implement the logic to figure out which immediates are encodable inline here?

[Mark Lam]

Comment on attachment 409213 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=409213&action=review

>> Source/JavaScriptCore/ChangeLog:18
>> +           that it is only called during primodial initialization time.
> 
> It seems like this is unnecessary, since we will crash once the data has been made read only anyway, right?

After thinking about this some more, I agree that this is redundant.  I'll remove it.

>> Source/JavaScriptCore/llint/LLIntData.cpp:65
>> +    LLIntInitializeAssertScope assertScope;
> 
> Nit: if you want to keep this assert, make it a ScopeExit, you're only using it once anyway.

The scope asserts on function entry (via the constructor) and on exit (via the destructor).  The exit scope solution is not the same.  But anyway, this is moot since I'll remove this.

>> Source/JavaScriptCore/offlineasm/arm.rb:269
>> +                        newList << Instruction.new(codeOrigin, "addp", [tmp, node.operands[1]])
> 
> Can you put a FIXME to implement the logic to figure out which immediates are encodable inline here?

255 is the basic (covered here already), but traditional 32-bit ARM instruction set does allow for encoding of some larger immediate via rotation tricks.  Not too sure about Thumb2 though.  I'll have the a FIXME just say that we can explore other encodings of immediate that fit in the add instruction.

[Mark Lam]

Thanks for the review.  Landed in r267371: <http://trac.webkit.org/r267371>.