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

Restore Session::finish call in socket ref #301

Closed
wants to merge 1 commit into from

Conversation

HeroicKatora
Copy link
Contributor

Using move semantics allows an Option to keep track of the
initialization state while satisfying the borrow checker. This replaces
the functionality that the 'consumed' flag had before. It also retains
the smaller object size since the Option of a reference can use the
representation of the null pointer, which is an invalid reference, as a
niche for the None variant.

Using move semantics allows an Option to keep track of the
initialization state while satisfying the borrow checker. This replaces
the functionality that the 'consumed' flag had before. It also retains
the smaller object size since the Option of a reference can use the
representation of the null pointer, which is an invalid reference, as a
niche for the None variant.
@whitequark
Copy link
Contributor

whitequark commented Jul 23, 2019

Right, I knew this is a potential solution but it irritates me on a fundamental level, because there's no inherent safety issue here, just Rust being fussy about destructuring values that impl Drop, and so paying for this with a branch on every Socket deref feels... wrong. I even wrote an RFC about this.

@whitequark
Copy link
Contributor

Arguably, that branch would not matter much, and I'll merge this if someone makes a convincing argument about that. I just hate it because there's no good reason Rust has to force this overhead on me.

@HeroicKatora
Copy link
Contributor Author

If you really see this as a performance argument, then I urge you to define some benchmarks for it. It shouldn't make any noticable difference in practice, that's at most 2 micro-ops on a perfectly predicted branch. The unwrap failure is even marked as unlikely in the Rust standard library and due to uniqueness of &mut the compiler is perfectly able to hoist the check outside of loops, etc. It's as far from paying for discarded instruction pipelining as it can be.

Should you intend to, nevertheless, not re-instantiate the Session::finish call, that's alright and you can close the PR but maybe think about removing the function altogether. As far as I can tell it's not even implemented anywhere? That would be much less confusing.

@whitequark
Copy link
Contributor

@HeroicKatora I see it more as a code size argument, since most targets smoltcp runs on don't even have micro-ops or branch prediction. But... there's probably lots of lower hanging fruit. So: I'm convinced.

@m-labs-homu r+

@m-labs-homu
Copy link

📌 Commit 108f72d has been approved by whitequark

@m-labs-homu
Copy link

⌛ Testing commit 108f72d with merge efdde33...

Sorry, something went wrong.

@whitequark
Copy link
Contributor

As far as I can tell it's not even implemented anywhere? That would be much less confusing.

The idea behind this function is to set up a poll only on sockets that were actually updated, as opposed to re-polling every socket on each call to iface.poll. It never quite materialized, but I think keeping the option open is good?.. Not sure.

Sorry, something went wrong.

@m-labs-homu
Copy link

☀️ Test successful - status-travis
Approved by: whitequark
Pushing efdde33 to master...

Sorry, something went wrong.

@HeroicKatora HeroicKatora deleted the drop_socket_ref branch July 23, 2019 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants