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

Implement LazyBTreeMap and use it in a few places. #50240

Merged
merged 1 commit into from Apr 28, 2018

Conversation

Projects
None yet
5 participants
@nnethercote
Contributor

nnethercote commented Apr 26, 2018

This is a thin wrapper around BTreeMap that avoids allocating upon creation.

I would prefer to change BTreeMap directly to make it lazy (like I did with HashSet in #36734) and I initially attempted that by making BTreeMap::root an Option<>. But then I also had to change Iter and Range to handle trees with no root, and those types have stability markers on them and I wasn't sure if that was acceptable. Also, BTreeMap has a lot of complex code and changing it all was challenging, and I didn't have high confidence about my general approach.

So I prototyped this wrapper instead and used it in the hottest locations to get some measurements about the effect. The measurements are pretty good!

  • Doing a debug build of serde, it reduces the total number of heap allocations from 17,728,709 to 13,359,384, a 25% reduction. The number of bytes allocated drops from 7,474,672,966 to 5,482,308,388, a 27% reduction.

  • It gives speedups of up to 3.6% on some rustc-perf benchmark jobs. crates.io, futures, and serde benefit most.

futures-check
        avg: -1.9%      min: -3.6%      max: -0.5%
serde-check
        avg: -2.1%      min: -3.5%      max: -0.7%
crates.io-check
        avg: -1.7%      min: -3.5%      max: -0.3%
serde
        avg: -2.0%      min: -3.0%      max: -0.9%
serde-opt
        avg: -1.8%      min: -2.9%      max: -0.3%
futures
        avg: -1.5%      min: -2.8%      max: -0.4%
tokio-webpush-simple-check
        avg: -1.1%      min: -2.2%      max: -0.1%
futures-opt
        avg: -1.2%      min: -2.1%      max: -0.4%
piston-image-check
        avg: -0.8%      min: -1.1%      max: -0.3%
crates.io
        avg: -0.6%      min: -1.0%      max: -0.3%

@Gankro, how do you think I should proceed here? Is leaving this as a wrapper reasonable? Or should I try to make BTreeMap itself lazy? If so, can I change the representation of Iter and Range?

Thanks!

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Apr 26, 2018

r? @cramertj

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Apr 26, 2018

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:05:26] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:27] tidy error: /checkout/src/librustc/infer/higher_ranked/mod.rs:250: line longer than 100 chars
[00:05:27] tidy error: /checkout/src/librustc/infer/higher_ranked/mod.rs:346: line longer than 100 chars
[00:05:28] some tidy checks failed
[00:05:28] 
[00:05:28] 
[00:05:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:28] 
[00:05:28] 
[00:05:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:28] Build completed unsuccessfully in 0:02:11
[00:05:28] Build completed unsuccessfully in 0:02:11
[00:05:28] Makefile:79: recipe for target 'tidy' failed
[00:05:28] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:179765a7
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Implement LazyBTreeMap and use it in a few places.
This is a thin wrapper around BTreeMap that avoids allocating upon
creation. It speeds up some rustc-perf benchmarks by up to 3.6%.
@Gankro

This comment has been minimized.

Contributor

Gankro commented Apr 26, 2018

Made the terrible mistake of clicking on this with my phone; will look at this closer tomorrow morning. Please feel free to ping me mercilessly assuming I have forgotten.

I agree this is something that should be looked at seriously, and sympathize with the complexity issues.

Could you elaborate on the stability issues with Iter and Range? ABI isn’t part of stability.

Have you looked at how Google’s C++ BTree handles emptiness?

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Apr 26, 2018

Could you elaborate on the stability issues with Iter and Range? ABI isn’t part of stability.

Here's the current range:

#[stable(feature = "btree_range", since = "1.17.0")]
pub struct Range<'a, K: 'a, V: 'a> {
    front: Handle<NodeRef<marker::Immut<'a>, K, V, marker::Leaf>, marker::Edge>,
    back: Handle<NodeRef<marker::Immut<'a>, K, V, marker::Leaf>, marker::Edge>,
}

Handles don't make sense when there aren't any nodes, so it'll have to change to something like this:

pub enum Range<'a, K: 'a, V: 'a> {
    Empty,
    NonEmpty {
        front: Handle<NodeRef<marker::Immut<'a>, K, V, marker::Leaf>, marker::Edge>,
        back: Handle<NodeRef<marker::Immut<'a>, K, V, marker::Leaf>, marker::Edge>,
    },
}

Which is fine, except I don't understand in detail what the stable marker means, and whether such a change is permitted. I hope so, because people shouldn't be relying on the representation of Range, IIUC. Perhaps that's what you mean by your mention of "ABI" above.

Have you looked at how Google’s C++ BTree handles emptiness?

No. I didn't even know such a thing existed :)

@Gankro

This comment has been minimized.

Contributor

Gankro commented Apr 26, 2018

Google Btree, which i believe has the same high-level design (parent pointers), but is also a bit of a huge templated nightmare: https://code.google.com/archive/p/cpp-btree/

