Skip to content

Do not type check lazy accessor bodies eagerly. #3354

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

Merged
merged 1 commit into from
Jul 7, 2016
Merged

Do not type check lazy accessor bodies eagerly. #3354

merged 1 commit into from
Jul 7, 2016

Conversation

rudkx
Copy link
Contributor

@rudkx rudkx commented Jul 6, 2016

What's in this pull request?

Remove some cross-file type checking overhead of lazy property accessors of properties that are typed patterns.


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

Start chipping away at cross-file type checker performance issues by
avoiding type checking of lazy property accessor bodies when the type in
question is defined in a different file and the lazy property is a typed
pattern.

We still type check these in the file they are defined in when we go to
type check the types defined within that file.

Resolves rdar://problem/26894118.

@rudkx rudkx self-assigned this Jul 6, 2016
@rudkx
Copy link
Contributor Author

rudkx commented Jul 6, 2016

@swift-ci Please test.

@rudkx
Copy link
Contributor Author

rudkx commented Jul 6, 2016

@DougGregor Doug, can you take a look at this small change to remove some of the cross-file type checking overhead we're seeing in project builds?

@lattner
Copy link
Contributor

lattner commented Jul 6, 2016

Woo, thank you for working on this @rudkx!

@slavapestov
Copy link
Contributor

Awesome @rudkx! Do you think this optimization should apply to all accessors, not just those for lazy properties? Obviously stored property accessors do not have bodies so typeCheckDecl() does very little work there, but at least for computed properties this might help. I still see a lot of explicit calls to typeCheckDecl() on accessors, but maybe the remaining ones are actually needed.

@rudkx
Copy link
Contributor Author

rudkx commented Jul 6, 2016

@swift-ci Please test.

@rudkx
Copy link
Contributor Author

rudkx commented Jul 6, 2016

@slavapestov Yes, there's definitely more to do here, but I wanted to knock off this piece first, and address the other issues separately. Addressing some things will probably be straightforward like this, but others definitely look like they will require some more extensive reworking.

@jrose-apple
Copy link
Contributor

Can you add a test case for this, to be really sure it's resolved? I'm concerned it's not the accessors that are actually causing the slowdown but the initial value expression itself.

@rudkx
Copy link
Contributor Author

rudkx commented Jul 6, 2016

@jrose-apple I've been thinking about how to test this, along with other type checking performance issues. I think in time we should build a separate type checker performance suite that can be run on the side, e.g. by a bot.

I thought about using -debug-time-function-bodies and FileChecking the output to see which functions get checked, but I noticed that depending on how/where exactly the expression type checker gets run, I've found that the time may get charged to the accessor, the first function that results in the type itself getting checked, or to no function at all (in the case of the initializers of some pattern binding decls). Overall this seems pretty fragile.

Other ideas?

On the specific concern you have, yes, in general we're paying a high cost for non-typed patterns with initializers, type checking them for every file the type appears in. One of the further improvements I want to look at is avoiding that cost when we can (e.g. when we don't need to generate the layout for the type, and when that particular initialized value is not used). From what I saw we don't do that type checking for typed-patterns though (at least for lazy, I need to verify for non-lazy), and I verified in this particular case that the cost was coming from type checking the lazy getter (which has the initializer baked into it).

@jrose-apple
Copy link
Contributor

Thanks for the explanation. -debug-time-function-bodies really does just time function bodies, so you could probably force it to explicitly output a check line by using the common "wrap in a closure for multi-statement initialization" idiom:

lazy var foo: Int = {
  var result = 1
  result += 1
  return result
}()

Because it's a multi-statement closure, its time should never be folded into any other check.

@DougGregor
Copy link
Member

This looks great. Thanks!

Start chipping away at cross-file type checker performance issues by
avoiding type checking of lazy property accessor bodies when the type in
question is defined in a different file and the lazy property is a typed
pattern.

We still type check these in the file they are defined in when we go to
type check the types defined within that file.

Resolves rdar://problem/26894118.
@rudkx
Copy link
Contributor Author

rudkx commented Jul 7, 2016

It turns out the type checker already had a debug option to ensure we don't descend into type checking things we don't want to type check, so I've updated an existing test for this case and verified that it fails without my change and passes with it.

@rudkx
Copy link
Contributor Author

rudkx commented Jul 7, 2016

@swift-ci Smoke test and merge.

@swift-ci swift-ci merged commit 3fbdc0b into swiftlang:master Jul 7, 2016
@rudkx rudkx deleted the fix-26894118 branch July 7, 2016 04:23
@lattner
Copy link
Contributor

lattner commented Jul 7, 2016

Huzzah, thanks again @rudkx!

@YosephHaryanto
Copy link

Hello, I was researching on why the lazy accessor get so long to be compiled.

Am I safe to assume that if I somehow define a lazy accessor with a type A that located in different file, but if I somehow return a type C inside the body, the compiler will allows it, or I fundamentally misunderstood something?

@DougGregor
Copy link
Member

The problem was that we were type-checking the initializer of the lazy variable from all of the other files, even when the variable had a type annotation. If the initializer was large (say, a big closure), that could be an overall compile-time performance problem.

kateinoigakukun pushed a commit to kateinoigakukun/swift that referenced this pull request Sep 5, 2021
…lang#3354)

`pip` is not always installed on macOS, and `pip3` should be enough to build. Let's make the use of `pip` conditional.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants