Skip to content

Commit bac6e4e

Browse files
committed
index: split wanted/unwanted RevWalkQueue instances
When working on common_ancestors_pos() optimization, I noticed it's slower to track items of different states in a single BinaryHeap than tracking them separately. This is also true for RevWalk implementations. The new code is simpler since we no longer have to count unwanted items manually. ``` revsets/v1.0.0..v2.40.0 ----------------------- 1 1.00 4.9±0.04ms 0 1.15 5.6±0.02ms ``` (in git repo)
1 parent 1aa0831 commit bac6e4e

File tree

2 files changed

+91
-132
lines changed

2 files changed

+91
-132
lines changed

lib/src/default_index/rev_walk.rs

Lines changed: 79 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use super::composite::CompositeIndex;
2828
use super::entry::IndexPosition;
2929
use super::entry::SmallIndexPositionsVec;
3030
use super::rev_walk_queue::RevWalkQueue;
31-
use super::rev_walk_queue::RevWalkWorkItemState;
31+
use super::rev_walk_queue::RevWalkWorkItem;
3232

3333
/// Like `Iterator`, but doesn't borrow the `index` internally.
3434
pub(super) trait RevWalk<I: ?Sized> {
@@ -331,12 +331,16 @@ impl<'a> RevWalkBuilder<'a> {
331331

332332
fn ancestors_with_min_pos(self, min_pos: IndexPosition) -> RevWalkAncestors<'a> {
333333
let index = self.index;
334-
let mut queue = RevWalkQueue::with_min_pos(min_pos);
335-
queue.extend_wanted(self.wanted, ());
336-
queue.extend_unwanted(self.unwanted);
334+
let mut wanted_queue = RevWalkQueue::with_min_pos(min_pos);
335+
let mut unwanted_queue = RevWalkQueue::with_min_pos(min_pos);
336+
wanted_queue.extend(self.wanted, ());
337+
unwanted_queue.extend(self.unwanted, ());
337338
RevWalkBorrowedIndexIter {
338339
index,
339-
walk: RevWalkImpl { queue },
340+
walk: RevWalkImpl {
341+
wanted_queue,
342+
unwanted_queue,
343+
},
340344
}
341345
}
342346

@@ -348,14 +352,16 @@ impl<'a> RevWalkBuilder<'a> {
348352
generation_range: Range<u32>,
349353
) -> RevWalkAncestorsGenerationRange<'a> {
350354
let index = self.index;
351-
let mut queue = RevWalkQueue::with_min_pos(IndexPosition::MIN);
355+
let mut wanted_queue = RevWalkQueue::with_min_pos(IndexPosition::MIN);
356+
let mut unwanted_queue = RevWalkQueue::with_min_pos(IndexPosition::MIN);
352357
let item_range = RevWalkItemGenerationRange::from_filter_range(generation_range.clone());
353-
queue.extend_wanted(self.wanted, Reverse(item_range));
354-
queue.extend_unwanted(self.unwanted);
358+
wanted_queue.extend(self.wanted, Reverse(item_range));
359+
unwanted_queue.extend(self.unwanted, ());
355360
RevWalkBorrowedIndexIter {
356361
index,
357362
walk: RevWalkGenerationRangeImpl {
358-
queue,
363+
wanted_queue,
364+
unwanted_queue,
359365
generation_end: generation_range.end,
360366
},
361367
}
@@ -415,18 +421,20 @@ impl<'a> RevWalkBuilder<'a> {
415421
let positions = self.ancestors_until_roots(root_positions.iter().copied());
416422
let descendants_index = RevWalkDescendantsIndex::build(index, positions);
417423

418-
let mut queue = RevWalkQueue::with_min_pos(Reverse(IndexPosition::MAX));
424+
let mut wanted_queue = RevWalkQueue::with_min_pos(Reverse(IndexPosition::MAX));
425+
let unwanted_queue = RevWalkQueue::with_min_pos(Reverse(IndexPosition::MAX));
419426
let item_range = RevWalkItemGenerationRange::from_filter_range(generation_range.clone());
420427
for pos in root_positions {
421428
// Do not add unreachable roots which shouldn't be visited
422429
if descendants_index.contains_pos(pos) {
423-
queue.push_wanted(Reverse(pos), Reverse(item_range));
430+
wanted_queue.push(Reverse(pos), Reverse(item_range));
424431
}
425432
}
426433
RevWalkOwnedIndexIter {
427434
index: descendants_index,
428435
walk: RevWalkGenerationRangeImpl {
429-
queue,
436+
wanted_queue,
437+
unwanted_queue,
430438
generation_end: generation_range.end,
431439
},
432440
}
@@ -439,33 +447,23 @@ pub(super) type RevWalkAncestors<'a> =
439447
#[derive(Clone)]
440448
#[must_use]
441449
pub(super) struct RevWalkImpl<P> {
442-
queue: RevWalkQueue<P, ()>,
450+
wanted_queue: RevWalkQueue<P, ()>,
451+
unwanted_queue: RevWalkQueue<P, ()>,
443452
}
444453

445454
impl<I: RevWalkIndex + ?Sized> RevWalk<I> for RevWalkImpl<I::Position> {
446455
type Item = I::Position;
447456

448457
fn next(&mut self, index: &I) -> Option<Self::Item> {
449-
while let Some(item) = self.queue.pop() {
450-
self.queue.skip_while_eq(&item.pos);
451-
if item.is_wanted() {
452-
self.queue
453-
.extend_wanted(index.adjacent_positions(item.pos), ());
454-
return Some(item.pos);
455-
} else if self.queue.wanted_count() == 0 {
456-
// No more wanted entries to walk
457-
debug_assert!(!self.queue.iter().any(|x| x.is_wanted()));
458-
return None;
459-
} else {
460-
self.queue
461-
.extend_unwanted(index.adjacent_positions(item.pos));
458+
while let Some(item) = self.wanted_queue.pop() {
459+
self.wanted_queue.skip_while_eq(&item.pos);
460+
if flush_queue_until(&mut self.unwanted_queue, index, item.pos).is_some() {
461+
continue;
462462
}
463+
self.wanted_queue
464+
.extend(index.adjacent_positions(item.pos), ());
465+
return Some(item.pos);
463466
}
464-
465-
debug_assert_eq!(
466-
self.queue.iter().filter(|x| !x.is_wanted()).count(),
467-
self.queue.unwanted_count()
468-
);
469467
None
470468
}
471469
}
@@ -481,7 +479,8 @@ pub(super) type RevWalkDescendantsGenerationRange = RevWalkOwnedIndexIter<
481479
#[must_use]
482480
pub(super) struct RevWalkGenerationRangeImpl<P> {
483481
// Sort item generations in ascending order
484-
queue: RevWalkQueue<P, Reverse<RevWalkItemGenerationRange>>,
482+
wanted_queue: RevWalkQueue<P, Reverse<RevWalkItemGenerationRange>>,
483+
unwanted_queue: RevWalkQueue<P, ()>,
485484
generation_end: u32,
486485
}
487486

@@ -498,54 +497,41 @@ impl<P: Ord> RevWalkGenerationRangeImpl<P> {
498497
start: gen.start + 1,
499498
end: gen.end.saturating_add(1),
500499
};
501-
self.queue
502-
.extend_wanted(index.adjacent_positions(pos), Reverse(succ_gen));
500+
self.wanted_queue
501+
.extend(index.adjacent_positions(pos), Reverse(succ_gen));
503502
}
504503
}
505504

506505
impl<I: RevWalkIndex + ?Sized> RevWalk<I> for RevWalkGenerationRangeImpl<I::Position> {
507506
type Item = I::Position;
508507

509508
fn next(&mut self, index: &I) -> Option<Self::Item> {
510-
while let Some(item) = self.queue.pop() {
511-
if let RevWalkWorkItemState::Wanted(Reverse(mut pending_gen)) = item.state {
512-
let mut some_in_range = pending_gen.contains_end(self.generation_end);
513-
while let Some(x) = self.queue.pop_eq(&item.pos) {
514-
// Merge overlapped ranges to reduce number of the queued items.
515-
// For queries like `:(heads-)`, `gen.end` is close to `u32::MAX`, so
516-
// ranges can be merged into one. If this is still slow, maybe we can add
517-
// special case for upper/lower bounded ranges.
518-
if let RevWalkWorkItemState::Wanted(Reverse(gen)) = x.state {
519-
some_in_range |= gen.contains_end(self.generation_end);
520-
pending_gen = if let Some(merged) = pending_gen.try_merge_end(gen) {
521-
merged
522-
} else {
523-
self.enqueue_wanted_adjacents(index, item.pos, pending_gen);
524-
gen
525-
};
526-
} else {
527-
unreachable!("no more unwanted items of the same entry");
528-
}
529-
}
530-
self.enqueue_wanted_adjacents(index, item.pos, pending_gen);
531-
if some_in_range {
532-
return Some(item.pos);
533-
}
534-
} else if self.queue.wanted_count() == 0 {
535-
// No more wanted entries to walk
536-
debug_assert!(!self.queue.iter().any(|x| x.is_wanted()));
537-
return None;
538-
} else {
539-
self.queue.skip_while_eq(&item.pos);
540-
self.queue
541-
.extend_unwanted(index.adjacent_positions(item.pos));
509+
while let Some(item) = self.wanted_queue.pop() {
510+
if flush_queue_until(&mut self.unwanted_queue, index, item.pos).is_some() {
511+
self.wanted_queue.skip_while_eq(&item.pos);
512+
continue;
513+
}
514+
let Reverse(mut pending_gen) = item.value;
515+
let mut some_in_range = pending_gen.contains_end(self.generation_end);
516+
while let Some(x) = self.wanted_queue.pop_eq(&item.pos) {
517+
// Merge overlapped ranges to reduce number of the queued items.
518+
// For queries like `:(heads-)`, `gen.end` is close to `u32::MAX`, so
519+
// ranges can be merged into one. If this is still slow, maybe we can add
520+
// special case for upper/lower bounded ranges.
521+
let Reverse(gen) = x.value;
522+
some_in_range |= gen.contains_end(self.generation_end);
523+
pending_gen = if let Some(merged) = pending_gen.try_merge_end(gen) {
524+
merged
525+
} else {
526+
self.enqueue_wanted_adjacents(index, item.pos, pending_gen);
527+
gen
528+
};
529+
}
530+
self.enqueue_wanted_adjacents(index, item.pos, pending_gen);
531+
if some_in_range {
532+
return Some(item.pos);
542533
}
543534
}
544-
545-
debug_assert_eq!(
546-
self.queue.iter().filter(|x| !x.is_wanted()).count(),
547-
self.queue.unwanted_count()
548-
);
549535
None
550536
}
551537
}
@@ -589,6 +575,22 @@ impl RevWalkItemGenerationRange {
589575
}
590576
}
591577

578+
/// Walks queue items until `bottom_pos`. Returns item if found at `bottom_pos`.
579+
fn flush_queue_until<I: RevWalkIndex + ?Sized>(
580+
queue: &mut RevWalkQueue<I::Position, ()>,
581+
index: &I,
582+
bottom_pos: I::Position,
583+
) -> Option<RevWalkWorkItem<I::Position, ()>> {
584+
while let Some(item) = queue.pop_if(|x| x.pos >= bottom_pos) {
585+
queue.skip_while_eq(&item.pos);
586+
queue.extend(index.adjacent_positions(item.pos), ());
587+
if item.pos == bottom_pos {
588+
return Some(item);
589+
}
590+
}
591+
None
592+
}
593+
592594
/// Walks descendants from the roots, in order of ascending index position.
593595
pub(super) type RevWalkDescendants<'a> =
594596
RevWalkBorrowedIndexIter<'a, CompositeIndex, RevWalkDescendantsImpl>;
@@ -899,22 +901,22 @@ mod tests {
899901
let to_commit_id = |pos| index.entry_by_pos(pos).commit_id();
900902

901903
let mut iter = make_iter(&[id_6.clone(), id_7.clone()], &[id_3.clone()]);
902-
assert_eq!(iter.walk.queue.len(), 2);
904+
assert_eq!(iter.walk.wanted_queue.len(), 2);
903905
assert_eq!(iter.next().map(to_commit_id), Some(id_7.clone()));
904906
assert_eq!(iter.next().map(to_commit_id), Some(id_6.clone()));
905907
assert_eq!(iter.next().map(to_commit_id), Some(id_5.clone()));
906-
assert_eq!(iter.walk.queue.len(), 2);
908+
assert_eq!(iter.walk.wanted_queue.len(), 2);
907909
assert_eq!(iter.next().map(to_commit_id), Some(id_4.clone()));
908-
assert_eq!(iter.walk.queue.len(), 1); // id_1 shouldn't be queued
910+
assert_eq!(iter.walk.wanted_queue.len(), 1); // id_1 shouldn't be queued
909911
assert_eq!(iter.next().map(to_commit_id), Some(id_3.clone()));
910-
assert_eq!(iter.walk.queue.len(), 0); // id_2 shouldn't be queued
912+
assert_eq!(iter.walk.wanted_queue.len(), 0); // id_2 shouldn't be queued
911913
assert!(iter.next().is_none());
912914

913915
let iter = make_iter(&[id_6.clone(), id_7.clone(), id_2.clone()], &[id_3.clone()]);
914-
assert_eq!(iter.walk.queue.len(), 2); // id_2 shouldn't be queued
916+
assert_eq!(iter.walk.wanted_queue.len(), 2); // id_2 shouldn't be queued
915917

916918
let iter = make_iter(&[id_6.clone(), id_7.clone()], &[]);
917-
assert_eq!(iter.walk.queue.len(), 0); // no ids should be queued
919+
assert_eq!(iter.walk.wanted_queue.len(), 0); // no ids should be queued
918920
}
919921

920922
#[test]

0 commit comments

Comments
 (0)