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

refactor(perf): Remove extra page indirection in Table #710

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Feb 21, 2025

By manually erasing the concrete slot types and dealing with the vtable ourselves

Copy link

netlify bot commented Feb 21, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 4b6dde4
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67c9bbd4efbdd700089fc94f

Copy link

codspeed-hq bot commented Feb 21, 2025

CodSpeed Performance Report

Merging #710 will improve performances by 4.31%

Comparing Veykril:veykril/push-vzrqlltwrwmu (4b6dde4) with master (9d2a978)

Summary

⚡ 6 improvements
✅ 5 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
amortized[SupertypeInput] 4.5 µs 4.3 µs +4.31%
new[Input] 11.6 µs 11.2 µs +4.1%
new[SupertypeInput] 17.2 µs 16.6 µs +3.81%
mutating[10] 14.5 µs 14 µs +3.55%
mutating[20] 14.7 µs 14.2 µs +3.49%
mutating[30] 14.8 µs 14.3 µs +3.46%

@Veykril
Copy link
Member Author

Veykril commented Feb 21, 2025

Results look promising

@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch 2 times, most recently from 85ebb05 to 5ba9ff5 Compare February 21, 2025 10:17
@Veykril
Copy link
Member Author

Veykril commented Feb 21, 2025

Alright all cleaned up, this will collide with #649

@Veykril Veykril changed the title [WIP] Remove extra page indirection in Table Remove extra page indirection in Table Feb 21, 2025
@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch from 5ba9ff5 to 1a8456e Compare February 21, 2025 10:18
@Veykril
Copy link
Member Author

Veykril commented Feb 21, 2025

We could likely give Accumulated a similar treatment to remove the Box<dyn Accumulated>, though that might be a bit trickier since we need to take care of the layout instability of Vec used within somehow

@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch from 1a8456e to da5d96b Compare February 22, 2025 15:12
@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch 4 times, most recently from bbebda9 to 54fc519 Compare February 25, 2025 15:10
@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch from 54fc519 to 2ae5f93 Compare February 26, 2025 06:42
let index = self.allocated.load(Ordering::Acquire);
if index == PAGE_LEN {
let _guard = self.0.allocation_lock.lock();
let index = self.0.allocated.load(Ordering::Acquire);
Copy link
Member Author

@Veykril Veykril Feb 26, 2025

Choose a reason for hiding this comment

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

Given we only ever modify allocated in this function while the allocation_lock is held, could we use Relaxed ordering for the load here ? I believe so as the lock call should already incur a happens-before relationship for the participating threads right?

Copy link
Member Author

@Veykril Veykril Feb 26, 2025

Choose a reason for hiding this comment

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

In fact it should be fine to make all loads (and the store in the critical section) relaxed due to the sole store happening within the critical section right? (not gonna include this in the PR either way)

@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch 3 times, most recently from cf213ee to f99f889 Compare February 26, 2025 14:33
@Veykril Veykril changed the title Remove extra page indirection in Table [perf] Remove extra page indirection in Table Mar 5, 2025
By manually erasing the concrete slot types and dealing with the vtable ourselves
@Veykril Veykril force-pushed the veykril/push-vzrqlltwrwmu branch from f99f889 to 4b6dde4 Compare March 6, 2025 15:14
@Veykril
Copy link
Member Author

Veykril commented Mar 6, 2025

I like the new lower limit, it makes my PRs pop out more now :^)

@Veykril Veykril changed the title [perf] Remove extra page indirection in Table refactor(perf): Remove extra page indirection in Table Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

1 participant