Skip to content

IGNITE-22989 Add sync/async read-write lock#4233

Merged
rpuch merged 6 commits intoapache:mainfrom
gridgain:ignite-22989
Aug 16, 2024
Merged

IGNITE-22989 Add sync/async read-write lock#4233
rpuch merged 6 commits intoapache:mainfrom
gridgain:ignite-22989

Conversation

@rpuch
Copy link
Contributor

@rpuch rpuch commented Aug 14, 2024

@rpuch rpuch marked this pull request as ready for review August 14, 2024 13:52
* read lock are waiting for write lock to be released, a write lock attempt will be served first when the release happens).
*/
@SuppressWarnings("unused")
private volatile int pendingWriteLocks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not making both parts a single long value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you asking about state and pendingWriteChecks? They don't always get CASed together, so I don't see why we would want to merge them together.

Copy link
Contributor

Choose a reason for hiding this comment

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

They're on a single cache line anyway, so there's already a naturally occurring contention

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they are in the same long, they are guaranteed to be on the same cache line; if they aren't, maybe they will be in different cache lines (this is possible at least in theory, right?). So no gain here, maybe even a loss.

Also, squashing them together complicates the code, which is also a loss.

I see no gain from squashing, only losses.

Comment on lines 276 to 280
CompletableFuture<?> future = runAsync(() -> {
lock.readLock();
lock.readUnlock();
});
future.get(10, SECONDS);
}, executor);
assertThat(future, willCompleteSuccessfully());
Copy link
Contributor

Choose a reason for hiding this comment

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

runWithTimeout does the same, is that correct? That's why I don't like on-liner methods, you just forget that you could use them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same logic, but different type of a closure

@rpuch rpuch merged commit da075b4 into apache:main Aug 16, 2024
@rpuch rpuch deleted the ignite-22989 branch August 16, 2024 07:58
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