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

Make list slightly less embarrassingly slow #515

Merged
merged 14 commits into from
Aug 3, 2024

Conversation

irevoire
Copy link
Contributor

@irevoire irevoire commented Jul 31, 2024

Make list faster in some cases.

Part of #448

On my computer, running:

time cargo run --release -- -e 'sum(map(sqrt, range(0, 10_000)))'

Take 42s on main vs 0.4s on my branch.

I don't think this approach is general enough, though. Having pattern matching would make performant numbat code easier to write.


How does it works?

I replaced the NumbatList type with a custom ArcListView type that shares its allocation with all other list referring to the same allocation.
It hold a reference counted ptr to the allocation + the bounds accessible by the allocation:

/// Reference counted list / list view
#[derive(Clone, Eq)]
pub struct ArcListView<T> {
    /// The original alloc shared between all values
    alloc: Arc<VecDeque<T>>,
    /// The indexes accessible to us. If `None` we own the whole allocation
    view: Option<(usize, usize)>,
}

Then, doing a tail is a matter of advancing the starting view by one.
head simply returns the first element.
And push_front check if he's the only owner of the allocation, if it's the case it never allocate (this is always the case while building the list for example so its important).
if it's not the only owner of the allocation, it clones the allocation entirely. We could probably do something smarter here with a chained list or something, but since it yields pretty good results, I didn't try harder.

We now need to take extra care while implementing PartialEq


EDIT: While playing with list a little bit I noticed that the following code was still taking around 15s:

let a = now()
let list = range(0, 10_000)
print("Made the list in {human(now() - a)}")

let b = now()
let reversed = reverse(list)
print("Reversed the list in {human(now() - b)}")

Which is expected considering the number of operations we're doing to push something at the end of a list even though we internally uses a vecdeque.
By providing an ffi cons_end function I went from 15s to 0.002s

@sharkdp
Copy link
Owner

sharkdp commented Aug 1, 2024

Wow — thank you! Excellent idea and fantastic results! I'm really looking forward to this. I recently played a lot with #73 and that is going to become so much faster.

Can we move ArcListView to a new module (maybe call it just ListView or NumbatList or similar?) and add some unit tests?

I plan to do a more detailed review soon.

@irevoire
Copy link
Contributor Author

irevoire commented Aug 2, 2024

Done ✅
The PR is ready to review again

numbat/src/ffi/lists.rs Outdated Show resolved Hide resolved
numbat/src/ffi/lists.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
numbat/src/list.rs Outdated Show resolved Hide resolved
@irevoire irevoire requested a review from sharkdp August 3, 2024 10:44
numbat/src/list.rs Outdated Show resolved Hide resolved
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you for the updates and explanations.

Fantastic work!

@sharkdp sharkdp merged commit c556dad into sharkdp:master Aug 3, 2024
15 checks passed
@irevoire irevoire deleted the make-list-slightly-faster branch August 3, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants