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.

Sign up for GitHub

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

Jump to bottom

Extend RWMutex Interface for locker package #1135

Merged
merged 9 commits into from
Feb 11, 2025
Merged

Extend RWMutex Interface for locker package #1135

merged 9 commits into from
Feb 11, 2025

Conversation

raararaara
Copy link
Contributor

@raararaara raararaara commented Feb 4, 2025 •
edited by hackerwins
Loading

What this PR does / why we need it:

This PR addresses the bug reported in Issue #1081, which was identified in version v0.5.6. The core issue involves a mismatch between doc.presences and doc.changeID.versionVector counts under certain scenarios.

This patch is a prerequisite for future changes that involve removing Lamport clocks from the version vector. The practical application of rwMutex will be after the temporary preservation logic introduced in PR #1090 , which was added to address GC errors, is resolved.

Any background context you want to provide?

The implementation of rwMutex resolves the problem by ensuring that updateVersionVector() is consistently executed without interference from concurrent routines. This change is necessary to maintain the integrity of the version vector in scenarios where a client detaches while concurrently processing changes from another client.

Consider the situation where Client A is to be detached and Client B triggers changes. If Client B's changes lead to the execution of PushPullChanges (Read; no local changes) on the yorkieServer for Client A, and Client A's detachment occurs nearly simultaneously at the clusterServer, the order of execution for updateVersionVector() becomes critical.

Should the detachment process remove the version vector first, followed by the PushPull (read), the subsequent upsert operation of updateVersionVector() could inadvertently recreate the version vector. This situation arises from the lack of isolation between the read and write routines, which this PR seeks to rectify through the rwMutex.

Limitations

Even with routine isolation, there are cases where updateVersionVector() may not execute properly due to 'context canceled' errors. A solution for this issue needs to be considered.

Which issue(s) this PR fixes:

Address #1081

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation:

Checklist:

  • Added relevant tests or not required
  • Addressed and resolved all CodeRabbit review comments
  • Didn't break anything

Summary by CodeRabbit

  • New Features
    • Enhanced the synchronization mechanism with concurrent read access, improving overall performance and reliability.
  • Tests
    • Expanded the test suite to validate the new locking behaviors and ensure robust concurrency control.
  • Chores
    • Introduced benchmarking to measure the performance of the updated locking functionality.

@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2025 •
edited
Loading

Walkthrough

The changes update the locking mechanism across multiple modules. In the locker package, the implementation switches to a read-write mutex with added RLock and RUnlock methods and adjusted waiters count logic. New tests have been added to validate the concurrency and proper sequencing of read and write locks. In the backend sync module, corresponding read-lock methods have been introduced into the Locker interface and its implementation. Additionally, server RPC methods now conditionally acquire locks based on presence of changes, and new benchmark tests evaluate the performance of the updated read-write locking functionality.

Changes

File(s) Change Summary
pkg/locker/locker.go Switched from standard mutex to sync.RWMutex, updated Lock/Unlock methods, added RLock/RUnlock methods, and refined waiter count management.
pkg/locker/locker_test.go Introduced new tests (TestRWLockerRLock, TestLockRLock, TestRWLockerConcurrency) and modified existing tests to accommodate updated lock behavior.
server/backend/sync/locker.go Added RLock and RUnlock methods (with cancelable context) to both the Locker interface and the internalLocker struct.
server/rpc/yorkie_server.go Added conditional checks using pack.HasChanges() to determine when to instantiate and acquire a lock in PushPullChanges and RemoveDocument methods.
test/bench/locker_bench_test.go Added BenchmarkRWLocker and benchmarkRWLockerParallel functions to assess the performance of the new read-write locking mechanism.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as "Caller"
    participant Locker as "lockCtr / Locker"
    participant RWMutex as "sync.RWMutex"
    
    Caller->>Locker: Call RLock() / Lock()
    Locker->>RWMutex: Acquire read/write lock (RLock or Lock)
    RWMutex-->>Locker: Lock acquired
    Note over Locker: Increment waiters count
    Caller->>Locker: Call RUnlock() / Unlock()
    Locker->>RWMutex: Release read/write lock (RUnlock or Unlock)
    RWMutex-->>Locker: Lock released
    Note over Locker: Decrement waiters count
Loading 
sequenceDiagram
    participant Client as "Client"
    participant YorkieServer as "yorkieServer"
    participant Locker as "Locker"
    participant Resource as "Document Resource"
  
    Client->>YorkieServer: Request (PushPullChanges/RemoveDocument)
    YorkieServer->>YorkieServer: Check pack.HasChanges()
    alt Changes Exist
        YorkieServer->>Locker: Instantiate and lock critical section
        Locker-->>YorkieServer: Lock acquired
        YorkieServer->>Resource: Process document changes
        YorkieServer->>Locker: Unlock critical section
    else No Changes
        YorkieServer->>Resource: Process without locking
    end
    YorkieServer-->>Client: Send response
Loading

Assessment against linked issues

Objective Addressed Explanation
Ensure GC and versionVector consistency for documents after v0.5.6 (#1081) ❌ The changes focus on locking improvements; no modifications related to GC or versionVector.

Possibly related PRs

Suggested reviewers

  • hackerwins
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov Codecov
Copy link

codecov bot commented Feb 5, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 69.23077% with 16 lines in your changes missing coverage. Please review.

Project coverage is 38.59%. Comparing base (2ba679b) to head (5fe9f7b).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
server/backend/sync/locker.go 0.00% 9 Missing ⚠️
pkg/locker/locker.go 82.92% 5 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
- Coverage   47.11%   38.59%   -8.53%     
==========================================
  Files          83      165      +82     
  Lines       12333    25293   +12960     
==========================================
+ Hits         5811     9761    +3950     
- Misses       5935    14712    +8777     
- Partials      587      820     +233

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions[bot]
github-actions bot reviewed Feb 7, 2025
View reviewed changes
Copy link

@github-actions github-actions bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Go Benchmark

Benchmark suite Current: 5fe9f7b Previous: 94533dd Ratio
BenchmarkDocument/constructor_test 1425 ns/op 1385 B/op 24 allocs/op 1424 ns/op 1385 B/op 24 allocs/op 1.00
BenchmarkDocument/constructor_test - ns/op 1425 ns/op 1424 ns/op 1.00
BenchmarkDocument/constructor_test - B/op 1385 B/op 1385 B/op 1
BenchmarkDocument/constructor_test - allocs/op 24 allocs/op 24 allocs/op 1
BenchmarkDocument/status_test 990.5 ns/op 1353 B/op 22 allocs/op 1096 ns/op 1353 B/op 22 allocs/op 0.90
BenchmarkDocument/status_test - ns/op 990.5 ns/op 1096 ns/op 0.90
BenchmarkDocument/status_test - B/op 1353 B/op 1353 B/op 1
BenchmarkDocument/status_test - allocs/op 22 allocs/op 22 allocs/op 1
BenchmarkDocument/equals_test 7657 ns/op 7562 B/op 129 allocs/op 8548 ns/op 7562 B/op 129 allocs/op 0.90
BenchmarkDocument/equals_test - ns/op 7657 ns/op 8548 ns/op 0.90
BenchmarkDocument/equals_test - B/op 7562 B/op 7562 B/op 1
BenchmarkDocument/equals_test - allocs/op 129 allocs/op 129 allocs/op 1
BenchmarkDocument/nested_update_test 16841 ns/op 12307 B/op 258 allocs/op 16883 ns/op 12307 B/op 258 allocs/op 1.00
BenchmarkDocument/nested_update_test - ns/op 16841 ns/op 16883 ns/op 1.00
BenchmarkDocument/nested_update_test - B/op 12307 B/op 12307 B/op 1
BenchmarkDocument/nested_update_test - allocs/op 258 allocs/op 258 allocs/op 1
BenchmarkDocument/delete_test 22928 ns/op 15789 B/op 339 allocs/op 23027 ns/op 15788 B/op 339 allocs/op 1.00
BenchmarkDocument/delete_test - ns/op 22928 ns/op 23027 ns/op 1.00
BenchmarkDocument/delete_test - B/op 15789 B/op 15788 B/op 1.00
BenchmarkDocument/delete_test - allocs/op 339 allocs/op 339 allocs/op 1
BenchmarkDocument/object_test 8487 ns/op 7034 B/op 118 allocs/op 8542 ns/op 7034 B/op 118 allocs/op 0.99
BenchmarkDocument/object_test - ns/op 8487 ns/op 8542 ns/op 0.99
BenchmarkDocument/object_test - B/op 7034 B/op 7034 B/op 1
BenchmarkDocument/object_test - allocs/op 118 allocs/op 118 allocs/op 1
BenchmarkDocument/array_test 29811 ns/op 12139 B/op 273 allocs/op 28428 ns/op 12139 B/op 273 allocs/op 1.05
BenchmarkDocument/array_test - ns/op 29811 ns/op 28428 ns/op 1.05
BenchmarkDocument/array_test - B/op 12139 B/op 12139 B/op 1
BenchmarkDocument/array_test - allocs/op 273 allocs/op 273 allocs/op 1
BenchmarkDocument/text_test 34160 ns/op 15188 B/op 484 allocs/op 32168 ns/op 15188 B/op 484 allocs/op 1.06
BenchmarkDocument/text_test - ns/op 34160 ns/op 32168 ns/op 1.06
BenchmarkDocument/text_test - B/op 15188 B/op 15188 B/op 1
BenchmarkDocument/text_test - allocs/op 484 allocs/op 484 allocs/op 1
BenchmarkDocument/text_composition_test 31301 ns/op 18702 B/op 501 allocs/op 31567 ns/op 18702 B/op 501 allocs/op 0.99
BenchmarkDocument/text_composition_test - ns/op 31301 ns/op 31567 ns/op 0.99
BenchmarkDocument/text_composition_test - B/op 18702 B/op 18702 B/op 1
BenchmarkDocument/text_composition_test - allocs/op 501 allocs/op 501 allocs/op 1
BenchmarkDocument/rich_text_test 85659 ns/op 39353 B/op 1146 allocs/op 86412 ns/op 39353 B/op 1146 allocs/op 0.99
BenchmarkDocument/rich_text_test - ns/op 85659 ns/op 86412 ns/op 0.99
BenchmarkDocument/rich_text_test - B/op 39353 B/op 39353 B/op 1
BenchmarkDocument/rich_text_test - allocs/op 1146 allocs/op 1146 allocs/op 1
BenchmarkDocument/counter_test 18124 ns/op 11811 B/op 253 allocs/op 18184 ns/op 11811 B/op 253 allocs/op 1.00
BenchmarkDocument/counter_test - ns/op 18124 ns/op 18184 ns/op 1.00
BenchmarkDocument/counter_test - B/op 11811 B/op 11811 B/op 1
BenchmarkDocument/counter_test - allocs/op 253 allocs/op 253 allocs/op 1
BenchmarkDocument/text_edit_gc_100 1384108 ns/op 864950 B/op 17282 allocs/op 1403309 ns/op 864916 B/op 17282 allocs/op 0.99
BenchmarkDocument/text_edit_gc_100 - ns/op 1384108 ns/op 1403309 ns/op 0.99
BenchmarkDocument/text_edit_gc_100 - B/op 864950 B/op 864916 B/op 1.00
BenchmarkDocument/text_edit_gc_100 - allocs/op 17282 allocs/op 17282 allocs/op 1
BenchmarkDocument/text_edit_gc_1000 51930380 ns/op 46839397 B/op 185588 allocs/op 51333352 ns/op 46838029 B/op 185583 allocs/op 1.01
BenchmarkDocument/text_edit_gc_1000 - ns/op 51930380 ns/op 51333352 ns/op 1.01
BenchmarkDocument/text_edit_gc_1000 - B/op 46839397 B/op 46838029 B/op 1.00
BenchmarkDocument/text_edit_gc_1000 - allocs/op 185588 allocs/op 185583 allocs/op 1.00
BenchmarkDocument/text_split_gc_100 2087054 ns/op 1581060 B/op 15951 allocs/op 2093575 ns/op 1581080 B/op 15950 allocs/op 1.00
BenchmarkDocument/text_split_gc_100 - ns/op 2087054 ns/op 2093575 ns/op 1.00
BenchmarkDocument/text_split_gc_100 - B/op 1581060 B/op 1581080 B/op 1.00
BenchmarkDocument/text_split_gc_100 - allocs/op 15951 allocs/op 15950 allocs/op 1.00
BenchmarkDocument/text_split_gc_1000 130638668 ns/op 137791004 B/op 184998 allocs/op 127257232 ns/op 137788694 B/op 184992 allocs/op 1.03
BenchmarkDocument/text_split_gc_1000 - ns/op 130638668 ns/op 127257232 ns/op 1.03
BenchmarkDocument/text_split_gc_1000 - B/op 137791004 B/op 137788694 B/op 1.00
BenchmarkDocument/text_split_gc_1000 - allocs/op 184998 allocs/op 184992 allocs/op 1.00
BenchmarkDocument/text_delete_all_10000 17061140 ns/op 10578389 B/op 56139 allocs/op 18187573 ns/op 10576869 B/op 56136 allocs/op 0.94
BenchmarkDocument/text_delete_all_10000 - ns/op 17061140 ns/op 18187573 ns/op 0.94
BenchmarkDocument/text_delete_all_10000 - B/op 10578389 B/op 10576869 B/op 1.00
BenchmarkDocument/text_delete_all_10000 - allocs/op 56139 allocs/op 56136 allocs/op 1.00
BenchmarkDocument/text_delete_all_100000 262082282 ns/op 105526424 B/op 566109 allocs/op 253549180 ns/op 105537140 B/op 566098 allocs/op 1.03
BenchmarkDocument/text_delete_all_100000 - ns/op 262082282 ns/op 253549180 ns/op 1.03
BenchmarkDocument/text_delete_all_100000 - B/op 105526424 B/op 105537140 B/op 1.00
BenchmarkDocument/text_delete_all_100000 - allocs/op 566109 allocs/op 566098 allocs/op 1.00
BenchmarkDocument/text_100 236077 ns/op 120907 B/op 5181 allocs/op 234457 ns/op 120908 B/op 5181 allocs/op 1.01
BenchmarkDocument/text_100 - ns/op 236077 ns/op 234457 ns/op 1.01
BenchmarkDocument/text_100 - B/op 120907 B/op 120908 B/op 1.00
BenchmarkDocument/text_100 - allocs/op 5181 allocs/op 5181 allocs/op 1
BenchmarkDocument/text_1000 2554089 ns/op 1156083 B/op 51084 allocs/op 2480369 ns/op 1156443 B/op 51084 allocs/op 1.03
BenchmarkDocument/text_1000 - ns/op 2554089 ns/op 2480369 ns/op 1.03
BenchmarkDocument/text_1000 - B/op 1156083 B/op 1156443 B/op 1.00
BenchmarkDocument/text_1000 - allocs/op 51084 allocs/op 51084 allocs/op 1
BenchmarkDocument/array_1000 1297503 ns/op 1088139 B/op 11879 allocs/op 1257409 ns/op 1088185 B/op 11879 allocs/op 1.03
BenchmarkDocument/array_1000 - ns/op 1297503 ns/op 1257409 ns/op 1.03
BenchmarkDocument/array_1000 - B/op 1088139 B/op 1088185 B/op 1.00
BenchmarkDocument/array_1000 - allocs/op 11879 allocs/op 11879 allocs/op 1
BenchmarkDocument/array_10000 13448264 ns/op 9887694 B/op 120729 allocs/op 13681071 ns/op 9887263 B/op 120728 allocs/op 0.98
BenchmarkDocument/array_10000 - ns/op 13448264 ns/op 13681071 ns/op 0.98
BenchmarkDocument/array_10000 - B/op 9887694 B/op 9887263 B/op 1.00
BenchmarkDocument/array_10000 - allocs/op 120729 allocs/op 120728 allocs/op 1.00
BenchmarkDocument/array_gc_100 135475 ns/op 99879 B/op 1266 allocs/op 133610 ns/op 99891 B/op 1266 allocs/op 1.01
BenchmarkDocument/array_gc_100 - ns/op 135475 ns/op 133610 ns/op 1.01
BenchmarkDocument/array_gc_100 - B/op 99879 B/op 99891 B/op 1.00
BenchmarkDocument/array_gc_100 - allocs/op 1266 allocs/op 1266 allocs/op 1
BenchmarkDocument/array_gc_1000 1476774 ns/op 1140979 B/op 12926 allocs/op 1435397 ns/op 1140909 B/op 12926 allocs/op 1.03
BenchmarkDocument/array_gc_1000 - ns/op 1476774 ns/op 1435397 ns/op 1.03
BenchmarkDocument/array_gc_1000 - B/op 1140979 B/op 1140909 B/op 1.00
BenchmarkDocument/array_gc_1000 - allocs/op 12926 allocs/op 12926 allocs/op 1
BenchmarkDocument/counter_1000 207841 ns/op 178136 B/op 5771 allocs/op 201849 ns/op 178135 B/op 5771 allocs/op 1.03
BenchmarkDocument/counter_1000 - ns/op 207841 ns/op 201849 ns/op 1.03
BenchmarkDocument/counter_1000 - B/op 178136 B/op 178135 B/op 1.00
BenchmarkDocument/counter_1000 - allocs/op 5771 allocs/op 5771 allocs/op 1
BenchmarkDocument/counter_10000 2225548 ns/op 2068969 B/op 59778 allocs/op 2156373 ns/op 2068953 B/op 59778 allocs/op 1.03
BenchmarkDocument/counter_10000 - ns/op 2225548 ns/op 2156373 ns/op 1.03
BenchmarkDocument/counter_10000 - B/op 2068969 B/op 2068953 B/op 1.00
BenchmarkDocument/counter_10000 - allocs/op 59778 allocs/op 59778 allocs/op 1
BenchmarkDocument/object_1000 1477417 ns/op 1437384 B/op 9925 allocs/op 1440665 ns/op 1437299 B/op 9925 allocs/op 1.03
BenchmarkDocument/object_1000 - ns/op 1477417 ns/op 1440665 ns/op 1.03
BenchmarkDocument/object_1000 - B/op 1437384 B/op 1437299 B/op 1.00
BenchmarkDocument/object_1000 - allocs/op 9925 allocs/op 9925 allocs/op 1
BenchmarkDocument/object_10000 14477380 ns/op 12350849 B/op 101229 allocs/op 14831135 ns/op 12350388 B/op 101226 allocs/op 0.98
BenchmarkDocument/object_10000 - ns/op 14477380 ns/op 14831135 ns/op 0.98
BenchmarkDocument/object_10000 - B/op 12350849 B/op 12350388 B/op 1.00
BenchmarkDocument/object_10000 - allocs/op 101229 allocs/op 101226 allocs/op 1.00
BenchmarkDocument/tree_100 1056618 ns/op 951029 B/op 6102 allocs/op 1014688 ns/op 951028 B/op 6102 allocs/op 1.04
BenchmarkDocument/tree_100 - ns/op 1056618 ns/op 1014688 ns/op 1.04
BenchmarkDocument/tree_100 - B/op 951029 B/op 951028 B/op 1.00
BenchmarkDocument/tree_100 - allocs/op 6102 allocs/op 6102 allocs/op 1
BenchmarkDocument/tree_1000 80472092 ns/op 86582458 B/op 60112 allocs/op 76348428 ns/op 86582358 B/op 60113 allocs/op 1.05
BenchmarkDocument/tree_1000 - ns/op 80472092 ns/op 76348428 ns/op 1.05
BenchmarkDocument/tree_1000 - B/op 86582458 B/op 86582358 B/op 1.00
BenchmarkDocument/tree_1000 - allocs/op 60112 allocs/op 60113 allocs/op 1.00
BenchmarkDocument/tree_10000 9666355105 ns/op 8581197936 B/op 600186 allocs/op 9527164193 ns/op 8581199056 B/op 600193 allocs/op 1.01
BenchmarkDocument/tree_10000 - ns/op 9666355105 ns/op 9527164193 ns/op 1.01
BenchmarkDocument/tree_10000 - B/op 8581197936 B/op 8581199056 B/op 1.00
BenchmarkDocument/tree_10000 - allocs/op 600186 allocs/op 600193 allocs/op 1.00
BenchmarkDocument/tree_delete_all_1000 82552824 ns/op 87567040 B/op 75292 allocs/op 78922211 ns/op 87565852 B/op 75286 allocs/op 1.05
BenchmarkDocument/tree_delete_all_1000 - ns/op 82552824 ns/op 78922211 ns/op 1.05
BenchmarkDocument/tree_delete_all_1000 - B/op 87567040 B/op 87565852 B/op 1.00
BenchmarkDocument/tree_delete_all_1000 - allocs/op 75292 allocs/op 75286 allocs/op 1.00
BenchmarkDocument/tree_edit_gc_100 4028423 ns/op 4147926 B/op 15147 allocs/op 3829784 ns/op 4147903 B/op 15147 allocs/op 1.05
BenchmarkDocument/tree_edit_gc_100 - ns/op 4028423 ns/op 3829784 ns/op 1.05
BenchmarkDocument/tree_edit_gc_100 - B/op 4147926 B/op 4147903 B/op 1.00
BenchmarkDocument/tree_edit_gc_100 - allocs/op 15147 allocs/op 15147 allocs/op 1
BenchmarkDocument/tree_edit_gc_1000 334055072 ns/op 384043870 B/op 154944 allocs/op 315673453 ns/op 384043876 B/op 154946 allocs/op 1.06
BenchmarkDocument/tree_edit_gc_1000 - ns/op 334055072 ns/op 315673453 ns/op 1.06
BenchmarkDocument/tree_edit_gc_1000 - B/op 384043870 B/op 384043876 B/op 1.00
BenchmarkDocument/tree_edit_gc_1000 - allocs/op 154944 allocs/op 154946 allocs/op 1.00
BenchmarkDocument/tree_split_gc_100 2700812 ns/op 2407325 B/op 11131 allocs/op 2600316 ns/op 2407268 B/op 11131 allocs/op 1.04
BenchmarkDocument/tree_split_gc_100 - ns/op 2700812 ns/op 2600316 ns/op 1.04
BenchmarkDocument/tree_split_gc_100 - B/op 2407325 B/op 2407268 B/op 1.00
BenchmarkDocument/tree_split_gc_100 - allocs/op 11131 allocs/op 11131 allocs/op 1
BenchmarkDocument/tree_split_gc_1000 203136555 ns/op 222495545 B/op 122064 allocs/op 189853022 ns/op 222497209 B/op 122061 allocs/op 1.07
BenchmarkDocument/tree_split_gc_1000 - ns/op 203136555 ns/op 189853022 ns/op 1.07
BenchmarkDocument/tree_split_gc_1000 - B/op 222495545 B/op 222497209 B/op 1.00
BenchmarkDocument/tree_split_gc_1000 - allocs/op 122064 allocs/op 122061 allocs/op 1.00
BenchmarkRPC/client_to_server 418279671 ns/op 17652325 B/op 223966 allocs/op 421647807 ns/op 16698856 B/op 223868 allocs/op 0.99
BenchmarkRPC/client_to_server - ns/op 418279671 ns/op 421647807 ns/op 0.99
BenchmarkRPC/client_to_server - B/op 17652325 B/op 16698856 B/op 1.06
BenchmarkRPC/client_to_server - allocs/op 223966 allocs/op 223868 allocs/op 1.00
BenchmarkRPC/client_to_client_via_server 777207647 ns/op 37703484 B/op 477312 allocs/op 779289986 ns/op 35481804 B/op 475313 allocs/op 1.00
BenchmarkRPC/client_to_client_via_server - ns/op 777207647 ns/op 779289986 ns/op 1.00
BenchmarkRPC/client_to_client_via_server - B/op 37703484 B/op 35481804 B/op 1.06
BenchmarkRPC/client_to_client_via_server - allocs/op 477312 allocs/op 475313 allocs/op 1.00
BenchmarkRPC/attach_large_document 1255918859 ns/op 1885237864 B/op 12480 allocs/op 1305798130 ns/op 1920985552 B/op 12561 allocs/op 0.96
BenchmarkRPC/attach_large_document - ns/op 1255918859 ns/op 1305798130 ns/op 0.96
BenchmarkRPC/attach_large_document - B/op 1885237864 B/op 1920985552 B/op 0.98
BenchmarkRPC/attach_large_document - allocs/op 12480 allocs/op 12561 allocs/op 0.99
BenchmarkRPC/adminCli_to_server 536117933 ns/op 19955212 B/op 291148 allocs/op 537951330 ns/op 19884004 B/op 291104 allocs/op 1.00
BenchmarkRPC/adminCli_to_server - ns/op 536117933 ns/op 537951330 ns/op 1.00
BenchmarkRPC/adminCli_to_server - B/op 19955212 B/op 19884004 B/op 1.00
BenchmarkRPC/adminCli_to_server - allocs/op 291148 allocs/op 291104 allocs/op 1.00
BenchmarkLocker 81.13 ns/op 32 B/op 1 allocs/op 81.74 ns/op 32 B/op 1 allocs/op 0.99
BenchmarkLocker - ns/op 81.13 ns/op 81.74 ns/op 0.99
BenchmarkLocker - B/op 32 B/op 32 B/op 1
BenchmarkLocker - allocs/op 1 allocs/op 1 allocs/op 1
BenchmarkLockerParallel 45 ns/op 0 B/op 0 allocs/op 45.57 ns/op 0 B/op 0 allocs/op 0.99
BenchmarkLockerParallel - ns/op 45 ns/op 45.57 ns/op 0.99
BenchmarkLockerParallel - B/op 0 B/op 0 B/op 1
BenchmarkLockerParallel - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkLockerMoreKeys 171.6 ns/op 31 B/op 0 allocs/op 178.5 ns/op 31 B/op 0 allocs/op 0.96
BenchmarkLockerMoreKeys - ns/op 171.6 ns/op 178.5 ns/op 0.96
BenchmarkLockerMoreKeys - B/op 31 B/op 31 B/op 1
BenchmarkLockerMoreKeys - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkRWLocker/RWLock_rate_2 50.24 ns/op 0 B/op 0 allocs/op 49.29 ns/op 0 B/op 0 allocs/op 1.02
BenchmarkRWLocker/RWLock_rate_2 - ns/op 50.24 ns/op 49.29 ns/op 1.02
BenchmarkRWLocker/RWLock_rate_2 - B/op 0 B/op 0 B/op 1
BenchmarkRWLocker/RWLock_rate_2 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkRWLocker/RWLock_rate_10 43.23 ns/op 0 B/op 0 allocs/op 43.7 ns/op 0 B/op 0 allocs/op 0.99
BenchmarkRWLocker/RWLock_rate_10 - ns/op 43.23 ns/op 43.7 ns/op 0.99
BenchmarkRWLocker/RWLock_rate_10 - B/op 0 B/op 0 B/op 1
BenchmarkRWLocker/RWLock_rate_10 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkRWLocker/RWLock_rate_100 58.48 ns/op 2 B/op 0 allocs/op 59.89 ns/op 2 B/op 0 allocs/op 0.98
BenchmarkRWLocker/RWLock_rate_100 - ns/op 58.48 ns/op 59.89 ns/op 0.98
BenchmarkRWLocker/RWLock_rate_100 - B/op 2 B/op 2 B/op 1
BenchmarkRWLocker/RWLock_rate_100 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkRWLocker/RWLock_rate_1000 87.98 ns/op 8 B/op 0 allocs/op 90.08 ns/op 9 B/op 0 allocs/op 0.98
BenchmarkRWLocker/RWLock_rate_1000 - ns/op 87.98 ns/op 90.08 ns/op 0.98
BenchmarkRWLocker/RWLock_rate_1000 - B/op 8 B/op 9 B/op 0.89
BenchmarkRWLocker/RWLock_rate_1000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkChange/Push_10_Changes 4472962 ns/op 150595 B/op 1625 allocs/op 4515163 ns/op 150527 B/op 1625 allocs/op 0.99
BenchmarkChange/Push_10_Changes - ns/op 4472962 ns/op 4515163 ns/op 0.99
BenchmarkChange/Push_10_Changes - B/op 150595 B/op 150527 B/op 1.00
BenchmarkChange/Push_10_Changes - allocs/op 1625 allocs/op 1625 allocs/op 1
BenchmarkChange/Push_100_Changes 16107194 ns/op 779382 B/op 8514 allocs/op 16380671 ns/op 777757 B/op 8513 allocs/op 0.98
BenchmarkChange/Push_100_Changes - ns/op 16107194 ns/op 16380671 ns/op 0.98
BenchmarkChange/Push_100_Changes - B/op 779382 B/op 777757 B/op 1.00
BenchmarkChange/Push_100_Changes - allocs/op 8514 allocs/op 8513 allocs/op 1.00
BenchmarkChange/Push_1000_Changes 127868016 ns/op 7199291 B/op 79329 allocs/op 129038636 ns/op 7056698 B/op 79326 allocs/op 0.99
BenchmarkChange/Push_1000_Changes - ns/op 127868016 ns/op 129038636 ns/op 0.99
BenchmarkChange/Push_1000_Changes - B/op 7199291 B/op 7056698 B/op 1.02
BenchmarkChange/Push_1000_Changes - allocs/op 79329 allocs/op 79326 allocs/op 1.00
BenchmarkChange/Pull_10_Changes 3671131 ns/op 124334 B/op 1452 allocs/op 3701038 ns/op 124505 B/op 1453 allocs/op 0.99
BenchmarkChange/Pull_10_Changes - ns/op 3671131 ns/op 3701038 ns/op 0.99
BenchmarkChange/Pull_10_Changes - B/op 124334 B/op 124505 B/op 1.00
BenchmarkChange/Pull_10_Changes - allocs/op 1452 allocs/op 1453 allocs/op 1.00
BenchmarkChange/Pull_100_Changes 5278406 ns/op 354681 B/op 5179 allocs/op 5378071 ns/op 354345 B/op 5178 allocs/op 0.98
BenchmarkChange/Pull_100_Changes - ns/op 5278406 ns/op 5378071 ns/op 0.98
BenchmarkChange/Pull_100_Changes - B/op 354681 B/op 354345 B/op 1.00
BenchmarkChange/Pull_100_Changes - allocs/op 5179 allocs/op 5178 allocs/op 1.00
BenchmarkChange/Pull_1000_Changes 10681070 ns/op 2196242 B/op 44678 allocs/op 10689625 ns/op 2198383 B/op 44679 allocs/op 1.00
BenchmarkChange/Pull_1000_Changes - ns/op 10681070 ns/op 10689625 ns/op 1.00
BenchmarkChange/Pull_1000_Changes - B/op 2196242 B/op 2198383 B/op 1.00
BenchmarkChange/Pull_1000_Changes - allocs/op 44678 allocs/op 44679 allocs/op 1.00
BenchmarkSnapshot/Push_3KB_snapshot 18470027 ns/op 904371 B/op 8514 allocs/op 18894322 ns/op 904676 B/op 8515 allocs/op 0.98
BenchmarkSnapshot/Push_3KB_snapshot - ns/op 18470027 ns/op 18894322 ns/op 0.98
BenchmarkSnapshot/Push_3KB_snapshot - B/op 904371 B/op 904676 B/op 1.00
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op 8514 allocs/op 8515 allocs/op 1.00
BenchmarkSnapshot/Push_30KB_snapshot 132068386 ns/op 8289171 B/op 89581 allocs/op 132183618 ns/op 8373884 B/op 90741 allocs/op 1.00
BenchmarkSnapshot/Push_30KB_snapshot - ns/op 132068386 ns/op 132183618 ns/op 1.00
BenchmarkSnapshot/Push_30KB_snapshot - B/op 8289171 B/op 8373884 B/op 0.99
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op 89581 allocs/op 90741 allocs/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot 7557079 ns/op 1116782 B/op 20062 allocs/op 7642939 ns/op 1115803 B/op 20058 allocs/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op 7557079 ns/op 7642939 ns/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot - B/op 1116782 B/op 1115803 B/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op 20062 allocs/op 20058 allocs/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot 19645424 ns/op 9305390 B/op 193604 allocs/op 19992543 ns/op 9314123 B/op 193607 allocs/op 0.98
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op 19645424 ns/op 19992543 ns/op 0.98
BenchmarkSnapshot/Pull_30KB_snapshot - B/op 9305390 B/op 9314123 B/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op 193604 allocs/op 193607 allocs/op 1.00
BenchmarkSplayTree/stress_test_100000 0.1944 ns/op 0 B/op 0 allocs/op 0.1966 ns/op 0 B/op 0 allocs/op 0.99
BenchmarkSplayTree/stress_test_100000 - ns/op 0.1944 ns/op 0.1966 ns/op 0.99
BenchmarkSplayTree/stress_test_100000 - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/stress_test_100000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSplayTree/stress_test_200000 0.3925 ns/op 0 B/op 0 allocs/op 0.3942 ns/op 0 B/op 0 allocs/op 1.00
BenchmarkSplayTree/stress_test_200000 - ns/op 0.3925 ns/op 0.3942 ns/op 1.00
BenchmarkSplayTree/stress_test_200000 - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/stress_test_200000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSplayTree/stress_test_300000 0.5879 ns/op 0 B/op 0 allocs/op 0.5867 ns/op 0 B/op 0 allocs/op 1.00
BenchmarkSplayTree/stress_test_300000 - ns/op 0.5879 ns/op 0.5867 ns/op 1.00
BenchmarkSplayTree/stress_test_300000 - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/stress_test_300000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSplayTree/random_access_100000 0.01267 ns/op 0 B/op 0 allocs/op 0.01311 ns/op 0 B/op 0 allocs/op 0.97
BenchmarkSplayTree/random_access_100000 - ns/op 0.01267 ns/op 0.01311 ns/op 0.97
BenchmarkSplayTree/random_access_100000 - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/random_access_100000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSplayTree/random_access_200000 0.02946 ns/op 0 B/op 0 allocs/op 0.03044 ns/op 0 B/op 0 allocs/op 0.97
BenchmarkSplayTree/random_access_200000 - ns/op 0.02946 ns/op 0.03044 ns/op 0.97
BenchmarkSplayTree/random_access_200000 - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/random_access_200000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSplayTree/random_access_300000 0.04191 ns/op 0 B/op 0 allocs/op 0.06163 ns/op 0 B/op 0 allocs/op 0.68
BenchmarkSplayTree/random_access_300000 - ns/op 0.04191 ns/op 0.06163 ns/op 0.68
BenchmarkSplayTree/random_access_300000 - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/random_access_300000 - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSplayTree/editing_trace_bench 0.001627 ns/op 0 B/op 0 allocs/op 0.002191 ns/op 0 B/op 0 allocs/op 0.74
BenchmarkSplayTree/editing_trace_bench - ns/op 0.001627 ns/op 0.002191 ns/op 0.74
BenchmarkSplayTree/editing_trace_bench - B/op 0 B/op 0 B/op 1
BenchmarkSplayTree/editing_trace_bench - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkSync/memory_sync_10_test 6925 ns/op 1342 B/op 35 allocs/op 7069 ns/op 1341 B/op 35 allocs/op 0.98
BenchmarkSync/memory_sync_10_test - ns/op 6925 ns/op 7069 ns/op 0.98
BenchmarkSync/memory_sync_10_test - B/op 1342 B/op 1341 B/op 1.00
BenchmarkSync/memory_sync_10_test - allocs/op 35 allocs/op 35 allocs/op 1
BenchmarkSync/memory_sync_100_test 53907 ns/op 9518 B/op 268 allocs/op 55847 ns/op 9508 B/op 268 allocs/op 0.97
BenchmarkSync/memory_sync_100_test - ns/op 53907 ns/op 55847 ns/op 0.97
BenchmarkSync/memory_sync_100_test - B/op 9518 B/op 9508 B/op 1.00
BenchmarkSync/memory_sync_100_test - allocs/op 268 allocs/op 268 allocs/op 1
BenchmarkSync/memory_sync_1000_test 616701 ns/op 75458 B/op 2096 allocs/op 622597 ns/op 75587 B/op 2101 allocs/op 0.99
BenchmarkSync/memory_sync_1000_test - ns/op 616701 ns/op 622597 ns/op 0.99
BenchmarkSync/memory_sync_1000_test - B/op 75458 B/op 75587 B/op 1.00
BenchmarkSync/memory_sync_1000_test - allocs/op 2096 allocs/op 2101 allocs/op 1.00
BenchmarkSync/memory_sync_10000_test 7762463 ns/op 763206 B/op 20537 allocs/op 8383743 ns/op 764280 B/op 20548 allocs/op 0.93
BenchmarkSync/memory_sync_10000_test - ns/op 7762463 ns/op 8383743 ns/op 0.93
BenchmarkSync/memory_sync_10000_test - B/op 763206 B/op 764280 B/op 1.00
BenchmarkSync/memory_sync_10000_test - allocs/op 20537 allocs/op 20548 allocs/op 1.00
BenchmarkTextEditing 5270986191 ns/op 3922655544 B/op 20619690 allocs/op 5207949983 ns/op 3922696984 B/op 20619793 allocs/op 1.01
BenchmarkTextEditing - ns/op 5270986191 ns/op 5207949983 ns/op 1.01
BenchmarkTextEditing - B/op 3922655544 B/op 3922696984 B/op 1.00
BenchmarkTextEditing - allocs/op 20619690 allocs/op 20619793 allocs/op 1.00
BenchmarkTree/10000_vertices_to_protobuf 4159542 ns/op 6363237 B/op 70025 allocs/op 4491630 ns/op 6363227 B/op 70025 allocs/op 0.93
BenchmarkTree/10000_vertices_to_protobuf - ns/op 4159542 ns/op 4491630 ns/op 0.93
BenchmarkTree/10000_vertices_to_protobuf - B/op 6363237 B/op 6363227 B/op 1.00
BenchmarkTree/10000_vertices_to_protobuf - allocs/op 70025 allocs/op 70025 allocs/op 1
BenchmarkTree/10000_vertices_from_protobuf 221890968 ns/op 442304465 B/op 290040 allocs/op 224972874 ns/op 442304382 B/op 290039 allocs/op 0.99
BenchmarkTree/10000_vertices_from_protobuf - ns/op 221890968 ns/op 224972874 ns/op 0.99
BenchmarkTree/10000_vertices_from_protobuf - B/op 442304465 B/op 442304382 B/op 1.00
BenchmarkTree/10000_vertices_from_protobuf - allocs/op 290040 allocs/op 290039 allocs/op 1.00
BenchmarkTree/20000_vertices_to_protobuf 8889170 ns/op 12890891 B/op 140028 allocs/op 9171585 ns/op 12890966 B/op 140028 allocs/op 0.97
BenchmarkTree/20000_vertices_to_protobuf - ns/op 8889170 ns/op 9171585 ns/op 0.97
BenchmarkTree/20000_vertices_to_protobuf - B/op 12890891 B/op 12890966 B/op 1.00
BenchmarkTree/20000_vertices_to_protobuf - allocs/op 140028 allocs/op 140028 allocs/op 1
BenchmarkTree/20000_vertices_from_protobuf 873913097 ns/op 1697483196 B/op 580092 allocs/op 900996865 ns/op 1697474200 B/op 580044 allocs/op 0.97
BenchmarkTree/20000_vertices_from_protobuf - ns/op 873913097 ns/op 900996865 ns/op 0.97
BenchmarkTree/20000_vertices_from_protobuf - B/op 1697483196 B/op 1697474200 B/op 1.00
BenchmarkTree/20000_vertices_from_protobuf - allocs/op 580092 allocs/op 580044 allocs/op 1.00
BenchmarkTree/30000_vertices_to_protobuf 13204740 ns/op 18976111 B/op 210029 allocs/op 14238757 ns/op 18976187 B/op 210029 allocs/op 0.93
BenchmarkTree/30000_vertices_to_protobuf - ns/op 13204740 ns/op 14238757 ns/op 0.93
BenchmarkTree/30000_vertices_to_protobuf - B/op 18976111 B/op 18976187 B/op 1.00
BenchmarkTree/30000_vertices_to_protobuf - allocs/op 210029 allocs/op 210029 allocs/op 1
BenchmarkTree/30000_vertices_from_protobuf 2004815582 ns/op 3751743648 B/op 870139 allocs/op 2019780929 ns/op 3751743584 B/op 870139 allocs/op 0.99
BenchmarkTree/30000_vertices_from_protobuf - ns/op 2004815582 ns/op 2019780929 ns/op 0.99
BenchmarkTree/30000_vertices_from_protobuf - B/op 3751743648 B/op 3751743584 B/op 1.00
BenchmarkTree/30000_vertices_from_protobuf - allocs/op 870139 allocs/op 870139 allocs/op 1

This comment was automatically generated by workflow using github-action-benchmark.

@raararaara raararaara marked this pull request as ready for review February 11, 2025 05:56
coderabbitai[bot]
coderabbitai bot reviewed Feb 11, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
pkg/locker/locker.go (2)

161-172: Waiters count and lock cleanup
The approach of unlocking first and then decrementing the waiters count prevents premature deletion of the lock. This logic is well-structured to avoid races. Just confirm that all error paths handle the waiters count consistently.

197-217: Read-unlock and removal logic
This mirrors the write-unlock approach, including the safe decrement of the waiters count. Watch out for situations where a read lock might still be active if a writer lock is pending. Comprehensive concurrency tests are advisable.

test/bench/locker_bench_test.go (1)

70-95: Benchmark for read-write locking
This benchmark effectively simulates mixed workloads of reads and writes by using a random condition. Consider adding more granular or varied concurrency scenarios (e.g., multiple keys in read mode) to capture additional performance insights.

server/backend/sync/locker.go (2)

77-82: Add read lock interface methods
Exposing RLock in the interface is consistent with the new read-write scheme. However, the code doesn’t use the provided context.Context for cancellation. Consider clarifying the docstring or supporting context cancellation if needed.

114-128: Read-unlock interface and context usage
Similar to RLock, RUnlock doesn’t leverage context cancellation. The docstring suggests a cancelable context, but it’s not used. Consider either removing references to cancellation or implementing a non-blocking approach for read-lock operations.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ccf25f0 and 5fe9f7b.

📒 Files selected for processing (5)
  • pkg/locker/locker.go (4 hunks)
  • pkg/locker/locker_test.go (4 hunks)
  • server/backend/sync/locker.go (2 hunks)
  • server/rpc/yorkie_server.go (2 hunks)
  • test/bench/locker_bench_test.go (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: bench
  • GitHub Check: build
🔇 Additional comments (12)
pkg/locker/locker.go (5)

52-52: Introduce read-write mutex
This switch to sync.RWMutex in lockCtr is a good move to allow concurrent read locks, improving throughput for read-only operations.

73-77: Clarify usage of the write lock
The Lock() function is now explicitly a write lock on the RWMutex. Ensure the rest of the code treats this as an exclusive lock and does not introduce unintended concurrency with readers.

83-86: Consistent naming for write-unlock
The Unlock() method is accurately updated to reflect that it unlocks a write lock. This appears consistent with the Lock() usage.

88-96: New read lock methods
The addition of RLock/RUnlock to lockCtr is appropriate. Make sure boundary cases (like multiple concurrent read locks plus a pending write lock) are tested.

174-195: Establish read lock concurrency
By acquiring the read lock outside the main mutex, the code avoids blocking other name-based lock operations. This is a recommended pattern. Verify that large-scale concurrency usage is tested in the suite.

test/bench/locker_bench_test.go (1)

25-25: Added import for formatting
The import of fmt helps label sub-benchmarks. No concerns.

pkg/locker/locker_test.go (4)

49-50: LGTM! Test assertions updated to match new locking behavior.

The test now correctly validates the waiters count increments for the locking mechanism.

Also applies to: 62-62

173-217: Well-structured test coverage for read lock functionality!

The test comprehensively covers critical scenarios:

  • Initial read lock acquisition
  • Concurrent read lock acquisition
  • Lock cleanup behavior

219-279: Excellent test coverage for read-write lock interactions!

The test thoroughly validates the mutual exclusion between read and write locks, ensuring proper blocking behavior in both scenarios:

  1. Read lock followed by write lock
  2. Write lock followed by read lock

281-317: Robust stress testing of concurrent read-write lock operations!

The test effectively validates the locking mechanism under high concurrency with mixed read and write operations. The cleanup verification ensures no resource leaks.

server/rpc/yorkie_server.go (2)

307-324: Smart optimization of locking mechanism!

The conditional locking based on HasChanges() is an excellent optimization that:

  1. Aligns with the PR objective to isolate read/write routines
  2. Reduces lock contention by only acquiring locks when necessary
  3. Includes proper error handling and cleanup

524-538: Consistent implementation of conditional locking!

The implementation maintains consistency with PushPullChanges, applying the same optimized locking pattern with proper error handling.

@hackerwins hackerwins changed the title Introduce rwMutex to isolate read and write routines during PushPull Extend RWMutex Interface for locker package Feb 11, 2025
hackerwins
hackerwins approved these changes Feb 11, 2025
View reviewed changes
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@hackerwins hackerwins merged commit 45c3259 into main Feb 11, 2025
5 checks passed
@hackerwins hackerwins deleted the rw-lock branch February 11, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@hackerwins hackerwins hackerwins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raararaara @hackerwins