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

Handle concurrent Tree.RemoveStyle #883

Merged
merged 1 commit into from
Jun 3, 2024
Merged

Handle concurrent Tree.RemoveStyle #883

merged 1 commit into from
Jun 3, 2024

Conversation

hackerwins
Copy link
Member

@hackerwins hackerwins commented Jun 3, 2024 •
edited by coderabbitai bot
Loading

What this PR does / why we need it:

Handle concurrent Tree.RemoveStyle

This commit addresses an issue where if a node has an attribute with the same key as the RemoveStyle is simultaneously inserted by Tree.Edit functions, the attribute of the concurrently inserted Node with the same key gets deleted. To resolve this issue, this commit adds filtering logic in RemoveStyle to prevent attribute deletion from concurrently inserted nodes.

Which issue(s) this PR fixes:

Address #884

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation:

Checklist:

  • Added relevant tests or not required
  • Didn't break anything

Summary by CodeRabbit

  • Bug Fixes

    • Improved the order of operations for attribute removal and creation time mapping to enhance styling accuracy.

  • New Features

    • Added support for handling text nodes differently during styling operations.
    • Introduced new parameters for more precise attribute management during styling and removal operations.

  • Tests

    • Updated concurrency test cases to include new attributes and operations for styling.

@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Jun 3, 2024 •
edited
Loading

Walkthrough

Recent updates focus on refining the handling of styling operations within the tree structures in the codebase. The changes primarily enhance the order of operations and the parameters involved in styling and attribute removal processes. These improvements ensure more accurate and efficient management of node attributes, particularly when dealing with concurrent edits and different actors.

Changes

File Change Summary
api/converter/from_pb.go Rearranged operations order in fromTreeStyle and updated operations.NewTreeStyleRemove call to include createdAtMapByActor.
pkg/document/crdt/tree.go Refined logic in canStyle and Style methods; added maxCreatedAtMapByActor parameter to RemoveStyle.
pkg/document/json/tree.go Added maxCreationMapByActor parameter to RemoveStyle method.
pkg/document/operations/tree_style.go Added maxCreatedAtMapByActor parameter to NewTreeStyleRemove and updated Execute method.
test/integration/tree_concurrency_test.go Modified TestTreeConcurrencyEditStyle to include new attributes and updated StyleRemove and StyleSet operations.

Sequence Diagram(s) (Beta)

sequenceDiagram
    participant User
    participant API
    participant CRDT
    participant JSON
    participant Operations

    User->>API: Call fromTreeStyle with TreeStyle
    API->>Operations: Call NewTreeStyleRemove with createdAtMapByActor
    Operations->>CRDT: Execute TreeStyle with maxCreatedAtMapByActor
    User->>JSON: Call RemoveStyle with maxCreationMapByActor
    JSON->>CRDT: RemoveStyle with updated parameters
    CRDT->>Operations: RemoveStyle with maxCreatedAtMapByActor
    Operations->>CRDT: Update node attributes
Loading

Poem

In the forest of code where trees reside,
With styles and attributes, they now abide.
A map of actors, a time to keep,
Ensuring harmony where conflicts seep.
Blue and bold, they dance anew,
In the world of bytes, a vibrant hue.
🐇✨

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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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 as 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 resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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 Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 9.52381% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 50.50%. Comparing base (3647f35) to head (e0837cf).

Files Patch % Lines
pkg/document/crdt/tree.go 11.42% 31 Missing ⚠️
api/converter/from_pb.go 0.00% 7 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
- Coverage   50.57%   50.50%   -0.07%     
==========================================
  Files          70       70              
  Lines       10435    10454      +19     
==========================================
+ Hits         5277     5280       +3     
- Misses       4630     4646      +16     
  Partials      528      528

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

@hackerwins hackerwins force-pushed the concurrent-removestyle branch from 587212d to e0837cf Compare June 3, 2024 08:39
github-actions[bot]
github-actions bot reviewed Jun 3, 2024
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

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: e0837cf Previous: 587212d Ratio
BenchmarkDocument/constructor_test 1480 ns/op 1337 B/op 24 allocs/op 1479 ns/op 1337 B/op 24 allocs/op 1.00
BenchmarkDocument/constructor_test - ns/op 1480 ns/op 1479 ns/op 1.00
BenchmarkDocument/constructor_test - B/op 1337 B/op 1337 B/op 1
BenchmarkDocument/constructor_test - allocs/op 24 allocs/op 24 allocs/op 1
BenchmarkDocument/status_test 951.2 ns/op 1305 B/op 22 allocs/op 951.6 ns/op 1305 B/op 22 allocs/op 1.00
BenchmarkDocument/status_test - ns/op 951.2 ns/op 951.6 ns/op 1.00
BenchmarkDocument/status_test - B/op 1305 B/op 1305 B/op 1
BenchmarkDocument/status_test - allocs/op 22 allocs/op 22 allocs/op 1
BenchmarkDocument/equals_test 8631 ns/op 7273 B/op 132 allocs/op 7600 ns/op 7273 B/op 132 allocs/op 1.14
BenchmarkDocument/equals_test - ns/op 8631 ns/op 7600 ns/op 1.14
BenchmarkDocument/equals_test - B/op 7273 B/op 7273 B/op 1
BenchmarkDocument/equals_test - allocs/op 132 allocs/op 132 allocs/op 1
BenchmarkDocument/nested_update_test 16902 ns/op 12139 B/op 262 allocs/op 16823 ns/op 12139 B/op 262 allocs/op 1.00
BenchmarkDocument/nested_update_test - ns/op 16902 ns/op 16823 ns/op 1.00
BenchmarkDocument/nested_update_test - B/op 12139 B/op 12139 B/op 1
BenchmarkDocument/nested_update_test - allocs/op 262 allocs/op 262 allocs/op 1
BenchmarkDocument/delete_test 22706 ns/op 15363 B/op 341 allocs/op 22659 ns/op 15364 B/op 341 allocs/op 1.00
BenchmarkDocument/delete_test - ns/op 22706 ns/op 22659 ns/op 1.00
BenchmarkDocument/delete_test - B/op 15363 B/op 15364 B/op 1.00
BenchmarkDocument/delete_test - allocs/op 341 allocs/op 341 allocs/op 1
BenchmarkDocument/object_test 8683 ns/op 6817 B/op 120 allocs/op 8582 ns/op 6817 B/op 120 allocs/op 1.01
BenchmarkDocument/object_test - ns/op 8683 ns/op 8582 ns/op 1.01
BenchmarkDocument/object_test - B/op 6817 B/op 6817 B/op 1
BenchmarkDocument/object_test - allocs/op 120 allocs/op 120 allocs/op 1
BenchmarkDocument/array_test 29074 ns/op 11947 B/op 276 allocs/op 32319 ns/op 11947 B/op 276 allocs/op 0.90
BenchmarkDocument/array_test - ns/op 29074 ns/op 32319 ns/op 0.90
BenchmarkDocument/array_test - B/op 11947 B/op 11947 B/op 1
BenchmarkDocument/array_test - allocs/op 276 allocs/op 276 allocs/op 1
BenchmarkDocument/text_test 31326 ns/op 14716 B/op 469 allocs/op 30702 ns/op 14715 B/op 469 allocs/op 1.02
BenchmarkDocument/text_test - ns/op 31326 ns/op 30702 ns/op 1.02
BenchmarkDocument/text_test - B/op 14716 B/op 14715 B/op 1.00
BenchmarkDocument/text_test - allocs/op 469 allocs/op 469 allocs/op 1
BenchmarkDocument/text_composition_test 29623 ns/op 18422 B/op 484 allocs/op 29300 ns/op 18422 B/op 484 allocs/op 1.01
BenchmarkDocument/text_composition_test - ns/op 29623 ns/op 29300 ns/op 1.01
BenchmarkDocument/text_composition_test - B/op 18422 B/op 18422 B/op 1
BenchmarkDocument/text_composition_test - allocs/op 484 allocs/op 484 allocs/op 1
BenchmarkDocument/rich_text_test 81958 ns/op 38476 B/op 1148 allocs/op 80550 ns/op 38477 B/op 1148 allocs/op 1.02
BenchmarkDocument/rich_text_test - ns/op 81958 ns/op 80550 ns/op 1.02
BenchmarkDocument/rich_text_test - B/op 38476 B/op 38477 B/op 1.00
BenchmarkDocument/rich_text_test - allocs/op 1148 allocs/op 1148 allocs/op 1
BenchmarkDocument/counter_test 17351 ns/op 10722 B/op 244 allocs/op 17401 ns/op 10722 B/op 244 allocs/op 1.00
BenchmarkDocument/counter_test - ns/op 17351 ns/op 17401 ns/op 1.00
BenchmarkDocument/counter_test - B/op 10722 B/op 10722 B/op 1
BenchmarkDocument/counter_test - allocs/op 244 allocs/op 244 allocs/op 1
BenchmarkDocument/text_edit_gc_100 1209400 ns/op 870876 B/op 16689 allocs/op 1182707 ns/op 870781 B/op 16689 allocs/op 1.02
BenchmarkDocument/text_edit_gc_100 - ns/op 1209400 ns/op 1182707 ns/op 1.02
BenchmarkDocument/text_edit_gc_100 - B/op 870876 B/op 870781 B/op 1.00
BenchmarkDocument/text_edit_gc_100 - allocs/op 16689 allocs/op 16689 allocs/op 1
BenchmarkDocument/text_edit_gc_1000 49036312 ns/op 50535917 B/op 181605 allocs/op 48973201 ns/op 50535373 B/op 181603 allocs/op 1.00
BenchmarkDocument/text_edit_gc_1000 - ns/op 49036312 ns/op 48973201 ns/op 1.00
BenchmarkDocument/text_edit_gc_1000 - B/op 50535917 B/op 50535373 B/op 1.00
BenchmarkDocument/text_edit_gc_1000 - allocs/op 181605 allocs/op 181603 allocs/op 1.00
BenchmarkDocument/text_split_gc_100 1815462 ns/op 1528676 B/op 15541 allocs/op 1796761 ns/op 1528710 B/op 15541 allocs/op 1.01
BenchmarkDocument/text_split_gc_100 - ns/op 1815462 ns/op 1796761 ns/op 1.01
BenchmarkDocument/text_split_gc_100 - B/op 1528676 B/op 1528710 B/op 1.00
BenchmarkDocument/text_split_gc_100 - allocs/op 15541 allocs/op 15541 allocs/op 1
BenchmarkDocument/text_split_gc_1000 114557393 ns/op 135076079 B/op 182088 allocs/op 114833531 ns/op 135075248 B/op 182086 allocs/op 1.00
BenchmarkDocument/text_split_gc_1000 - ns/op 114557393 ns/op 114833531 ns/op 1.00
BenchmarkDocument/text_split_gc_1000 - B/op 135076079 B/op 135075248 B/op 1.00
BenchmarkDocument/text_split_gc_1000 - allocs/op 182088 allocs/op 182086 allocs/op 1.00
BenchmarkDocument/text_delete_all_10000 16622271 ns/op 10182780 B/op 40672 allocs/op 16911014 ns/op 10184181 B/op 40680 allocs/op 0.98
BenchmarkDocument/text_delete_all_10000 - ns/op 16622271 ns/op 16911014 ns/op 0.98
BenchmarkDocument/text_delete_all_10000 - B/op 10182780 B/op 10184181 B/op 1.00
BenchmarkDocument/text_delete_all_10000 - allocs/op 40672 allocs/op 40680 allocs/op 1.00
BenchmarkDocument/text_delete_all_100000 291024890 ns/op 142702932 B/op 411774 allocs/op 309024126 ns/op 142667568 B/op 411645 allocs/op 0.94
BenchmarkDocument/text_delete_all_100000 - ns/op 291024890 ns/op 309024126 ns/op 0.94
BenchmarkDocument/text_delete_all_100000 - B/op 142702932 B/op 142667568 B/op 1.00
BenchmarkDocument/text_delete_all_100000 - allocs/op 411774 allocs/op 411645 allocs/op 1.00
BenchmarkDocument/text_100 232517 ns/op 120036 B/op 5081 allocs/op 220474 ns/op 120037 B/op 5081 allocs/op 1.05
BenchmarkDocument/text_100 - ns/op 232517 ns/op 220474 ns/op 1.05
BenchmarkDocument/text_100 - B/op 120036 B/op 120037 B/op 1.00
BenchmarkDocument/text_100 - allocs/op 5081 allocs/op 5081 allocs/op 1
BenchmarkDocument/text_1000 2488702 ns/op 1169027 B/op 50085 allocs/op 2380403 ns/op 1169038 B/op 50085 allocs/op 1.05
BenchmarkDocument/text_1000 - ns/op 2488702 ns/op 2380403 ns/op 1.05
BenchmarkDocument/text_1000 - B/op 1169027 B/op 1169038 B/op 1.00
BenchmarkDocument/text_1000 - allocs/op 50085 allocs/op 50085 allocs/op 1
BenchmarkDocument/array_1000 1252261 ns/op 1091552 B/op 11832 allocs/op 1211793 ns/op 1091354 B/op 11831 allocs/op 1.03
BenchmarkDocument/array_1000 - ns/op 1252261 ns/op 1211793 ns/op 1.03
BenchmarkDocument/array_1000 - B/op 1091552 B/op 1091354 B/op 1.00
BenchmarkDocument/array_1000 - allocs/op 11832 allocs/op 11831 allocs/op 1.00
BenchmarkDocument/array_10000 13237543 ns/op 9800589 B/op 120299 allocs/op 13342678 ns/op 9799444 B/op 120294 allocs/op 0.99
BenchmarkDocument/array_10000 - ns/op 13237543 ns/op 13342678 ns/op 0.99
BenchmarkDocument/array_10000 - B/op 9800589 B/op 9799444 B/op 1.00
BenchmarkDocument/array_10000 - allocs/op 120299 allocs/op 120294 allocs/op 1.00
BenchmarkDocument/array_gc_100 152532 ns/op 132703 B/op 1260 allocs/op 146588 ns/op 132708 B/op 1260 allocs/op 1.04
BenchmarkDocument/array_gc_100 - ns/op 152532 ns/op 146588 ns/op 1.04
BenchmarkDocument/array_gc_100 - B/op 132703 B/op 132708 B/op 1.00
BenchmarkDocument/array_gc_100 - allocs/op 1260 allocs/op 1260 allocs/op 1
BenchmarkDocument/array_gc_1000 1447095 ns/op 1159206 B/op 12877 allocs/op 1390301 ns/op 1159182 B/op 12877 allocs/op 1.04
BenchmarkDocument/array_gc_1000 - ns/op 1447095 ns/op 1390301 ns/op 1.04
BenchmarkDocument/array_gc_1000 - B/op 1159206 B/op 1159182 B/op 1.00
BenchmarkDocument/array_gc_1000 - allocs/op 12877 allocs/op 12877 allocs/op 1
BenchmarkDocument/counter_1000 217082 ns/op 193081 B/op 5771 allocs/op 199901 ns/op 193079 B/op 5771 allocs/op 1.09
BenchmarkDocument/counter_1000 - ns/op 217082 ns/op 199901 ns/op 1.09
BenchmarkDocument/counter_1000 - B/op 193081 B/op 193079 B/op 1.00
BenchmarkDocument/counter_1000 - allocs/op 5771 allocs/op 5771 allocs/op 1
BenchmarkDocument/counter_10000 2258134 ns/op 2088018 B/op 59778 allocs/op 2227413 ns/op 2087995 B/op 59778 allocs/op 1.01
BenchmarkDocument/counter_10000 - ns/op 2258134 ns/op 2227413 ns/op 1.01
BenchmarkDocument/counter_10000 - B/op 2088018 B/op 2087995 B/op 1.00
BenchmarkDocument/counter_10000 - allocs/op 59778 allocs/op 59778 allocs/op 1
BenchmarkDocument/object_1000 1411787 ns/op 1428290 B/op 9849 allocs/op 1350930 ns/op 1428096 B/op 9849 allocs/op 1.05
BenchmarkDocument/object_1000 - ns/op 1411787 ns/op 1350930 ns/op 1.05
BenchmarkDocument/object_1000 - B/op 1428290 B/op 1428096 B/op 1.00
BenchmarkDocument/object_1000 - allocs/op 9849 allocs/op 9849 allocs/op 1
BenchmarkDocument/object_10000 15394653 ns/op 12167788 B/op 100567 allocs/op 15156743 ns/op 12166269 B/op 100563 allocs/op 1.02
BenchmarkDocument/object_10000 - ns/op 15394653 ns/op 15156743 ns/op 1.02
BenchmarkDocument/object_10000 - B/op 12167788 B/op 12166269 B/op 1.00
BenchmarkDocument/object_10000 - allocs/op 100567 allocs/op 100563 allocs/op 1.00
BenchmarkDocument/tree_100 1084657 ns/op 943703 B/op 6101 allocs/op 1034268 ns/op 943700 B/op 6101 allocs/op 1.05
BenchmarkDocument/tree_100 - ns/op 1084657 ns/op 1034268 ns/op 1.05
BenchmarkDocument/tree_100 - B/op 943703 B/op 943700 B/op 1.00
BenchmarkDocument/tree_100 - allocs/op 6101 allocs/op 6101 allocs/op 1
BenchmarkDocument/tree_1000 78039806 ns/op 86460424 B/op 60114 allocs/op 72576681 ns/op 86460263 B/op 60114 allocs/op 1.08
BenchmarkDocument/tree_1000 - ns/op 78039806 ns/op 72576681 ns/op 1.08
BenchmarkDocument/tree_1000 - B/op 86460424 B/op 86460263 B/op 1.00
BenchmarkDocument/tree_1000 - allocs/op 60114 allocs/op 60114 allocs/op 1
BenchmarkDocument/tree_10000 9877095053 ns/op 8580674976 B/op 600256 allocs/op 9568447884 ns/op 8580660944 B/op 600224 allocs/op 1.03
BenchmarkDocument/tree_10000 - ns/op 9877095053 ns/op 9568447884 ns/op 1.03
BenchmarkDocument/tree_10000 - B/op 8580674976 B/op 8580660944 B/op 1.00
BenchmarkDocument/tree_10000 - allocs/op 600256 allocs/op 600224 allocs/op 1.00
BenchmarkDocument/tree_delete_all_1000 78936219 ns/op 87532037 B/op 75263 allocs/op 76488146 ns/op 87508686 B/op 75264 allocs/op 1.03
BenchmarkDocument/tree_delete_all_1000 - ns/op 78936219 ns/op 76488146 ns/op 1.03
BenchmarkDocument/tree_delete_all_1000 - B/op 87532037 B/op 87508686 B/op 1.00
BenchmarkDocument/tree_delete_all_1000 - allocs/op 75263 allocs/op 75264 allocs/op 1.00
BenchmarkDocument/tree_edit_gc_100 3888345 ns/op 4146695 B/op 15140 allocs/op 3776647 ns/op 4146685 B/op 15141 allocs/op 1.03
BenchmarkDocument/tree_edit_gc_100 - ns/op 3888345 ns/op 3776647 ns/op 1.03
BenchmarkDocument/tree_edit_gc_100 - B/op 4146695 B/op 4146685 B/op 1.00
BenchmarkDocument/tree_edit_gc_100 - allocs/op 15140 allocs/op 15141 allocs/op 1.00
BenchmarkDocument/tree_edit_gc_1000 310554045 ns/op 383747218 B/op 154856 allocs/op 300357573 ns/op 383746162 B/op 154848 allocs/op 1.03
BenchmarkDocument/tree_edit_gc_1000 - ns/op 310554045 ns/op 300357573 ns/op 1.03
BenchmarkDocument/tree_edit_gc_1000 - B/op 383747218 B/op 383746162 B/op 1.00
BenchmarkDocument/tree_edit_gc_1000 - allocs/op 154856 allocs/op 154848 allocs/op 1.00
BenchmarkDocument/tree_split_gc_100 2627968 ns/op 2412473 B/op 11125 allocs/op 2525616 ns/op 2412500 B/op 11125 allocs/op 1.04
BenchmarkDocument/tree_split_gc_100 - ns/op 2627968 ns/op 2525616 ns/op 1.04
BenchmarkDocument/tree_split_gc_100 - B/op 2412473 B/op 2412500 B/op 1.00
BenchmarkDocument/tree_split_gc_100 - allocs/op 11125 allocs/op 11125 allocs/op 1
BenchmarkDocument/tree_split_gc_1000 192103401 ns/op 222250489 B/op 121992 allocs/op 179799183 ns/op 222251612 B/op 121990 allocs/op 1.07
BenchmarkDocument/tree_split_gc_1000 - ns/op 192103401 ns/op 179799183 ns/op 1.07
BenchmarkDocument/tree_split_gc_1000 - B/op 222250489 B/op 222251612 B/op 1.00
BenchmarkDocument/tree_split_gc_1000 - allocs/op 121992 allocs/op 121990 allocs/op 1.00
BenchmarkRPC/client_to_server 384834898 ns/op 16826984 B/op 174470 allocs/op 377729051 ns/op 16804344 B/op 173440 allocs/op 1.02
BenchmarkRPC/client_to_server - ns/op 384834898 ns/op 377729051 ns/op 1.02
BenchmarkRPC/client_to_server - B/op 16826984 B/op 16804344 B/op 1.00
BenchmarkRPC/client_to_server - allocs/op 174470 allocs/op 173440 allocs/op 1.01
BenchmarkRPC/client_to_client_via_server 636499367 ns/op 31605420 B/op 319764 allocs/op 644572312 ns/op 32767560 B/op 317988 allocs/op 0.99
BenchmarkRPC/client_to_client_via_server - ns/op 636499367 ns/op 644572312 ns/op 0.99
BenchmarkRPC/client_to_client_via_server - B/op 31605420 B/op 32767560 B/op 0.96
BenchmarkRPC/client_to_client_via_server - allocs/op 319764 allocs/op 317988 allocs/op 1.01
BenchmarkRPC/attach_large_document 1493352401 ns/op 1885789112 B/op 8822 allocs/op 1534700041 ns/op 1919090976 B/op 8823 allocs/op 0.97
BenchmarkRPC/attach_large_document - ns/op 1493352401 ns/op 1534700041 ns/op 0.97
BenchmarkRPC/attach_large_document - B/op 1885789112 B/op 1919090976 B/op 0.98
BenchmarkRPC/attach_large_document - allocs/op 8822 allocs/op 8823 allocs/op 1.00
BenchmarkRPC/adminCli_to_server 548678221 ns/op 35950144 B/op 289507 allocs/op 546874934 ns/op 35939972 B/op 288516 allocs/op 1.00
BenchmarkRPC/adminCli_to_server - ns/op 548678221 ns/op 546874934 ns/op 1.00
BenchmarkRPC/adminCli_to_server - B/op 35950144 B/op 35939972 B/op 1.00
BenchmarkRPC/adminCli_to_server - allocs/op 289507 allocs/op 288516 allocs/op 1.00
BenchmarkLocker 63.28 ns/op 16 B/op 1 allocs/op 62.73 ns/op 16 B/op 1 allocs/op 1.01
BenchmarkLocker - ns/op 63.28 ns/op 62.73 ns/op 1.01
BenchmarkLocker - B/op 16 B/op 16 B/op 1
BenchmarkLocker - allocs/op 1 allocs/op 1 allocs/op 1
BenchmarkLockerParallel 39.07 ns/op 0 B/op 0 allocs/op 39.29 ns/op 0 B/op 0 allocs/op 0.99
BenchmarkLockerParallel - ns/op 39.07 ns/op 39.29 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 146.1 ns/op 15 B/op 0 allocs/op 138 ns/op 15 B/op 0 allocs/op 1.06
BenchmarkLockerMoreKeys - ns/op 146.1 ns/op 138 ns/op 1.06
BenchmarkLockerMoreKeys - B/op 15 B/op 15 B/op 1
BenchmarkLockerMoreKeys - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkChange/Push_10_Changes 3902377 ns/op 121360 B/op 1284 allocs/op 3938749 ns/op 121469 B/op 1284 allocs/op 0.99
BenchmarkChange/Push_10_Changes - ns/op 3902377 ns/op 3938749 ns/op 0.99
BenchmarkChange/Push_10_Changes - B/op 121360 B/op 121469 B/op 1.00
BenchmarkChange/Push_10_Changes - allocs/op 1284 allocs/op 1284 allocs/op 1
BenchmarkChange/Push_100_Changes 14660653 ns/op 574680 B/op 6654 allocs/op 14594995 ns/op 574638 B/op 6655 allocs/op 1.00
BenchmarkChange/Push_100_Changes - ns/op 14660653 ns/op 14594995 ns/op 1.00
BenchmarkChange/Push_100_Changes - B/op 574680 B/op 574638 B/op 1.00
BenchmarkChange/Push_100_Changes - allocs/op 6654 allocs/op 6655 allocs/op 1.00
BenchmarkChange/Push_1000_Changes 117977563 ns/op 5288839 B/op 63150 allocs/op 116735362 ns/op 5211821 B/op 63150 allocs/op 1.01
BenchmarkChange/Push_1000_Changes - ns/op 117977563 ns/op 116735362 ns/op 1.01
BenchmarkChange/Push_1000_Changes - B/op 5288839 B/op 5211821 B/op 1.01
BenchmarkChange/Push_1000_Changes - allocs/op 63150 allocs/op 63150 allocs/op 1
BenchmarkChange/Pull_10_Changes 2975570 ns/op 100785 B/op 1004 allocs/op 2901282 ns/op 101042 B/op 1005 allocs/op 1.03
BenchmarkChange/Pull_10_Changes - ns/op 2975570 ns/op 2901282 ns/op 1.03
BenchmarkChange/Pull_10_Changes - B/op 100785 B/op 101042 B/op 1.00
BenchmarkChange/Pull_10_Changes - allocs/op 1004 allocs/op 1005 allocs/op 1.00
BenchmarkChange/Pull_100_Changes 4463833 ns/op 265686 B/op 3475 allocs/op 4375832 ns/op 266689 B/op 3475 allocs/op 1.02
BenchmarkChange/Pull_100_Changes - ns/op 4463833 ns/op 4375832 ns/op 1.02
BenchmarkChange/Pull_100_Changes - B/op 265686 B/op 266689 B/op 1.00
BenchmarkChange/Pull_100_Changes - allocs/op 3475 allocs/op 3475 allocs/op 1
BenchmarkChange/Pull_1000_Changes 9111133 ns/op 1492678 B/op 29849 allocs/op 8648631 ns/op 1490747 B/op 29858 allocs/op 1.05
BenchmarkChange/Pull_1000_Changes - ns/op 9111133 ns/op 8648631 ns/op 1.05
BenchmarkChange/Pull_1000_Changes - B/op 1492678 B/op 1490747 B/op 1.00
BenchmarkChange/Pull_1000_Changes - allocs/op 29849 allocs/op 29858 allocs/op 1.00
BenchmarkSnapshot/Push_3KB_snapshot 17785550 ns/op 707581 B/op 6660 allocs/op 16851213 ns/op 716145 B/op 6655 allocs/op 1.06
BenchmarkSnapshot/Push_3KB_snapshot - ns/op 17785550 ns/op 16851213 ns/op 1.06
BenchmarkSnapshot/Push_3KB_snapshot - B/op 707581 B/op 716145 B/op 0.99
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op 6660 allocs/op 6655 allocs/op 1.00
BenchmarkSnapshot/Push_30KB_snapshot 122751681 ns/op 5517004 B/op 63154 allocs/op 120487528 ns/op 5614075 B/op 63188 allocs/op 1.02
BenchmarkSnapshot/Push_30KB_snapshot - ns/op 122751681 ns/op 120487528 ns/op 1.02
BenchmarkSnapshot/Push_30KB_snapshot - B/op 5517004 B/op 5614075 B/op 0.98
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op 63154 allocs/op 63188 allocs/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot 6754831 ns/op 921023 B/op 15510 allocs/op 6491251 ns/op 923401 B/op 15512 allocs/op 1.04
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op 6754831 ns/op 6491251 ns/op 1.04
BenchmarkSnapshot/Pull_3KB_snapshot - B/op 921023 B/op 923401 B/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op 15510 allocs/op 15512 allocs/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot 15768098 ns/op 7156350 B/op 150103 allocs/op 15251562 ns/op 7153214 B/op 150105 allocs/op 1.03
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op 15768098 ns/op 15251562 ns/op 1.03
BenchmarkSnapshot/Pull_30KB_snapshot - B/op 7156350 B/op 7153214 B/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op 150103 allocs/op 150105 allocs/op 1.00
BenchmarkSync/memory_sync_10_test 6884 ns/op 1286 B/op 38 allocs/op 6831 ns/op 1286 B/op 38 allocs/op 1.01
BenchmarkSync/memory_sync_10_test - ns/op 6884 ns/op 6831 ns/op 1.01
BenchmarkSync/memory_sync_10_test - B/op 1286 B/op 1286 B/op 1
BenchmarkSync/memory_sync_10_test - allocs/op 38 allocs/op 38 allocs/op 1
BenchmarkSync/memory_sync_100_test 51834 ns/op 8641 B/op 273 allocs/op 50851 ns/op 8641 B/op 273 allocs/op 1.02
BenchmarkSync/memory_sync_100_test - ns/op 51834 ns/op 50851 ns/op 1.02
BenchmarkSync/memory_sync_100_test - B/op 8641 B/op 8641 B/op 1
BenchmarkSync/memory_sync_100_test - allocs/op 273 allocs/op 273 allocs/op 1
BenchmarkSync/memory_sync_1000_test 588506 ns/op 74390 B/op 2122 allocs/op 580705 ns/op 74217 B/op 2114 allocs/op 1.01
BenchmarkSync/memory_sync_1000_test - ns/op 588506 ns/op 580705 ns/op 1.01
BenchmarkSync/memory_sync_1000_test - B/op 74390 B/op 74217 B/op 1.00
BenchmarkSync/memory_sync_1000_test - allocs/op 2122 allocs/op 2114 allocs/op 1.00
BenchmarkSync/memory_sync_10000_test 8642806 ns/op 733711 B/op 20277 allocs/op 7077489 ns/op 738034 B/op 20370 allocs/op 1.22
BenchmarkSync/memory_sync_10000_test - ns/op 8642806 ns/op 7077489 ns/op 1.22
BenchmarkSync/memory_sync_10000_test - B/op 733711 B/op 738034 B/op 0.99
BenchmarkSync/memory_sync_10000_test - allocs/op 20277 allocs/op 20370 allocs/op 1.00
BenchmarkTextEditing 5449966022 ns/op 3901944080 B/op 18743550 allocs/op 5344758022 ns/op 3901934888 B/op 18743492 allocs/op 1.02
BenchmarkTextEditing - ns/op 5449966022 ns/op 5344758022 ns/op 1.02
BenchmarkTextEditing - B/op 3901944080 B/op 3901934888 B/op 1.00
BenchmarkTextEditing - allocs/op 18743550 allocs/op 18743492 allocs/op 1.00

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

@hackerwins hackerwins marked this pull request as ready for review June 3, 2024 08:56
@hackerwins hackerwins merged commit f3dbee7 into main Jun 3, 2024
4 checks passed
@hackerwins hackerwins deleted the concurrent-removestyle branch June 3, 2024 09:00
coderabbitai[bot]
coderabbitai bot reviewed Jun 3, 2024
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: 2

Outside diff range and nitpick comments (1)
pkg/document/crdt/tree.go (1)

397-400: The change to prevent styling on text nodes is appropriate and aligns with the PR's objectives.

Consider adding a comment explaining why text nodes should not be styled, for future maintainability.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3647f35 and e0837cf.

Files selected for processing (5)
  • api/converter/from_pb.go (1 hunks)
  • pkg/document/crdt/tree.go (3 hunks)
  • pkg/document/json/tree.go (2 hunks)
  • pkg/document/operations/tree_style.go (2 hunks)
  • test/integration/tree_concurrency_test.go (2 hunks)
Additional comments not posted (6)
pkg/document/operations/tree_style.go (1)

74-85: The addition of maxCreatedAtMapByActor in NewTreeStyleRemove is consistent with the PR's objective to handle concurrent modifications more effectively by considering the creation times of nodes. This change should help prevent the deletion of attributes from concurrently inserted nodes, which aligns with the problem statement described in the PR.

pkg/document/json/tree.go (1)

Line range hint 265-278: The implementation of maxCreationMapByActor in RemoveStyle is a critical update that aligns with the changes made in the CRDT tree handling. This parameter helps manage node creation times more accurately, which is essential for the correct application of styles in a concurrent environment. It's important to ensure that this new parameter is well-integrated and tested across different scenarios to maintain the robustness of the tree styling functionality.

test/integration/tree_concurrency_test.go (2)

493-496: The addition of a new attribute "color": "blue" in the TestTreeConcurrencyEditStyle function is a significant change that tests the new styling logic under concurrent conditions. This change is well-aligned with the PR's objectives to enhance the handling of tree styles. It's crucial to ensure that the tests cover all edge cases where concurrent modifications could affect the styling attributes.

525-525: The update to the StyleRemove operation for the attribute "color" from "remove-bold" to "remove-color" is a necessary change to test the new logic introduced in the PR. This ensures that the tests are aligned with the actual functionality changes and helps verify the robustness of the new styling logic under concurrent operations.

api/converter/from_pb.go (1)

514-526: The modifications in fromTreeStyle function align well with the PR's objectives to handle concurrent modifications effectively. The use of createdAtMapByActor ensures that the creation times are considered, which is crucial for the correct application of styles in concurrent environments.

pkg/document/crdt/tree.go (1)

1018-1067: The addition of maxCreatedAtMapByActor to manage node creation times in RemoveStyle is a significant improvement for handling concurrency.

Ensure that unit tests cover scenarios where concurrent modifications occur to validate the new logic.

Verification successful

The search results indicate that there are indeed tests related to concurrent modifications in the codebase, specifically in test/integration/tree_concurrency_test.go and test/integration/tree_test.go. These tests include scenarios that involve concurrent operations, which likely cover the RemoveStyle method.

  • test/integration/tree_concurrency_test.go includes tests like concurrently-style-style-test and concurrently-edit-style-test.
  • test/integration/tree_test.go includes multiple tests with "concurrently" in their names, indicating coverage of concurrent scenarios.

Given this information, it appears that the unit tests do cover concurrent modifications for the RemoveStyle method.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the new unit tests cover concurrent modifications.

# Test: Search for unit tests related to `RemoveStyle`. Expect: Tests should include scenarios with concurrent modifications.
rg --type go $'Test.*RemoveStyle.*concurrent'

Length of output: 46

Script:

#!/bin/bash
# Description: Verify if the unit tests for `RemoveStyle` cover concurrent modifications.

# Step 1: Search for all unit tests related to `RemoveStyle`.
rg --type go 'Test.*RemoveStyle'

# Step 2: Inspect the found tests for scenarios with concurrent modifications.
# This will be done by searching for keywords like 'concurrent', 'parallel', or 'race' within the test files.
for file in $(rg --files-with-matches --type go 'Test.*RemoveStyle'); do
  echo "Inspecting $file for concurrent modification scenarios..."
  rg --type go -e 'concurrent' -e 'parallel' -e 'race' $file
done

Length of output: 91

Script:

#!/bin/bash
# Description: Verify if the unit tests for `RemoveStyle` cover concurrent modifications.

# Step 1: Search for all unit tests related to `RemoveStyle`.
rg --type go 'RemoveStyle'

# Step 2: Inspect the found tests for scenarios with concurrent modifications.
# This will be done by searching for keywords like 'concurrent', 'parallel', or 'race' within the test files.
for file in $(rg --files-with-matches --type go 'RemoveStyle'); do
  echo "Inspecting $file for concurrent modification scenarios..."
  rg --type go -e 'concurrent' -e 'parallel' -e 'race' $file
done

Length of output: 8402

pkg/document/operations/tree_style.go
@@ -100,7 +102,7 @@ func (e *TreeStyle) Execute(root *crdt.Root) error {
return err
}
} else {
pairs, err = obj.RemoveStyle(e.from, e.to, e.attributesToRemove, e.executedAt)
_, pairs, err = obj.RemoveStyle(e.from, e.to, e.attributesToRemove, e.executedAt, e.maxCreatedAtMapByActor)
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The search results indicate that there are existing tests for the RemoveStyle method in test/integration/tree_concurrency_test.go and test/integration/tree_test.go. However, there are no tests specifically covering the maxCreatedAtMapByActor parameter.

  • test/integration/tree_concurrency_test.go
  • test/integration/tree_test.go

Please add unit tests that specifically target the new maxCreatedAtMapByActor parameter to ensure its correctness under various concurrent conditions.

Analysis chain

Ensure that the RemoveStyle method correctly handles the new maxCreatedAtMapByActor parameter. This change is crucial for maintaining the integrity of node attributes in concurrent editing scenarios. It would be beneficial to add more detailed unit tests specifically targeting this new logic to ensure its correctness under various concurrent conditions.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new logic in `RemoveStyle` is covered by unit tests.

# Test: Search for unit tests covering the new logic in `RemoveStyle`.
rg --type go $'func Test.*RemoveStyle' test/

Length of output: 45

Script:

#!/bin/bash
# Locate the definition of the `RemoveStyle` method to understand its context.
ast-grep --lang go --pattern 'func ($_).RemoveStyle($_, $_, $_, $_, $_) { $$$ }'

# Search for any tests related to `RemoveStyle`.
rg --type go 'RemoveStyle' test/

# Specifically look for tests involving the `maxCreatedAtMapByActor` parameter.
rg --type go 'maxCreatedAtMapByActor' test/

Length of output: 608

pkg/document/crdt/tree.go
@@ -982,7 +986,7 @@ func (t *Tree) Style(
}
}

if node.canStyle(editedAt, maxCreatedAt) && !node.IsText() && len(attrs) > 0 {
if node.canStyle(editedAt, maxCreatedAt) && len(attrs) > 0 {
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The canStyle method in tree.go explicitly prevents text nodes from being styled by returning false if n.IsText() is true. This contradicts the change in the Style method, which now allows text nodes to be styled.

  • pkg/document/crdt/tree.go:397-399: The canStyle method returns false for text nodes.
  • pkg/document/crdt/tree.go:989: The Style method now allows text nodes to be styled if canStyle returns true.

This inconsistency needs to be addressed to ensure the logic is coherent.

Analysis chain

Verify the consistency of allowing text nodes to be styled here, given the update in canStyle method.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if there are any other methods that contradict the logic of not styling text nodes.

# Test: Search for methods related to styling text nodes. Expect: No other methods should allow styling text nodes.
rg --type go $'IsText() && .*Style'

Length of output: 36

Script:

#!/bin/bash
# Search for the implementation of the `canStyle` method in the codebase.
ast-grep --lang go --pattern 'func ($_ $_) canStyle($_, $_) bool { $$$ }'

# Search for all usages of the `canStyle` method in the codebase.
rg --type go 'canStyle'

Length of output: 1524

@coderabbitai coderabbitai bot mentioned this pull request Dec 5, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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

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

Successfully merging this pull request may close these issues.
1 participant
@hackerwins