BtreeMap range_search spruced up by ssomers · Pull Request #68499 · rust-lang/rust

ssomers · 2020-01-24T00:20:17Z

#39457 created a lower level entry point for range_search to operate on, but it's really not hard to move it up a level of abstraction, making it somewhat shorter and reusing existing unsafe code (new_edge is unsafe although it is currently not tagged as such).

Benchmark added. Comparison says there's no real difference:

>cargo benchcmp old3.txt new3.txt --threshold 5
 name                                           old3.txt ns/iter  new3.txt ns/iter  diff ns/iter   diff %  speedup
 btree::map::find_seq_100                       19                21                           2   10.53%   x 0.90
 btree::map::range_excluded_unbounded           3,117             2,838                     -279   -8.95%   x 1.10
 btree::map::range_included_unbounded           1,768             1,871                      103    5.83%   x 0.94
 btree::set::intersection_10k_neg_vs_10k_pos    35                37                           2    5.71%   x 0.95
 btree::set::intersection_staggered_100_vs_10k  2,488             2,314                     -174   -6.99%   x 1.08
 btree::set::is_subset_10k_vs_100               3                 2                           -1  -33.33%   x 1.50

r? @Mark-Simulacrum

bors · 2020-01-30T05:48:10Z

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

Mark-Simulacrum

r=me, just let me know what you think of my comment(s) and whether you want to do them here or not

src/liballoc/collections/btree/map.rs

Mark-Simulacrum · 2020-02-06T21:58:07Z

src/liballoc/collections/btree/search.rs

Could we move/copy the doc comment here onto search_range since that's the common "entry point" now?

Doesn't have to be in this PR.

Right, though I couldn't find any other documentation describing a returned enum (other than Result or Option).

Uh, not sure how that's relevant? We rarely encounter returned enums in the wild, I think (though I'm sure now that I've said it I'll think of dozens of examples once I post this).

If you're looking for a format or so to follow, I wouldn't worry that much about that. I'd be far more interested in just some docs pointing out what the variants mean; basically same as the docs here do wrt to false/true and the index, but applied to the enum.

I found it hard to describe without mentioning the variant (or at least, the GoDown variant which is somewhat confusing).

Mentioning the variant is good (and indeed I would expect the docs to do so).

ssomers · 2020-02-07T01:08:05Z

whether you want to do

Oops, I wanted and I did.

Mark-Simulacrum · 2020-02-07T01:09:53Z

src/liballoc/collections/btree/search.rs

❤️ Thanks for adding some docs here too :)

Mark-Simulacrum · 2020-02-07T01:11:55Z

src/liballoc/collections/btree/search.rs

Hm, I imagine it's not irrelevant if the type is immut -- it's still that edge, right? e.g. a hypothetical [T]::binary_search-like API on BTree would want that index/edge.

I don't understand. It's all in the "node is a leaf" case, why would you care if you won't insert?

Well, maybe it's not my business what someone wants to do with it, but it feels wrong to suggest it's the place to insert if it's an immutable handle.

There's (eventual) plans to add something like the LinkedList Cursor API to BTreeMap so that you could actually navigate in the conceptual "Array" that the map represents. (In fact, the linked list cursors are considered as a proving ground for that API design).

As another example -- though also an API that I believe BTree's don't provide today -- say I have a non-existent needle x, and I want to find the predecessor/successor of that x without traversing O(n) elements (as the iterator API would force me to). With this index, I could return that information fairly easily with O(log(n)) accesses, right? (where n is the length/size of the map).

And I didn't want to say "if BorrowType is marker::Mut", because I guess you can use it for "marker::Owned", but I'm not sure.

It's the way binary_search on slices describes it, and this is the same API (I believe) so I think it's reasonable to do so here too.

If the value is found then Result::Ok is returned, containing the index of the matching element. If there are multiple matches, then any one of the matches could be returned. If the value is not found then Result::Err is returned, containing the index where a matching element could be inserted while maintaining sorted order.

You're absolutely right. How about "belongs"?

I didn't see that comment. Yes, "could be inserted" sounds good, and "place to insert" also doesn't say the handle can be used to insert, it's just a location.

Anyway, going offline now.

(The quote was from the binary search docs, to be clear).

Could be inserted is fine, or some other wording. Anything works :)

Mark-Simulacrum · 2020-02-07T01:54:34Z

@bors r+

Great, thanks!

bors · 2020-02-07T01:54:35Z

📌 Commit ae03e16 has been approved by Mark-Simulacrum

bors · 2020-02-07T06:25:02Z

⌛ Testing commit ae03e16 with merge b5e21db...

@Mark-Simulacrum

BtreeMap range_search spruced up #39457 created a lower level entry point for `range_search` to operate on, but it's really not hard to move it up a level of abstraction, making it somewhat shorter and reusing existing unsafe code (`new_edge` is unsafe although it is currently not tagged as such). Benchmark added. Comparison says there's no real difference: ``` >cargo benchcmp old3.txt new3.txt --threshold 5 name old3.txt ns/iter new3.txt ns/iter diff ns/iter diff % speedup btree::map::find_seq_100 19 21 2 10.53% x 0.90 btree::map::range_excluded_unbounded 3,117 2,838 -279 -8.95% x 1.10 btree::map::range_included_unbounded 1,768 1,871 103 5.83% x 0.94 btree::set::intersection_10k_neg_vs_10k_pos 35 37 2 5.71% x 0.95 btree::set::intersection_staggered_100_vs_10k 2,488 2,314 -174 -6.99% x 1.08 btree::set::is_subset_10k_vs_100 3 2 -1 -33.33% x 1.50 ``` r? @Mark-Simulacrum

bors · 2020-02-07T09:41:43Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing b5e21db to master...

rust-highfive assigned Mark-Simulacrum Jan 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2020

Mark-Simulacrum approved these changes Feb 6, 2020

View reviewed changes

Create benchmarks for BTreeMap::range

be051ad

Mark-Simulacrum reviewed Feb 7, 2020

View reviewed changes

Lift range_search up one level of abstraction

ae03e16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 7, 2020

bors merged commit ae03e16 into rust-lang:master Feb 7, 2020

ssomers deleted the btree_search_tidying branch March 27, 2020 13:17

Uh oh!

Conversation

ssomers commented Jan 24, 2020

Uh oh!

bors commented Jan 30, 2020

Uh oh!

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssomers commented Feb 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssomers Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Feb 7, 2020

Uh oh!

bors commented Feb 7, 2020

Uh oh!

bors commented Feb 7, 2020

Uh oh!

bors commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ssomers Feb 7, 2020 •

edited

Loading