Bug 153445 - Switch FTL to B3 on X86_64/Mac
Summary: Switch FTL to B3 on X86_64/Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 153739
Blocks: 150279
  Show dependency treegraph
 
Reported: 2016-01-25 13:45 PST by Filip Pizlo
Modified: 2016-02-01 03:10 PST (History)
16 users (show)

See Also:


Attachments
the patch (3.47 KB, patch)
2016-01-25 14:56 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-01-25 13:45:36 PST
Patch forthcoming.  Already rubber stamped by ggaren.
Comment 1 Filip Pizlo 2016-01-25 14:50:16 PST
Michael, Ossy: I CC'd you guys because this tracks enabling B3 on Mac and disabling LLVM on Mac.  We think that it's stable and fast enough to justify this.

We will continue to work on making B3 even faster after this lands.  It will be easy to make it fast and stable once it's actually enabled.
Comment 2 Filip Pizlo 2016-01-25 14:56:19 PST
Created attachment 269797 [details]
the patch
Comment 3 Michael Catanzaro 2016-01-25 15:20:23 PST
Wow, I wasn't expecting to see this so soon!

I think it would be safer to branch for WebKitGTK+ 2.12 before we make this switch, but 2.10 did not depend on LLVM, and it's silly and annoying for distributors for us to introduce a new LLVM dependency in 2.12 that's just going to go away in 2.14 (as I presume we will have enabled B3 by then). So I think we should either (a) enable B3 for 2.12, or (b) disable FTL for 2.12 and enable FTL/B3 for 2.14. I don't want to release 2.12 using LLVM.

Also, once this Mac patch lands, LLVM will be used exclusively for EFL and GTK. I guess if we switch, that code can be removed?

(Note: B3 doesn't build for me, so it can't work, but I don't know how much work it would need once we make it build.)
Comment 4 Michael Catanzaro 2016-01-25 15:23:02 PST
Comment on attachment 269797 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGCommon.h:40
>  // to 0 before committing!

Don't forget to remove this comment ("Remember to set this to 0 before committing!")
Comment 5 Filip Pizlo 2016-01-25 15:41:25 PST
Landed in http://trac.webkit.org/changeset/195562!
Comment 6 Filip Pizlo 2016-01-25 15:42:13 PST
(In reply to comment #3)
> Wow, I wasn't expecting to see this so soon!
> 
> I think it would be safer to branch for WebKitGTK+ 2.12 before we make this
> switch, but 2.10 did not depend on LLVM, and it's silly and annoying for
> distributors for us to introduce a new LLVM dependency in 2.12 that's just
> going to go away in 2.14 (as I presume we will have enabled B3 by then). So
> I think we should either (a) enable B3 for 2.12, or (b) disable FTL for 2.12
> and enable FTL/B3 for 2.14. I don't want to release 2.12 using LLVM.
> 
> Also, once this Mac patch lands, LLVM will be used exclusively for EFL and
> GTK. I guess if we switch, that code can be removed?

Don't forget about iOS.  Right now, it's still using LLVM.

> 
> (Note: B3 doesn't build for me, so it can't work, but I don't know how much
> work it would need once we make it build.)

It's probably close.  I'm happy to help. :-)
Comment 7 Filip Pizlo 2016-01-25 15:42:30 PST
(In reply to comment #4)
> Comment on attachment 269797 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269797&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCommon.h:40
> >  // to 0 before committing!
> 
> Don't forget to remove this comment ("Remember to set this to 0 before
> committing!")

Ooops, I'll land a follow-up that edits this comment.
Comment 8 Filip Pizlo 2016-01-25 15:45:00 PST
(In reply to comment #7)
> (In reply to comment #4)
> > Comment on attachment 269797 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=269797&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGCommon.h:40
> > >  // to 0 before committing!
> > 
> > Don't forget to remove this comment ("Remember to set this to 0 before
> > committing!")
> 
> Ooops, I'll land a follow-up that edits this comment.

http://trac.webkit.org/changeset/195563
Comment 9 Gyuyoung Kim 2016-01-25 17:28:23 PST
(In reply to comment #6)
> (In reply to comment #3)
> > Wow, I wasn't expecting to see this so soon!
> > 
> > I think it would be safer to branch for WebKitGTK+ 2.12 before we make this
> > switch, but 2.10 did not depend on LLVM, and it's silly and annoying for
> > distributors for us to introduce a new LLVM dependency in 2.12 that's just
> > going to go away in 2.14 (as I presume we will have enabled B3 by then). So
> > I think we should either (a) enable B3 for 2.12, or (b) disable FTL for 2.12
> > and enable FTL/B3 for 2.14. I don't want to release 2.12 using LLVM.
> > 
> > Also, once this Mac patch lands, LLVM will be used exclusively for EFL and
> > GTK. I guess if we switch, that code can be removed?
> 
> Don't forget about iOS.  Right now, it's still using LLVM.

Nice job ! Let me check if EFL port also can support B3 soon. Thanks.
Comment 10 Carlos Garcia Campos 2016-01-25 23:17:39 PST
(In reply to comment #3)
> Wow, I wasn't expecting to see this so soon!
> 
> I think it would be safer to branch for WebKitGTK+ 2.12 before we make this
> switch, but 2.10 did not depend on LLVM, and it's silly and annoying for
> distributors for us to introduce a new LLVM dependency in 2.12 that's just
> going to go away in 2.14 (as I presume we will have enabled B3 by then). So
> I think we should either (a) enable B3 for 2.12, or (b) disable FTL for 2.12
> and enable FTL/B3 for 2.14. I don't want to release 2.12 using LLVM.
> 
> Also, once this Mac patch lands, LLVM will be used exclusively for EFL and
> GTK. I guess if we switch, that code can be removed?
> 
> (Note: B3 doesn't build for me, so it can't work, but I don't know how much
> work it would need once we make it build.)

https://bugs.webkit.org/show_bug.cgi?id=153478
Comment 11 Csaba Osztrogonác 2016-01-26 04:06:20 PST
Just a question, when do you plan to drop LLVM based FTL support?

B3 based FTL isn't ready on Linux (X86_64) yet. The biggest problem is 
bug153422, but there were crashes (only 5-6) before this regression.

If you plan to remove LLVM based FTL support before anybody can fix Linux
issues, we should disable FTL JIT before you land the removal patch.
Comment 12 Chris Dumez 2016-01-26 09:28:12 PST
6-7% progression on Speedometer as well :)
Comment 13 Chris Dumez 2016-01-26 09:34:39 PST
No obvious impact on PLT though.