They use memory and do nothing good if you're only running in the LLInt
Created attachment 340169 [details] patch
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 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 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 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 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.
(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.
Created attachment 340173 [details] patch for landing
Comment on attachment 340173 [details] patch for landing Clearing flags on attachment: 340173 Committed r231703: <https://trac.webkit.org/changeset/231703>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40164001>
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 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
(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.
(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