-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Implement legacy UTF8 APIs in terms of new components #9188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@swift-ci Please benchmark |
@swift-ci Please test |
Build comment file:Optimized (O) Regression (14)
Improvement (17)
No Changes (238)
Regression (7)
Improvement (3)
No Changes (259)
|
@swift-ci Please smoke test |
@swift-ci Please smoke test linux platform |
@swift-ci Please smoke test OS X platform |
Build comment file:Optimized (O) Regression (16)
Improvement (17)
No Changes (236)
Regression (5)
Improvement (12)
No Changes (252)
|
37f3153
to
b1ebc5c
Compare
@swift-ci Please benchmark |
@swift-ci Please benchmark |
@swift-ci Please smoke test |
Build comment file:Optimized (O) Regression (11)
Improvement (21)
No Changes (237)
Regression (3)
Improvement (13)
No Changes (253)
|
@swift-ci Please smoke test and merge |
Build comment file:Optimized (O) Regression (11)
Improvement (28)
No Changes (230)
Regression (5)
Improvement (12)
No Changes (252)
|
Do we have an idea what happened with MapReduceString and StringEdits? Anything we can do to recoup? |
@aschwaighofer this PR contains a bunch of commits that change inlining and incidentally remove gobs of ARC traffic. It would be really helpful to understand whether these things are caused by the calling convention, and if not, whether there's anything we can do to control them. |
@airspeedswift As you surmised from the speculative revert commits, I'm having a hard time finding a magic combination that appears to eliminate all regressions. |
If adding @inline(__always) affects the result that means that the optimizer was able to actually inline the function (because it could identify it as the callee). This means that it can as well specialize the function by changing it from owned to guaranteed - which it does (almost? @gottesmm) unconditionally (it needs to find the post dominating release (set). does that fail often @gottesmm ?). i.e it very likely did change the calling convention to @guaranteed before you added the @inline(_always) annotation. Therefore, changing the default convention would probably not do us any good here because the optimizer already does it for us. What inlining in general gives more context to other optimizations. This is good in two directions:
Can you point at a specific regression you want me to look at? |
@aschwaighofer Yes. I am not sure. I gave this code over to Xin and he changed the pass significantly. Erik and I are maintaining it now. But it would be good for someone to go back in and see if we are missing any patterns. |
@aschwaighofer None of the regressions that come from just taking the first commit, without the additional inlining commits, stood out for me. They all had the same character when I analyzed them: R/R traffic where it shouldn't exist, suppressable by inlining the invoker of that R/R traffic into its caller. We could run another benchmark in CI against a PR that reverts all the inlining commits in this one if you want to know what those are, but I suspect you would want a local run anyway(?) |
@aschwaighofer There's a pretty good, simple example of this in https://bugs.swift.org/browse/SR-4837 |
No description provided.