Bug 185539 – Don't use inferred types when the JIT is disabled

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

[Saam Barati]

They use memory and do nothing good if you're only running in the LLInt

[Saam Barati]

Created attachment 340169 [details]
patch

[Yusuke Suzuki]

Comment on attachment 340169 [details]
patch

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

Patch looks good. But I have one question.

> Source/JavaScriptCore/runtime/VM.h:558
> +    ALWAYS_INLINE bool canUseJIT() const { return m_canUseJIT; }

Why do we change this to VM's member function? IIRC, our CSSJIT selector compiler uses VM::canUseJIT(). And since it is a static function, we can remove VM& reference from CSS JIT compiler.

[Saam Barati]

Comment on attachment 340169 [details]
patch

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

>> Source/JavaScriptCore/runtime/VM.h:558
>> +    ALWAYS_INLINE bool canUseJIT() const { return m_canUseJIT; }
> 
> Why do we change this to VM's member function? IIRC, our CSSJIT selector compiler uses VM::canUseJIT(). And since it is a static function, we can remove VM& reference from CSS JIT compiler.

I just wanted it to be a single load instead of an out of line function call. I didn't realize the CSSJIT uses this. I'll fix that.

[Yusuke Suzuki]

Comment on attachment 340169 [details]
patch

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

>>> Source/JavaScriptCore/runtime/VM.h:558
>>> +    ALWAYS_INLINE bool canUseJIT() const { return m_canUseJIT; }
>> 
>> Why do we change this to VM's member function? IIRC, our CSSJIT selector compiler uses VM::canUseJIT(). And since it is a static function, we can remove VM& reference from CSS JIT compiler.
> 
> I just wanted it to be a single load instead of an out of line function call. I didn't realize the CSSJIT uses this. I'll fix that.

OK, it is performance reason. If it is critical, I'm OK to have m_canUseJIT in VM side :)

[Yusuke Suzuki]

Comment on attachment 340169 [details]
patch

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

>>>> Source/JavaScriptCore/runtime/VM.h:558
>>>> +    ALWAYS_INLINE bool canUseJIT() const { return m_canUseJIT; }
>>> 
>>> Why do we change this to VM's member function? IIRC, our CSSJIT selector compiler uses VM::canUseJIT(). And since it is a static function, we can remove VM& reference from CSS JIT compiler.
>> 
>> I just wanted it to be a single load instead of an out of line function call. I didn't realize the CSSJIT uses this. I'll fix that.
> 
> OK, it is performance reason. If it is critical, I'm OK to have m_canUseJIT in VM side :)

And calling processCanUseJIT things from CSSJIT.

[Saam Barati]

Comment on attachment 340169 [details]
patch

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

>>>>> Source/JavaScriptCore/runtime/VM.h:558
>>>>> +    ALWAYS_INLINE bool canUseJIT() const { return m_canUseJIT; }
>>>> 
>>>> Why do we change this to VM's member function? IIRC, our CSSJIT selector compiler uses VM::canUseJIT(). And since it is a static function, we can remove VM& reference from CSS JIT compiler.
>>> 
>>> I just wanted it to be a single load instead of an out of line function call. I didn't realize the CSSJIT uses this. I'll fix that.
>> 
>> OK, it is performance reason. If it is critical, I'm OK to have m_canUseJIT in VM side :)
> 
> And calling processCanUseJIT things from CSSJIT.

I doubt it's critical. I just don't like it when we generate more code to make a function call than if that function call were inlined. I think what I'll do is just define an exported extern bool or something like that.

[Saam Barati]

(In reply to Saam Barati from comment #6)
> Comment on attachment 340169 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340169&action=review
> 
> >>>>> Source/JavaScriptCore/runtime/VM.h:558
> >>>>> +    ALWAYS_INLINE bool canUseJIT() const { return m_canUseJIT; }
> >>>> 
> >>>> Why do we change this to VM's member function? IIRC, our CSSJIT selector compiler uses VM::canUseJIT(). And since it is a static function, we can remove VM& reference from CSS JIT compiler.
> >>> 
> >>> I just wanted it to be a single load instead of an out of line function call. I didn't realize the CSSJIT uses this. I'll fix that.
> >> 
> >> OK, it is performance reason. If it is critical, I'm OK to have m_canUseJIT in VM side :)
> > 
> > And calling processCanUseJIT things from CSSJIT.
> 
> I doubt it's critical. I just don't like it when we generate more code to
> make a function call than if that function call were inlined. I think what
> I'll do is just define an exported extern bool or something like that.

Ima just keep it as an out of line call for now. That's the simplest thing to do.

[Saam Barati]

Created attachment 340173 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 340173 [details]
patch for landing

Clearing flags on attachment: 340173

Committed r231703: <https://trac.webkit.org/changeset/231703>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40164001>

[Filip Pizlo]

Comment on attachment 340173 [details]
patch for landing

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

> Source/JavaScriptCore/runtime/Structure.h:611
> +        if (hasBeenDictionary() || (!shouldOptimize && !m_inferredTypeTable) || !VM::canUseJIT())

I don't like that canUseJIT is not inline, and that there are so many conditions here.  willStoreValueSlow() is a slow path, but it's not such a slow path that we wan't to add out-of-line calls.

[Saam Barati]

Comment on attachment 340173 [details]
patch for landing

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

>> Source/JavaScriptCore/runtime/Structure.h:611
>> +        if (hasBeenDictionary() || (!shouldOptimize && !m_inferredTypeTable) || !VM::canUseJIT())
> 
> I don't like that canUseJIT is not inline, and that there are so many conditions here.  willStoreValueSlow() is a slow path, but it's not such a slow path that we wan't to add out-of-line calls.

I thought about this when landing this, but I opted for just doing the out of line call because it was slightly annoying to add a fast path for canUseJIT. We can add a fast path for this. Or we can also just set this value in initializeThreading and make it a single load from a static variable

[Filip Pizlo]

(In reply to Saam Barati from comment #13)
> Comment on attachment 340173 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340173&action=review
> 
> >> Source/JavaScriptCore/runtime/Structure.h:611
> >> +        if (hasBeenDictionary() || (!shouldOptimize && !m_inferredTypeTable) || !VM::canUseJIT())
> > 
> > I don't like that canUseJIT is not inline, and that there are so many conditions here.  willStoreValueSlow() is a slow path, but it's not such a slow path that we wan't to add out-of-line calls.
> 
> I thought about this when landing this, but I opted for just doing the out
> of line call because it was slightly annoying to add a fast path for
> canUseJIT. We can add a fast path for this. Or we can also just set this
> value in initializeThreading and make it a single load from a static variable

By the time any of these functions are called, at least one VM had to have been constructed.  So it's a bit weird that VM::canUseJIT() is a thing that does lazy initialization.  Maybe other users of it benefit from the lazy initialization, but this sure doesn't.

[Saam Barati]

(In reply to Filip Pizlo from comment #14)
> (In reply to Saam Barati from comment #13)
> > Comment on attachment 340173 [details]
> > patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340173&action=review
> > 
> > >> Source/JavaScriptCore/runtime/Structure.h:611
> > >> +        if (hasBeenDictionary() || (!shouldOptimize && !m_inferredTypeTable) || !VM::canUseJIT())
> > > 
> > > I don't like that canUseJIT is not inline, and that there are so many conditions here.  willStoreValueSlow() is a slow path, but it's not such a slow path that we wan't to add out-of-line calls.
> > 
> > I thought about this when landing this, but I opted for just doing the out
> > of line call because it was slightly annoying to add a fast path for
> > canUseJIT. We can add a fast path for this. Or we can also just set this
> > value in initializeThreading and make it a single load from a static variable
> 
> By the time any of these functions are called, at least one VM had to have
> been constructed.  So it's a bit weird that VM::canUseJIT() is a thing that
> does lazy initialization.  Maybe other users of it benefit from the lazy
> initialization, but this sure doesn't.
Agreed. I’m pretty sure we even call this during VM construction, so no reason for any laziness. I’ll prepare a fix