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

introduce slots #176

Merged
merged 16 commits into from
Jul 2, 2019
Merged

Conversation

nikomatsakis
Copy link
Member

As discussed on Zulip this PR series optimizes our internal dependency data structures to avoid hashing of keys/values on query revalidation. My measurements found this to be approximately a ~35% win, though @matklad saw results up to 50%.

This PR does introduce some subtle logic around Send+Sync owing to the use of trait objects. It requires a bit of a hack to ensure that the database is Send+Sync iff the keys/values are Send+Sync. I found what feels like a decent solution to this problem by transmuting a Arc<dyn Slot> to a Arc<dyn Slot + Send + Sync> and using phantom-data to ensure that the composite is only send+sync if the keys/values are send+sync. The truth is that this is kind of overkill since slots are not exposed outside of the database, and if the keys/values are not send+sync, the database as a whole won't be -- but it still feels good to try and be precise here.

version from scratch trivial change x 10
master ~4.5s 815-820ms
approx. LRU ~4.5s 537ms
precise LRU 4.5-4.8s 540-550ms

Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Haven't looked in depth, but noticed a couple of odd things!

src/dependency.rs Outdated Show resolved Hide resolved
src/input.rs Show resolved Hide resolved
src/lru.rs Outdated Show resolved Hide resolved
src/lru/test.rs Outdated Show resolved Hide resolved
src/lru.rs Outdated Show resolved Hide resolved
@matklad
Copy link
Member

matklad commented Jul 2, 2019

@nikomatsakis wanna rebase?

We could use e.g. intrusive-collections but from reading the docs and
surveying the source it wasn't *obvious* to me that it had the right
semantics.
The unsafe impl now asserts that the `DatabaseSlot` implementor type
is indeed `Send+Sync` if `DB::DatabaseData` is `Send+Sync`. Since our
query keys/values are a part of database-data, this means that `Slot`
must be `Send+Sync` if the key/value are `Send+Sync`. We test this
with a function that will cause compliation to fail if we accidentally
introduce an `Rc<T>` etc.
@nikomatsakis
Copy link
Member Author

@matklad been doing that =)

@nikomatsakis nikomatsakis merged commit 636e48d into salsa-rs:master Jul 2, 2019
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