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

feat: support deletion for memory graph #606

Merged
merged 13 commits into from
Oct 16, 2023
Merged

feat: support deletion for memory graph #606

merged 13 commits into from
Oct 16, 2023

Conversation

wangxiaoxuan273
Copy link
Contributor

@wangxiaoxuan273 wangxiaoxuan273 commented Sep 25, 2023 •
edited
Loading

Signed-off-by: Xiaoxuan Wang wangxiaoxuan119@gmail.com

Part 1/4 of #454

Based on draft PR #582

Current behavior regarding graph.Memory.Remove(node):

  • node entry in m.successors and m.nodes is removed.
  • node is removed from its successors predecessors list.
  • node entry in m.predecessors is NOT removed, unless all its predecessors no longer exist.
  • node is NOT removed from its predecessors' m.successors list. The m.successors is always in accordance with the actual content.

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023 •
edited
Loading

Codecov Report

Merging #606 (012930a) into main (e8e4f84) will increase coverage by 0.26%.
The diff coverage is 96.49%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #606      +/-   ##
==========================================
+ Coverage   74.84%   75.11%   +0.26%     
==========================================
  Files          58       59       +1     
  Lines        5328     5420      +92     
==========================================
+ Hits         3988     4071      +83     
- Misses        987      994       +7     
- Partials      353      355       +2
Files Coverage Δ
internal/container/set/set.go 100.00% <100.00%> (ø)
internal/graph/memory.go 90.00% <96.36%> (ø)

Wwwsylvia
Wwwsylvia reviewed Sep 25, 2023
View reviewed changes
internal/graph/benchmark_test.go Outdated Show resolved Hide resolved
internal/graph/deletablememory.go Outdated Show resolved Hide resolved
internal/graph/deletablememory_test.go Outdated Show resolved Hide resolved
@wangxiaoxuan273 wangxiaoxuan273 mentioned this pull request Oct 7, 2023
Merged
@wangxiaoxuan273 wangxiaoxuan273 force-pushed the deletable-memory branch 3 times, most recently from ae365e5 to c952059 Compare October 8, 2023 07:44
Wwwsylvia
Wwwsylvia reviewed Oct 9, 2023
View reviewed changes
internal/graph/memory.go Show resolved Hide resolved
internal/graph/memory_test.go Outdated Show resolved Hide resolved
internal/graph/memory_test.go Show resolved Hide resolved
internal/graph/memory_test.go Outdated Show resolved Hide resolved
@Wwwsylvia
Copy link
Member

Wwwsylvia commented Oct 9, 2023 •
edited
Loading

Can we share the benchmark result here? So that the community understands why we merge Memory and DeletableMemory.

@wangxiaoxuan273
Copy link
Contributor Author

wangxiaoxuan273 commented Oct 9, 2023 •
edited
Loading

Can we share the benchmark result here? So that the community understands why we merge Memory and DeletableMemory.

Here are the benchmark results of the original graph.Memory implementation (titled Memory, which was optimized for concurrent reads and adds, there was no delete functionality), compared to the current one (titled DeletableMemory, which enables concurrent delete):

  • for function Index
Screenshot 2023-09-25 at 18 11 14
  • for function IndexAll
Screenshot 2023-09-25 at 18 13 09
  • for function Predecessors
Screenshot 2023-09-25 at 18 16 26

The performance is similar, so we use the current implementation.

shizhMSFT
shizhMSFT approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

shizhMSFT
shizhMSFT reviewed Oct 10, 2023
View reviewed changes
internal/graph/memory_test.go Outdated Show resolved Hide resolved
internal/graph/memory_test.go Outdated Show resolved Hide resolved
@shizhMSFT
Copy link
Contributor

The benchmark looks good to me. We can merge this PR once you have addressed @Wwwsylvia 's comments and above minor comments related to the graphs in the test files.

@wangxiaoxuan273 wangxiaoxuan273 force-pushed the deletable-memory branch 2 times, most recently from ad92549 to 693a352 Compare October 12, 2023 14:41
Wwwsylvia
Wwwsylvia reviewed Oct 13, 2023
View reviewed changes
Copy link
Member

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Some comments may need rephrasing.

internal/graph/memory.go Outdated Show resolved Hide resolved
@wangxiaoxuan273
feat: support deletion for memory graph
012ab6c 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
added benchmark tests
e1c667f 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
added an ASCII drawing
f98443e 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved comments
6f54047 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
merged deletable memory into memory
08fde14 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
resolved some comments and update the 1st test
b7a10d4 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
cleaned up remove test and updated remove behavior
e32a36c 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
done refining the 1st test
02948ff 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
refined the second test
3f4848d 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
refined doc of Remove
66b1736 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
refined the drawing!
8c0d22a 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
refined the predecessor test
f68a3a3 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@wangxiaoxuan273
rephrased the doc
012930a 
Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
Wwwsylvia
Wwwsylvia approved these changes Oct 16, 2023
View reviewed changes
Copy link
Member

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

LGTM

@Wwwsylvia Wwwsylvia merged commit 459a246 into oras-project:main Oct 16, 2023
7 checks passed
@wangxiaoxuan273 wangxiaoxuan273 deleted the deletable-memory branch October 16, 2023 04:59
@Wwwsylvia Wwwsylvia mentioned this pull request Oct 19, 2023
Closed
shizhMSFT pushed a commit that referenced this pull request Nov 20, 2023
@wangxiaoxuan273
feat: extend oci.Store with Delete() (#614)
75d200e 
Part 4/4 of #454 

This PR should be reviewed and merged after PR #606 #607 #608

---------

Signed-off-by: Xiaoxuan Wang <wangxiaoxuan119@gmail.com>
@Wwwsylvia Wwwsylvia mentioned this pull request Nov 21, 2023
Closed
@Wwwsylvia Wwwsylvia mentioned this pull request Jan 26, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@Wwwsylvia Wwwsylvia Wwwsylvia approved these changes

@sajayantony sajayantony Awaiting requested review from sajayantony sajayantony is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker SteveLasker is a code owner

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

Successfully merging this pull request may close these issues.
4 participants
@wangxiaoxuan273 @codecov-commenter @Wwwsylvia @shizhMSFT