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

feat(tree_index, #157): implement TreeIndex::peek_entry #160

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

qthree
Copy link
Contributor

@qthree qthree commented Sep 19, 2024

Naive implementation of #157
Tests are passing, but idk about performance impact and lifetime rules in LeafNode::search and InternalNode::search - is it even allowed to pass key reference to outside?

@wvwwvwwv
Copy link
Owner

The compiler is very good at dead code elimination, so there should be no problem in terms of performance (all are inlined). Just briefly looked into it, this seems OK, but I’ll do a thorough code review on the weekend.

@wvwwvwwv
Copy link
Owner

@qthree can you possibly run cargo bench with/without the change? + mark this PR as ready when you think it's ready.

@qthree
Copy link
Contributor Author

qthree commented Sep 21, 2024

main:

TreeIndex: peek         time:   [55.716 ns 56.343 ns 56.837 ns]
                        change: [-3.6546% -0.7751% +2.3509%] (p = 0.62 > 0.05)
                        No change in performance detected.

tree-peek-with:

TreeIndex: peek         time:   [61.050 ns 61.810 ns 62.458 ns]
                        change: [+6.8138% +10.045% +13.459%] (p = 0.00 < 0.05)
                        Performance has regressed.

@qthree qthree marked this pull request as ready for review September 21, 2024 12:19
@wvwwvwwv
Copy link
Owner

I tested similar code in godbolt, but the compiler was able to eliminate the additional key field if inlined. I guess the code is relatively big, so the whole call stack cannot fit into a single compilation unit. I think, you can copy-and-paste the search methods only for peek_entry (search_entry); though most code will be duplicated… performance vs code quality -> performance in this case.

@wvwwvwwv wvwwvwwv merged commit a298240 into wvwwvwwv:main Sep 24, 2024
6 checks passed
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