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

Avoid slices where individuals are good enough #68451

Closed
wants to merge 1 commit into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 22, 2020

As mentioned in #67686, slices on immutable trees are almost exclusively used to get_unchecked a particular item or position. The only exception happens through the (locally) public keys function used only by search_linear.

A step beyond is to not make slices on immutable trees at all, but pass in an index argument. This:

  • Simplifies the code because the callers no longer have to do the get_unchecked and such themselves.
  • Avoids "leaking" the slice concept outside node.rs (for instance, a project to efficiently store a key-value pair with a 3-byte key and a 1-byte value would be a local change in node.rs) (that's really an independent point, which we could address by keeping the slice-returning function function keys as non-public, and tweaking search_linear just like is done n this PR)
  • (On first sight, looks like it would speed up things because we no longer fetch length and construct slices, but benchmarks report no significant difference. Seems this was optimized out anyway.)
  • But makes immutable access more different from mutable access (it's impossible to transform mutable slice access in the same way, because for insertion at least you really need to know slice length);
  • Somewhat deceives readers, because the references are used as slice pointers in ptr::copy_nonoverlapping calls; but then again, the current implementation does the same just in time: it just assumes that the source slice and the destination slices are the same.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2020
@shepmaster
Copy link
Member

This is (or will be, I suppose) beyond my knowledge :-)

r? @LukasKalbertodt

@Mark-Simulacrum
Copy link
Member

I've been doing review of the BTreeMap patches, so r? @Mark-Simulacrum

@ssomers
Copy link
Contributor Author

ssomers commented Jan 22, 2020

It's definitely of much less use, priority and conviction than my other PRs (the useful parts have been merged in through other PRs). Not sure why github displays a "Ready for review" button on a draft PR, yet bothers reviewers from the start.

PS oh right, it's assignee, not reviewer
PPS well no, the label says S-waiting-on-review - but that's probably rust-highfive's choice, not github's

@bors
Copy link
Contributor

bors commented Jan 30, 2020

☔ The latest upstream changes (presumably #68659) made this pull request unmergeable. Please resolve the merge conflicts.

@ssomers
Copy link
Contributor Author

ssomers commented Feb 2, 2020

When I rebase on a very recent master, which includes the clone_from implementation, test_clone_from panics on 'assertion failed: idx < self.len()' (reported on Windows only) and/or on a unwrap_unchecked in navigate::next_unchecked_deallocating. No problem testing with Miri (apart from that test lasting 5 minutes), no problem on the master itself (obviously). No idea why but I guess something is not right about this PR.

@ssomers ssomers closed this Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants