After the TMP refactor in r229391 (https://bugs.webkit.org/show_bug.cgi?id=183263), the WinCairo Release build takes 50 minutes to compile JSC alone, compared to only 10 minutes prior to the patch. This is an MSVC-specific issue -- their optimizer has trouble dealing with "too liberal a use of __forceinline". (https://blogs.msdn.microsoft.com/vcblog/2018/01/04/visual-studio-2017-throughput-improvements-and-advice/) We have reported the issue to Microsoft, but in the meantime it would seem wise to define ALWAYS_INLINE as simply "inline" on Windows, which brings the compile time back down to 10 minutes.
Created attachment 336631 [details] Patch
That seems like a pretty wide net to cast to avoid the force-inline problem with the TMP code. Could you instead have a TMP_ALWAYS_INLINE or something like that which only applies to that code? On the other hand, I don't know how much tuning has been done for the Windows build and inlining, so the heuristics we've applied may be totally wrong. Do we have developers who measure whether this changes pessimizes performance?
(In reply to JF Bastien from comment #2) > That seems like a pretty wide net to cast to avoid the force-inline problem > with the TMP code. Could you instead have a TMP_ALWAYS_INLINE or something > like that which only applies to that code? That might be a possibility. I guess there's a similar thing going on here: https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/graphics/FormatConverter.cpp#L39-L43 (...actually I wonder if this one's still necessary in VS2017, hehe.) > On the other hand, I don't know how much tuning has been done for the > Windows build and inlining, so the heuristics we've applied may be totally > wrong. Do we have developers who measure whether this changes pessimizes > performance? That would be great to know, especially as this should apply equally to AppleWin. I could look into performance test comparisons over here as well.
(In reply to Ross Kirsling from comment #3) > (In reply to JF Bastien from comment #2) > > That seems like a pretty wide net to cast to avoid the force-inline problem > > with the TMP code. Could you instead have a TMP_ALWAYS_INLINE or something > > like that which only applies to that code? > > That might be a possibility. I guess there's a similar thing going on here: > https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/ > graphics/FormatConverter.cpp#L39-L43 > (...actually I wonder if this one's still necessary in VS2017, hehe.) > > > On the other hand, I don't know how much tuning has been done for the > > Windows build and inlining, so the heuristics we've applied may be totally > > wrong. Do we have developers who measure whether this changes pessimizes > > performance? > > That would be great to know, especially as this should apply equally to > AppleWin. I could look into performance test comparisons over here as well. Alex / Per / Brent are more informed about WebKit on Windows than I am, so I'll defer to them.
Comment on attachment 336631 [details] Patch I agree with JF that this is too drastic a change. I think we should instead do something like I needed to do in https://bugs.webkit.org/show_bug.cgi?id=125808 and only remove the __forceinline on only the problematic inlines. There are a lot of instances of ALWAYS_INLINE that we probably want to keep.
(In reply to Alex Christensen from comment #5) > Comment on attachment 336631 [details] > Patch > > I agree with JF that this is too drastic a change. I think we should > instead do something like I needed to do in > https://bugs.webkit.org/show_bug.cgi?id=125808 and only remove the > __forceinline on only the problematic inlines. There are a lot of instances > of ALWAYS_INLINE that we probably want to keep. Sounds good. Just thought I'd kick off the conversation from the naive approach. :)
It appears that the issue is indeed quite localized and must have to do with MSVC getting overwhelmed by recursive templates in particular. I've confirmed that: 1. Using non-forced inline for every `setupArgumentsImpl` overload results in a 9.5-minute build. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/CCallHelpers.h#L318-L464 2. Using non-forced inline for `marshallArgumentRegister` alone results in an 11-minute build. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/jit/CCallHelpers.h#L299 These are both potentially acceptable -- does anyone have a clear preference? Perhaps a recommendation of which test(s) would be most useful for perf impact?
The least invasive change would be to make marshallArgumentRegister non-forced-inline on MSVC only.
(In reply to Alex Christensen from comment #8) > The least invasive change would be to make marshallArgumentRegister > non-forced-inline on MSVC only. Sounds good. FWIW, I also checked Sunspider benchmarks, which (unsurprisingly) support the same conclusion: Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. marshallArgumentRegister setupArgumentsImpl 3d-cube 17.1135+-1.2752 ? 17.4554+-1.8218 ? might be 1.0200x slower 3d-morph 12.1973+-0.8931 ? 13.6648+-2.0364 ? might be 1.1203x slower 3d-raytrace 23.5151+-2.4159 ? 25.1138+-7.1721 ? might be 1.0680x slower access-binary-trees 8.3303+-0.5779 ? 8.7787+-2.4330 ? might be 1.0538x slower access-fannkuch 14.8218+-2.2912 ? 15.4262+-7.4340 ? might be 1.0408x slower access-nbody 11.0309+-1.1147 10.1383+-0.2915 might be 1.0880x faster access-nsieve 6.6365+-0.8363 6.3742+-0.4838 might be 1.0411x faster bitops-3bit-bits-in-byte 5.1899+-0.1658 ? 5.8102+-1.2525 ? might be 1.1195x slower bitops-bits-in-byte 7.3887+-2.3269 6.6924+-0.1452 might be 1.1040x faster bitops-bitwise-and 9.1475+-1.1505 ? 9.3076+-1.3356 ? might be 1.0175x slower bitops-nsieve-bits 7.7549+-1.3521 7.7289+-0.6407 controlflow-recursive 7.2060+-0.4298 ! 8.4953+-0.8057 ! definitely 1.1789x slower crypto-aes 19.2986+-8.2190 18.0129+-3.0947 might be 1.0714x faster crypto-md5 8.2644+-1.2072 7.7612+-1.1438 might be 1.0648x faster crypto-sha1 8.1423+-1.5990 7.9152+-0.6251 might be 1.0287x faster date-format-tofte 35.1650+-0.6483 ? 36.1490+-2.2741 ? might be 1.0280x slower date-format-xparb 21.6351+-2.0519 21.2189+-0.1581 might be 1.0196x faster math-cordic 11.2579+-2.4418 9.3053+-0.4511 might be 1.2098x faster math-partial-sums 13.2822+-1.8823 13.0983+-2.1215 might be 1.0140x faster math-spectral-norm 7.7226+-0.4115 7.6494+-0.2857 regexp-dna 11.4542+-0.2199 ? 12.5857+-3.0776 ? might be 1.0988x slower string-base64 13.4223+-1.0573 ? 15.1614+-2.0637 ? might be 1.1296x slower string-fasta 20.6008+-2.9576 ? 21.6987+-6.0305 ? might be 1.0533x slower string-tagcloud 32.4288+-0.5497 ? 34.3539+-3.1244 ? might be 1.0594x slower string-unpack-code 84.0179+-8.3223 ? 90.4980+-27.9551 ? might be 1.0771x slower string-validate-input 23.8311+-1.8061 ? 24.0583+-1.7425 ? <arithmetic> 16.9560+-0.5638 ? 17.4789+-0.6725 ? might be 1.0308x slower
Created attachment 336731 [details] Patch
Additionally, it looks like that "C1063 Fatal Error" no longer occurs for FormatConverter.cpp, so we should be able to restore the ALWAYS_INLINE there in a follow-up patch.
(In reply to Ross Kirsling from comment #11) > Additionally, it looks like that "C1063 Fatal Error" no longer occurs for > FormatConverter.cpp, so we should be able to restore the ALWAYS_INLINE there > in a follow-up patch. I've been meaning to look into whether we should just remove the ALWAYS_INLINE there. It could make the binary significantly smaller while de-optiizing the outer loop, which would probably be a performance gain.
Comment on attachment 336731 [details] Patch Clearing flags on attachment: 336731 Committed r230062: <https://trac.webkit.org/changeset/230062>
All reviewed patches have been landed. Closing bug.
<rdar://problem/38980095>
Somehow this patch caused Mac port to break. We can't load apple.com now :(
Re-opened since this is blocked by bug 184128
(In reply to Ryosuke Niwa from comment #16) > Somehow this patch caused Mac port to break. We can't load apple.com now :( This change makes no change on Mac. You have almost certainly misdiagnosed
(In reply to Alex Christensen from comment #18) > (In reply to Ryosuke Niwa from comment #16) > > Somehow this patch caused Mac port to break. We can't load apple.com now :( > > This change makes no change on Mac. You have almost certainly misdiagnosed You say that but Safari builds before this patch loads web pages file, and Safari builds after this patch cause web content process to instantly crash. The crash reproduces at r230069 but doesn't after the rollout in r230070. So your assertion that this change makes no change on Mac is obviously wrong. It certainly affects Mac even though the code change doesn't seem like it should.
At r230075 (after the rollout, running Xcode 9.2 on Sierra), I can confirm that: - `run-minibrowser` succeeds with or without the patch (both release and debug) - the "play" button in Xcode fails with or without the patch (WebProcess crash on launch) I'm not sure what's causing the issue in Xcode, but it's not this patch; I will plan to reland accordingly.
Woah, let Ryosuke double check before relanding.
(In reply to Ross Kirsling from comment #20) > At r230075 (after the rollout, running Xcode 9.2 on Sierra), I can confirm > that: What about r230069 before the rollout?
(In reply to Alex Christensen from comment #21) > Woah, let Ryosuke double check before relanding. Whoops, okay. Standing by. (In reply to Alex Christensen from comment #22) > What about r230069 before the rollout? Not sure. Builds are a half-hour apiece so I've been working on this since 9:15 AM.
(In reply to Ross Kirsling from comment #20) > - `run-minibrowser` succeeds with or without the patch (both release and > debug) Er, to be more clear, there's no reproduction with `build-webkit [--debug]` followed by `run-minibrowser [--debug]` or `run-safari [--debug]` on the command line. I don't really understand what differs between this and the build-and-run from within Xcode.
The definition of ALWAYS_INLINE depends on NDEBUG, so you'll need to do a release build.
(In reply to Alex Christensen from comment #25) > The definition of ALWAYS_INLINE depends on NDEBUG, so you'll need to do a > release build. Sure, I did both across the board to be extra careful.
Alright, one more scenario just to make things perfectly unambiguous. :D 1. git checkout dd7a49d554b8583f61cfda3f0a42ecdf3ed5b6de # i.e. r230061, the revision immediately before this patch landed 2. git clean -ffdx # because why not 3. Open Xcode, Product > Clean 4. Hit the Play button => WebContent process crashes immediately.
(In reply to Ross Kirsling from comment #27) > Alright, one more scenario just to make things perfectly unambiguous. :D > > 1. git checkout dd7a49d554b8583f61cfda3f0a42ecdf3ed5b6de # i.e. r230061, the > revision immediately before this patch landed > > 2. git clean -ffdx # because why not > > 3. Open Xcode, Product > Clean > > 4. Hit the Play button > > => WebContent process crashes immediately. ...or apparently this doesn't count as a thorough clean, from Xcode's perspective. Evidently there's a file: /Users/.../Library/Developer/Xcode/DerivedData/WebKit-.../Build/Products/Debug/WebKit.framework/Resources/com.apple.WebProcess.sb To which my comment from this patch was added: > // Sometimes needed due to MSVC optimizer sensitivities. And that's syntactically invalid amid s-expressions. This leaves me with many questions, but I'm guessing that there's a reason why only block comments are used in WTF/Compiler.h. :(
Aha! Change the comment style and resubmit!
Actually, just remove the comment.
Created attachment 336823 [details] Patch for landing
(In reply to Alex Christensen from comment #18) > (In reply to Ryosuke Niwa from comment #16) > This change makes no change on Mac. You have almost certainly misdiagnosed For the record, you did not misdiagnose, Ryosuke.
(In reply to Alex Christensen from comment #32) > (In reply to Alex Christensen from comment #18) > > (In reply to Ryosuke Niwa from comment #16) > > This change makes no change on Mac. You have almost certainly misdiagnosed > For the record, you did not misdiagnose, Ryosuke. Indeed, what a trip. I've created https://bugs.webkit.org/show_bug.cgi?id=184166 to track the real bug here. :)
Comment on attachment 336823 [details] Patch for landing Clearing flags on attachment: 336823 Committed r230091: <https://trac.webkit.org/changeset/230091>