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

add faster safe rust variant that does not drop needlessly #52

Merged
merged 2 commits into from May 18, 2018

Conversation

@vbarrielle
Copy link
Contributor

@vbarrielle vbarrielle commented May 18, 2018

In the split and merge parts, calls to drop were inserted to delete
options that had been taken, ie were already none. Thus the calls to
drop would always conclude they didn't need to drop, but this would waste
computational resources. Using the swap and forget pattern, we can
safely inform the compiler that the taken node does not actually need to
be freed.

This significantly decreased the runtime on my machine (from ~0.5s for the idiomatic version down to ~0.3s for the new version).

In the split and merge parts, calls to drop were inserted to delete
options that had been taken, ie were already none. Thus the calls to
drop would always conclude they didn't need to drop, but this would waste
computational resources. Using the swap and forget pattern, we can
safely inform the compiler that the taken node does not actually need to
be freed.
if let Some(mut orig_node) = orig {
if orig_node.x < value {
let mut split_pair = split_binary(orig_node.right.take(), value);
::std::mem::swap(&mut orig_node.right, &mut split_pair.0);
Copy link
Owner

@frol frol May 18, 2018

Choose a reason for hiding this comment

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

::std:: can be dropped here, right?

Loading

Copy link
Contributor Author

@vbarrielle vbarrielle May 18, 2018

Choose a reason for hiding this comment

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

Right, I've updated to fix this.

Loading

@frol
Copy link
Owner

@frol frol commented May 18, 2018

@vbarrielle Nice trick! Thank you! This dropped the execution time from 0.37s to 0.24s while still keeping the safety guarantees!

Loading

@frol frol merged commit c5e9205 into frol:master May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants