chore: Bump acp_core version #3392

Lodek · 2025-01-19T21:10:07Z

Relevant issue(s)

Description

This PR bumps the version of the acp_core dependency which addresses the performance issues reported regarding registering a large number of objects and creating relationships.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Insertion in Defra was tested using a simple script to time it:

import datetime                 
import subprocess                                              
                                                               
cmd = """~/go/bin/defradb client collection create --name Document '[{{ "id": "{}" }}]' --identity 2668057b5e39d8323907304fdff87
02ee3c2d439128d5b3e028433e92db4b689"""                          
                               
now = datetime.datetime.now()
for i in range(10**6):                                         
    result = subprocess.run(cmd.format(str(i)),                 
    shell=True,         
    capture_output=True,        
    )                          
    if result.returncode != 0:                                 
        raise RuntimeError(f"subprocess failed: {result.stdout + result.stderr}")

end = datetime.datetime.now()
print(f'Added {10**6} documents in {end-now}')%

The script ran for 1h 52m and created 123646 documents.

I also timed a count query:

> time ~/go/bin/defradb client query --identity 2668057b5e39d8323907304fdff8702ee3c2d43912
8d5b3e028433e92db4b689 '{_count(Document: {filter:{}})}'        
------ Request Results ------                                  
{                              
  "data": {                                                    
    "_count": 123646                                           
  }                                                            
}                                                              
~/go/bin/defradb client query --identity  '{_count(Document: {filter:{}})}'  0.05s user 0.02s system 0% cpu 15.502 total

Schema used:

type Document @policy(
    id: "<POLICY_ID_PLACEHOLDER>",
    resource: "document"
){
    id: String

Policy used:

name: Test Policy

actor:
  name: actor

resources:
  document:
    permissions:
      read:
        expr: owner + reader + writer

      write:
        expr: owner + writer

    relations:
      owner:
        types:
          - actor

      reader:
        types:
          - actor

      writer:
        types:
          - actor

The query took roughly ~15 secs to execute.

Running a set of benchmark tests against ACP Core, registering ~1M objs took 56 secs.

goos: linux
goarch: amd64
pkg: github.com/sourcenetwork/acp_core/test/benchmark/engine/object
cpu: 12th Gen Intel(R) Core(TM) i7-1270P
BenchmarkObjectRegistration/obj=256-16                74          13683819 ns/op
BenchmarkObjectRegistration/obj=512-16                48          27570359 ns/op
BenchmarkObjectRegistration/obj=1024-16               24          56960745 ns/op
BenchmarkObjectRegistration/obj=2048-16               12         110284574 ns/op
BenchmarkObjectRegistration/obj=4096-16                5         216729450 ns/op
BenchmarkObjectRegistration/obj=8192-16                3         417582417 ns/op
BenchmarkObjectRegistration/obj=16384-16               2         822705992 ns/op
BenchmarkObjectRegistration/obj=32768-16               1        1546204003 ns/op
BenchmarkObjectRegistration/obj=65536-16               1        3171046221 ns/op
BenchmarkObjectRegistration/obj=131072-16              1        6223276065 ns/op
BenchmarkObjectRegistration/obj=262144-16              1        13429632643 ns/op
BenchmarkObjectRegistration/obj=524288-16              1        26931390766 ns/op
oBenchmarkObjectRegistration/obj=1048576-16                    1        56728450599 ns/op
PASS

Specify the platform(s) on which this was tested:

Arch Linux

codecov · 2025-01-19T22:36:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.27%. Comparing base (05cb0cd) to head (56592c8).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3392      +/-   ##
===========================================
+ Coverage    78.17%   78.27%   +0.10%     
===========================================
  Files          392      392              
  Lines        35905    35905              
===========================================
+ Hits         28067    28104      +37     
+ Misses        6166     6141      -25     
+ Partials      1672     1660      -12

Flag	Coverage Δ
all-tests	`78.27% <ø> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 12 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05cb0cd...56592c8. Read the comment docs.

fredcarle

LGTM

shahzadlone · 2025-01-20T17:16:53Z

@Lodek

I am in middle of benchmarking this change to see how local acp improves.

questions:

Sourcehub dependency wasn't bumped in this PR is that okay?
I thought you mentioned acp policy ids would be unstable with the new version (this change doesn't seem to be failing our previous policy IDs?).

Lodek · 2025-01-20T17:57:02Z

@shahzadlone
Yes, not bumping SourceHub was intentional. I created a branch which introduced these optimizations such that we wouldn't need big refactors for the time being. Refactoring Defra's code to accept the new version of acp_core would be nice for the upcoming cycle, I'll see to it.

shahzadlone · 2025-01-20T18:55:16Z

I have finished the benchmarks! Wowza!!! Awesome work the Check remains the same but the 2 most problematic ones (Register and IsDocRegistered) both improved by quite a bit. Here is the report:
acp_bench_report_v0.0.0-20250119220233-a3579dc36a39.md

TLDR Comparisons of Old vs New:

Register:

BenchmarkACPRegisterOld/scale=256,inMem=true-22       14     81.2 ms/op     62.8 MB/op       1M allocs/op
BenchmarkACPRegisterOld/scale=512,inMem=true-22        4      283 ms/op      229 MB/op       4M allocs/op
BenchmarkACPRegisterOld/scale=1024,inMem=true-22       1      1.07 s/op      871 MB/op      15M allocs/op
BenchmarkACPRegisterOld/scale=2048,inMem=true-22       1      4.00 s/op     3.41 GB/op      60M allocs/op
BenchmarkACPRegisterOld/scale=4096,inMem=true-22       1      16.1 s/op     13.5 GB/op     238M allocs/op
BenchmarkACPRegisterOld/scale=8192,inMem=true-22       1      68.9 s/op     53.9 GB/op     946M allocs/op

BenchmarkACPRegisterNew/scale=256,inMem=true-22       57     21.8 ms/op     11.6 MB/op     213k allocs/op
BenchmarkACPRegisterNew/scale=512,inMem=true-22       25     44.5 ms/op     23.1 MB/op     426k allocs/op
BenchmarkACPRegisterNew/scale=1024,inMem=true-22      13     88.7 ms/op     46.3 MB/op     852k allocs/op
BenchmarkACPRegisterNew/scale=2048,inMem=true-22       6      178 ms/op     92.6 MB/op       2M allocs/op
BenchmarkACPRegisterNew/scale=4096,inMem=true-22       3      364 ms/op      185 MB/op       3M allocs/op
BenchmarkACPRegisterNew/scale=8192,inMem=true-22       2      744 ms/op      371 MB/op       7M allocs/op

CheckDocAccess:

BenchmarkACPCheckDocAccessOld/scale=256,inMem=true-22       10     126 µs/op     34.9 kB/op     772 allocs/op
BenchmarkACPCheckDocAccessOld/scale=512,inMem=true-22       10     160 µs/op     34.9 kB/op     772 allocs/op
BenchmarkACPCheckDocAccessOld/scale=1024,inMem=true-22      10     138 µs/op     34.9 kB/op     772 allocs/op
BenchmarkACPCheckDocAccessOld/scale=2048,inMem=true-22      10     177 µs/op     34.9 kB/op     772 allocs/op
BenchmarkACPCheckDocAccessOld/scale=4096,inMem=true-22      10     171 µs/op     35.0 kB/op     773 allocs/op
BenchmarkACPCheckDocAccessOld/scale=8192,inMem=true-22      10     216 µs/op     36.2 kB/op     774 allocs/op

BenchmarkACPCheckDocAccessNew/scale=256,inMem=true-22       10     101 µs/op     34.7 kB/op     769 allocs/op
BenchmarkACPCheckDocAccessNew/scale=512,inMem=true-22       10     102 µs/op     34.8 kB/op     770 allocs/op
BenchmarkACPCheckDocAccessNew/scale=1024,inMem=true-22      10     101 µs/op     34.9 kB/op     771 allocs/op
BenchmarkACPCheckDocAccessNew/scale=2048,inMem=true-22      10     101 µs/op     34.9 kB/op     771 allocs/op
BenchmarkACPCheckDocAccessNew/scale=4096,inMem=true-22      10     195 µs/op     34.9 kB/op     772 allocs/op
BenchmarkACPCheckDocAccessNew/scale=8192,inMem=true-22      10     102 µs/op     34.9 kB/op     772 allocs/op

IsDocRegistered:

BenchmarkACPIsDocRegisteredOld/scale=256,inMem=true-22       10      249 µs/op      221 kB/op      4k allocs/op
BenchmarkACPIsDocRegisteredOld/scale=512,inMem=true-22       10      484 µs/op      422 kB/op      8k allocs/op
BenchmarkACPIsDocRegisteredOld/scale=1024,inMem=true-22      10      957 µs/op      836 kB/op     15k allocs/op
BenchmarkACPIsDocRegisteredOld/scale=2048,inMem=true-22      10     2.42 ms/op     1.64 MB/op     29k allocs/op
BenchmarkACPIsDocRegisteredOld/scale=4096,inMem=true-22      10     3.98 ms/op     3.32 MB/op     58k allocs/op
BenchmarkACPIsDocRegisteredOld/scale=8192,inMem=true-22      10     9.00 ms/op     6.53 MB/op    115k allocs/op

BenchmarkACPIsDocRegisteredNew/scale=256,inMem=true-22       10     54.6 µs/op     20.9 kB/op     395 allocs/op
BenchmarkACPIsDocRegisteredNew/scale=512,inMem=true-22       10     49.1 µs/op     20.9 kB/op     395 allocs/op
BenchmarkACPIsDocRegisteredNew/scale=1024,inMem=true-22      10     44.3 µs/op     20.9 kB/op     395 allocs/op
BenchmarkACPIsDocRegisteredNew/scale=2048,inMem=true-22      10     50.0 µs/op     20.9 kB/op     395 allocs/op
BenchmarkACPIsDocRegisteredNew/scale=4096,inMem=true-22      10     47.0 µs/op     20.9 kB/op     395 allocs/op
BenchmarkACPIsDocRegisteredNew/scale=8192,inMem=true-22      10     50.1 µs/op     20.9 kB/op     395 allocs/op

shahzadlone · 2025-01-20T19:00:51Z

@shahzadlone Yes, not bumping SourceHub was intentional. I created a branch which introduced these optimizations such that we wouldn't need big refactors for the time being. Refactoring Defra's code to accept the new version of acp_core would be nice for the upcoming cycle, I'll see to it.

Ok cool! I am happy with that. In the meanwhile I will clean up the testing framework to not depend on the policyID. I would really want to bump sourcehub for some other utils functions too (list/dump a target policy command for example).

shahzadlone

LGTM! I would argue however that this is more a perf PR then a chore :P but up to you.

Great stuff!

shahzadlone

@Lodek One more thing please link this PR with an issue

Lodek added 2 commits January 19, 2025 18:08

chore: bump acp_core

b11ada0

chore: downgrade raccondb

f5a3538

Lodek changed the title ~~chore: bump acp_core~~ chore: Bump acp_core version Jan 19, 2025

Lodek marked this pull request as ready for review January 19, 2025 22:58

Lodek requested review from shahzadlone and fredcarle January 20, 2025 16:03

fredcarle assigned Lodek Jan 20, 2025

fredcarle approved these changes Jan 20, 2025

View reviewed changes

Lodek added 2 commits January 20, 2025 15:20

Merge branch 'develop' into chore/bump-acp

7984bd0

Merge branch 'develop' into chore/bump-acp

56592c8

shahzadlone approved these changes Jan 20, 2025

View reviewed changes

shahzadlone reviewed Jan 20, 2025

View reviewed changes

Lodek merged commit e756de1 into develop Jan 20, 2025
44 of 45 checks passed

Lodek deleted the chore/bump-acp branch January 20, 2025 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Bump acp_core version #3392

chore: Bump acp_core version #3392

Lodek commented Jan 19, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

fredcarle left a comment

shahzadlone commented Jan 20, 2025

Lodek commented Jan 20, 2025

shahzadlone commented Jan 20, 2025 •

edited

Loading

shahzadlone commented Jan 20, 2025

shahzadlone left a comment

shahzadlone left a comment

chore: Bump acp_core version #3392

chore: Bump acp_core version #3392

Conversation

Lodek commented Jan 19, 2025 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jan 19, 2025 • edited Loading

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Jan 20, 2025

Lodek commented Jan 20, 2025

shahzadlone commented Jan 20, 2025 • edited Loading

TLDR Comparisons of Old vs New:

shahzadlone commented Jan 20, 2025

shahzadlone left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Lodek commented Jan 19, 2025 •

edited

Loading

codecov bot commented Jan 19, 2025 •

edited

Loading

shahzadlone commented Jan 20, 2025 •

edited

Loading