re: stability — yeah we only guarantee basic source-stability for stable types/methods. So just that they exist and work. It’s possible to rely on the layout of these types but that’s unexpected and not ensured (though we would consider an exception if it was really rampant).

I wonder if we could do some kind of static empty sentinel thing..?

@Gankro

This comment has been minimized.

Contributor

Gankro commented Apr 26, 2018

Skimmed the code a bit more, and yeah it seems like a lot of this can be done by just adding checks for len == 0, or making iterators that have dangling-but-equal pointers (similar to how we do empty slices).

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Apr 27, 2018

@Gankro and I discussed some more. This is all a bit complicated. The simplest thing is just to proceed with the patch as is, without moving the laziness into BTreeMap. @cramerjt, does that sound reasonable to you?

@Gankro

This comment has been minimized.

Contributor

Gankro commented Apr 27, 2018

( @cramertj )

@cramertj

This comment has been minimized.

Member

cramertj commented Apr 27, 2018

Yeah, this seems like a fine starting point to me! (thanks for the ping, @Gankro!)

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Apr 27, 2018

📌 Commit 259ae18 has been approved by cramertj

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Apr 28, 2018

Note: @Gankro opened #50266 for making BTreeMap properly lazy.

@bors

This comment has been minimized.

Contributor

bors commented Apr 28, 2018

⌛️ Testing commit 259ae18 with merge 66363b2...

bors added a commit that referenced this pull request Apr 28, 2018

Auto merge of #50240 - nnethercote:LazyBTreeMap, r=cramertj
Implement LazyBTreeMap and use it in a few places.

This is a thin wrapper around BTreeMap that avoids allocating upon creation.

I would prefer to change BTreeMap directly to make it lazy (like I did with HashSet in #36734) and I initially attempted that by making BTreeMap::root an Option<>. But then I also had to change Iter and Range to handle trees with no root, and those types have stability markers on them and I wasn't sure if that was acceptable. Also, BTreeMap has a lot of complex code and changing it all was challenging, and I didn't have high confidence about my general approach.

So I prototyped this wrapper instead and used it in the hottest locations to get some measurements about the effect. The measurements are pretty good!

- Doing a debug build of serde, it reduces the total number of heap allocations from 17,728,709 to 13,359,384, a 25% reduction. The number of bytes allocated drops from 7,474,672,966 to 5,482,308,388, a 27% reduction.

- It gives speedups of up to 3.6% on some rustc-perf benchmark jobs. crates.io, futures, and serde benefit most.
```
futures-check
        avg: -1.9%      min: -3.6%      max: -0.5%
serde-check
        avg: -2.1%      min: -3.5%      max: -0.7%
crates.io-check
        avg: -1.7%      min: -3.5%      max: -0.3%
serde
        avg: -2.0%      min: -3.0%      max: -0.9%
serde-opt
        avg: -1.8%      min: -2.9%      max: -0.3%
futures
        avg: -1.5%      min: -2.8%      max: -0.4%
tokio-webpush-simple-check
        avg: -1.1%      min: -2.2%      max: -0.1%
futures-opt
        avg: -1.2%      min: -2.1%      max: -0.4%
piston-image-check
        avg: -0.8%      min: -1.1%      max: -0.3%
crates.io
        avg: -0.6%      min: -1.0%      max: -0.3%
```
@Gankro, how do you think I should proceed here? Is leaving this as a wrapper reasonable? Or should I try to make BTreeMap itself lazy? If so, can I change the representation of Iter and Range?

Thanks!
@bors

This comment has been minimized.

Contributor

bors commented Apr 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: cramertj
Pushing 66363b2 to master...

@bors bors merged commit 259ae18 into rust-lang:master Apr 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:LazyBTreeMap branch Apr 29, 2018

nnethercote added a commit to nnethercote/rust that referenced this pull request May 14, 2018

Remove LazyBTreeMap.
It was introduced in rust-lang#50240 to avoid an allocation when creating a new
BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the
default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.

@nnethercote nnethercote referenced this pull request May 14, 2018

Merged

Remove LazyBTreeMap. #50740

kennytm added a commit to kennytm/rust that referenced this pull request May 15, 2018

Rollup merge of rust-lang#50740 - nnethercote:rm-LazyBTreeMap, r=cram…
…ertj

Remove LazyBTreeMap.

It was introduced in rust-lang#50240 to avoid an allocation when creating a new
BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the
default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.

kennytm added a commit to kennytm/rust that referenced this pull request May 16, 2018

Rollup merge of rust-lang#50740 - nnethercote:rm-LazyBTreeMap, r=cram…
…ertj

Remove LazyBTreeMap.

It was introduced in rust-lang#50240 to avoid an allocation when creating a new
BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the
default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.

kennytm added a commit to kennytm/rust that referenced this pull request May 16, 2018

Rollup merge of rust-lang#50740 - nnethercote:rm-LazyBTreeMap, r=cram…
…ertj

Remove LazyBTreeMap.

It was introduced in rust-lang#50240 to avoid an allocation when creating a new
BTreeMap, which gave some speed-ups. But then rust-lang#50352 made that the
default behaviour for BTreeMap, so LazyBTreeMap is no longer necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment