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

BTree: initialize node on the heap through initialized header #82226

Closed
wants to merge 1 commit into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Feb 17, 2021

My angle on #81494.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2021
@ssomers
Copy link
Contributor Author

ssomers commented Feb 17, 2021

The benchmarks only really say it's not the same as before.

benchcmp old new --threshold 5
 name                                           old ns/iter  new ns/iter  diff ns/iter   diff %  speedup
 btree::map::clone_fat_100_and_drain_all        135,825      123,130           -12,695   -9.35%   x 1.10
 btree::map::clone_fat_100_and_pop_all          155,275      142,557           -12,718   -8.19%   x 1.09
 btree::map::clone_fat_100_and_remove_all       190,490      179,100           -11,390   -5.98%   x 1.06
 btree::map::clone_fat_val_100_and_drain_all    48,842       56,050              7,208   14.76%   x 0.87
 btree::map::clone_fat_val_100_and_pop_all      59,313       66,690              7,377   12.44%   x 0.89
 btree::map::clone_fat_val_100_and_remove_all   66,970       74,564              7,594   11.34%   x 0.90
 btree::map::clone_slim_10k_and_remove_half     517,690      487,620           -30,070   -5.81%   x 1.06
 btree::map::find_rand_100                      13           12                     -1   -7.69%   x 1.08
 btree::map::find_rand_10_000                   60           56                     -4   -6.67%   x 1.07
 btree::map::find_seq_100                       13           12                     -1   -7.69%   x 1.08
 btree::map::find_seq_10_000                    41           36                     -5  -12.20%   x 1.14
 btree::map::first_and_last_100                 42           48                      6   14.29%   x 0.88
 btree::map::iteration_100000                   522,490      489,535           -32,955   -6.31%   x 1.07
 btree::map::iteration_mut_100000               543,440      485,065           -58,375  -10.74%   x 1.12
 btree::map::range_included_excluded            488,475      427,105           -61,370  -12.56%   x 1.14
 btree::map::range_included_unbounded           147,245      160,521            13,276    9.02%   x 0.92
 btree::set::difference_random_100_vs_10k       3,172        2,834                -338  -10.66%   x 1.12
 btree::set::difference_staggered_100_vs_10k    2,255        3,030                 775   34.37%   x 0.74
 btree::set::intersection_staggered_100_vs_10k  2,003        2,632                 629   31.40%   x 0.76
 btree::set::is_subset_100_vs_100               592          555                   -37   -6.25%   x 1.07
 btree::set::is_subset_100_vs_10k               1,589        1,718                 129    8.12%   x 0.92
 btree::set::is_subset_10k_vs_10k               58,993       64,393              5,400    9.15%   x 0.92

@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 17, 2021
@bors
Copy link
Contributor

bors commented Feb 17, 2021

⌛ Trying commit 9da1244df533250a0faf9e8dd1d1ba02fbfe8c44 with merge 395ce193633fda52d070a349e9f6ddcb6cc456fc...

@rust-log-analyzer

This comment has been minimized.

LeafNode::init(leaf.as_mut_ptr());
let mut leaf = Box::<Self>::new_uninit();
// We only need to initialize the header; the arrays are MaybeUninit.
(*leaf.as_mut_ptr()).head = Head::new();
Copy link
Member

Choose a reason for hiding this comment

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

Should we still use a raw write here? There's no Drop, but I'm not sure if this still technically has implications about the former uninit head value.

Copy link
Contributor Author

@ssomers ssomers Feb 17, 2021

Choose a reason for hiding this comment

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

I don't really know how to do it non-raw. I'm happy when it compiles and when Miri says it the unit tests are okay. Although I haven't updated or run the raw pointer tests lately, so I'm not sure Miri checks anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think you mean the opposite of what I first understood. It isn't a raw write any more (because of #73987) and we probably should still do it.

@ssomers ssomers force-pushed the btree_node_init_2 branch 2 times, most recently from 4521d89 to 29b3646 Compare February 17, 2021 18:14
@ssomers
Copy link
Contributor Author

ssomers commented Feb 17, 2021

First I forgot about gdb_providers, and now that isn't backwards compatible (yet), and it's a pain to test anyway. And if I'm right in #82115 about the odd preference for 32 bit width in generated code,

  • It's not something BTree should deal with.
  • And if it is, the first question should be: why did we store two 16 bit ints in a 64 bit void? They should be half a target_pointer_width wide (or their type is specified as 0..=11, the compiler chooses size and signedness wisely, and then wakes you up from your dream).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-perf Status: Waiting on a perf run to be completed. 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.

None yet

8 participants