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 #[inline] and #[cold] in far more places #834

Merged
merged 1 commit into from Nov 4, 2020

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Nov 4, 2020

Fixes #832

I added inline to anything small enough or where the compiler would be able to DCE it. I added cold to functions hit in error paths (even if they're small). I also avoided inlining deprecated functions and std::fmt::Display impls, etc.

This also has a few unrelated changes that I noticed when I went through every file.

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #834 into master will decrease coverage by 0.10%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #834      +/-   ##
==========================================
- Coverage   77.31%   77.21%   -0.11%     
==========================================
  Files          48       48              
  Lines        5506     5512       +6     
==========================================
- Hits         4257     4256       -1     
- Misses       1249     1256       +7     
Impacted Files Coverage Δ
src/backup.rs 63.93% <ø> (ø)
src/blob/mod.rs 89.44% <ø> (+0.62%) ⬆️
src/busy.rs 62.50% <ø> (ø)
src/cache.rs 96.03% <ø> (ø)
src/collation.rs 71.42% <ø> (ø)
src/column.rs 70.45% <ø> (ø)
src/config.rs 85.71% <ø> (ø)
src/context.rs 41.17% <ø> (-3.99%) ⬇️
src/error.rs 11.38% <ø> (ø)
src/functions.rs 79.60% <ø> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7574124...801bf4f. Read the comment docs.

@@ -127,21 +136,13 @@ pub struct MappedRows<'stmt, F> {
map: F,
}

impl<'stmt, T, F> MappedRows<'stmt, F>
Copy link
Member Author

@thomcc thomcc Nov 4, 2020

Choose a reason for hiding this comment

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

MappedRows::new isn't needed if we just implement query_map in terms of query(...).mapped which this patch does too. These changes are unrelated but accidentally got rolled into this patch due to me forgetting to stash some WIP stuff, but it doesn't change behavior or the public API at all and reduces the amount of code.

Ditto for AndThenRows::new

Type::Real => write!(f, "Real"),
Type::Text => write!(f, "Text"),
Type::Blob => write!(f, "Blob"),
Type::Null => f.pad("Null"),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated but allows us to work in cases where a align/width/etc format args are provided. Admittedly, IDK how relevant that is here, but it's better to use Formatter::pad for this sort of thing and I noticed it.

@thomcc
Copy link
Member Author

thomcc commented Nov 4, 2020

This is probably more aggressive about #[inline] than it would be if written from scratch, but this allows a lot of rusqlite code to be much lower overhead compared to direct SQLite api usage when compiled in release mode (#[inline] does nothing in debug mode).

Without this, you could still have that benefit with lto = true but most people dont use that.

@thomcc
Copy link
Member Author

thomcc commented Nov 4, 2020

(Going to land this w/o review, but if you have an issue @gwenn I'm okay with reverting it and filing a PR that goes through review)

@thomcc thomcc merged commit 65c38bf into rusqlite:master Nov 4, 2020
@thomcc thomcc deleted the inline-things branch November 4, 2020 15:59
Copy link
Collaborator

@gwenn gwenn left a comment

Choose a reason for hiding this comment

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

LGTM

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.

More functions should be marked as #[inline]
2 participants