Bug 185539 - Don't use inferred types when the JIT is disabled
Summary: Don't use inferred types when the JIT is disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks: 185524
  Show dependency treegraph
 
Reported: 2018-05-10 21:11 PDT by Saam Barati
Modified: 2018-07-02 08:31 PDT (History)
13 users (show)

See Also:


Attachments
patch (11.02 KB, patch)
2018-05-10 21:46 PDT, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing (3.26 KB, patch)
2018-05-10 23:05 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2018-05-10 21:11:12 PDT
They use memory and do nothing good if you're only running in the LLInt
Comment 1 Saam Barati 2018-05-10 21:46:22 PDT
Created attachment 340169 [details]
patch
Comment 2 Yusuke Suzuki 2018-05-10 21:51:23 PDT
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.
Comment 3 Saam Barati 2018-05-10 21:53:18 PDT
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.
Comment 4 Yusuke Suzuki 2018-05-10 21:55:03 PDT
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 :)
Comment 5 Yusuke Suzuki 2018-05-10 21:55:36 PDT
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.
Comment 6 Saam Barati 2018-05-10 21:59:08 PDT
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.
Comment 7 Saam Barati 2018-05-10 22:52:36 PDT
(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.
Comment 8 Saam Barati 2018-05-10 23:05:26 PDT
Created attachment 340173 [details]
patch for landing
Comment 9 WebKit Commit Bot 2018-05-11 09:08:48 PDT
Comment on attachment 340173 [details]
patch for landing

Clearing flags on attachment: 340173

Committed r231703: <https://trac.webkit.org/changeset/231703>
Comment 10 WebKit Commit Bot 2018-05-11 09:08:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-05-11 09:09:23 PDT
<rdar://problem/40164001>
Comment 12 Filip Pizlo 2018-07-01 12:08:58 PDT
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.
Comment 13 Saam Barati 2018-07-01 12:54:25 PDT
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
Comment 14 Filip Pizlo 2018-07-02 08:06:24 PDT
(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.
Comment 15 Saam Barati 2018-07-02 08:31:16 PDT
(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