Skip to content

[cxx-interop] Use cached record when possible. #34869

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 14, 2021
Merged

[cxx-interop] Use cached record when possible. #34869

merged 1 commit into from
Jan 14, 2021

Conversation

zoecarver
Copy link
Contributor

It is not completely uncommon for a record to get imported while importing its members (for example, if the member points back to the parent record). In this case, simply use the already-imported record. This should improve performance but also prevent an error where a member accidentally had two parents.

This patch also moves around some of the member import/add logic to allow for the above "optimization."

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Nov 24, 2020
@zoecarver
Copy link
Contributor Author

Hopefully, the comments clearly present the issue, but just in case, here is what happens without this patch:

  1. Visit class "A" (1)
  2. class "A" points to class "B"
  3. class "B" points to class "A"
  4. Import and cache class "A"
  5. finish importing class "A" (1)
  6. import member "M" of class "A" (1)
  7. find cached result of member "M"
  8. try to re-add member "M" to class "A" (1)
  9. member "M" is already added to cached version of class "A"
  10. ERROR

@zoecarver zoecarver mentioned this pull request Nov 30, 2020
Merged
@zoecarver
Copy link
Contributor Author

@hlopko friendly ping.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@swift-ci please test OS X platform.

@zoecarver
Copy link
Contributor Author

@swift-ci please test Windows.

@zoecarver
Copy link
Contributor Author

It seems like smoke test is only triggering Linux recently?

@zoecarver
Copy link
Contributor Author

@swift-ci please test Windows.

@swift-ci
Copy link
Contributor

swift-ci commented Dec 4, 2020

Build failed
Swift Test OS X Platform
Git Sha - 37c8457d3ee5bccc7f4a9af29a4becf2b1d152ed

@zoecarver
Copy link
Contributor Author

(Unrelated async issues on 32 bits... retrying.)

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test OS X.

@zoecarver
Copy link
Contributor Author

@hlopko friendly ping.

@zoecarver
Copy link
Contributor Author

@hlopko this patch is actually deceivingly small. It's mostly just a NFC that moves things around. If it would be easier for you to review it if I broke out the non-functional part, I'm happy to do that.

@zoecarver zoecarver force-pushed the cxx/fix/forward-declared-with-child branch from 37c8457 to 79e4995 Compare December 22, 2020 02:51
@zoecarver
Copy link
Contributor Author

@hlopko another friendly ping :)

gribozavr
gribozavr reviewed Jan 13, 2021
View reviewed changes
@zoecarver
[cxx-interop] Use cached record when possible.
ecd1018 
It is not completely uncommon for a record to get imported while
importing it's members (for example, if the member points back to the
parent record). In this case, simply use the already-imported record.
This should improve performance but also prevent an error where a member
accidentally had two parents.

This patch also moves around some of the member import/add logic to
allow for the above "optimization."
@zoecarver zoecarver force-pushed the cxx/fix/forward-declared-with-child branch from 79e4995 to ecd1018 Compare January 13, 2021 19:20
@zoecarver
Copy link
Contributor Author

@gribozavr thanks for the comments! I've addressed all the inline comments (and added some formatting changes). Let me know what you think :)

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux.

@zoecarver zoecarver merged commit 9b0c17e into swiftlang:main Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gribozavr gribozavr gribozavr approved these changes

@hlopko hlopko Awaiting requested review from hlopko

@varungandhi-apple varungandhi-apple Awaiting requested review from varungandhi-apple

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zoecarver @swift-ci @gribozavr