Skip to content
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

Rebind less load more #2055

Merged
merged 3 commits into from
Mar 7, 2022
Merged

Rebind less load more #2055

merged 3 commits into from
Mar 7, 2022

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Mar 1, 2022

Reduce the amount of rebinding we do with sockaddr types.

Motivation:

SR-15912 recently caused a bunch of issues with compilation of NIO in the nightlies. While these were arguably Swift bugs, we're also skating pretty close to the edge by using withMemoryRebound here: it's often safer for us to load the structures, rather than use random pointers.

Modifications:

  • Replace a bunch of rebindings with loads

Result:

Fewer calls with withMemoryRebound.

@Lukasa Lukasa added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Mar 1, 2022
$0.withMemoryRebound(to: sockaddr_in.self, capacity: 1) {
$0.pointee
}
return withUnsafeMutableBytes(of: &self) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be withUnsafeBytes(of:) (and elsewhere)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wonder if this function actually needs to be mutating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This func is mutating because on older Swift releases we didn't have a version of withUnsafeBytes that did not take an inout as an argument. It's possible we dropped all the Swift releases where that's true, let me check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be fine, I've updated the PR with removal of the mutatings.

Copy link
Member

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

@Lukasa
Copy link
Contributor Author

Lukasa commented Mar 2, 2022

@swift-nio-bot test this please

@Lukasa Lukasa enabled auto-merge (squash) March 2, 2022 22:12
Copy link
Contributor

@glessard glessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your improvements to my drive-by fix.

@Lukasa
Copy link
Contributor Author

Lukasa commented Mar 7, 2022

@swift-nio-bot test this please

@Lukasa Lukasa merged commit b2195f7 into apple:main Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants