Skip to content

Conversation

@gui1117
Copy link
Contributor

@gui1117 gui1117 commented Sep 29, 2025

We are doing 2 reads in one time. But this is not good.

Per parking lot doc:

/// This lock uses a task-fair locking policy which avoids both reader and
/// writer starvation. This means that readers trying to acquire the lock will
/// block even if the lock is unlocked when there are writers waiting to acquire
/// the lock. Because of this, attempts to recursively acquire a read lock
/// within a single thread may result in a deadlock.

@gui1117 gui1117 marked this pull request as ready for review September 29, 2025 11:02
@gui1117 gui1117 added the T0-node This PR/Issue is related to the topic “node”. label Sep 29, 2025
pub fn maintain(&self) {
log::trace!(target: LOG_TARGET, "Started store maintenance");
let deleted = self.index.write().maintain(self.timestamp());
let mut index = self.index.write();
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to keep the write lock for the whole duration of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somehow I thought unnamed variable would still be dropped at the end of their "scope" but indeed it is dropped instantly.

Everywhere in the code it seems to lock the index until the operation of the DB is done. But not here.

Copy link
Contributor Author

@gui1117 gui1117 Sep 29, 2025

Choose a reason for hiding this comment

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

I just removed this change then, it is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can have this situation:

  • index and db have statement s1 in expired.
  • maintain starts: index has no expired statements, db still have s1 in expired
  • other operation in between: s1 is submitted then s2 is submitted in the same channel as s1: index has s2 as statement, s1 as expired statement, db has s2 as statement and s1 as expired statement
  • maintain finishes: remove the statement s1 from expired in db: now index has s2 as statement and s1 as expired, but db only has s2 as statement. They are inconsistent, but it is not harmful AFAICT.

Comment on lines 674 to 675
let mut index = self.index.write();
let deleted = index.maintain(self.timestamp());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut index = self.index.write();
let deleted = index.maintain(self.timestamp());
let deleted: Vec<_> = {
let deleted = self.index.write().maintain(self.timestamp());
deleted.into_iter().map(|hash| (col::EXPIRED, hash.to_vec(), None)).collect()
};
let (purged, active, expired) = {
let index = self.index.read();
(deleted.len(), index.total_size, index.expired.len())
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed this change, indeed it is not needed the write lock is dropped instantly so there is no deadlock.

fn statements(&self) -> Result<Vec<(Hash, Statement)>> {
let index = self.index.read();
let mut result = Vec::with_capacity(index.entries.len());
for h in self.index.read().entries.keys() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@AndreiEres
Copy link
Contributor

I would prefer to continue work in #9868 where I can add the other stuff related to statement-store performance

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 30, 2025

closed in favor of #9868

@gui1117 gui1117 closed this Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T0-node This PR/Issue is related to the topic “node”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants