[Access] Upgrade lru cache #4605

nozim · 2023-08-08T10:04:51Z

Following up with #4565, upgrade all the dependencies on lru.Cache with generic version for enhanced performance.

…s tests

codecov-commenter · 2023-08-08T23:23:04Z

Codecov Report

Patch coverage: 71.60% and project coverage change: +0.54% 🎉

Comparison is base (ac879c7) 54.57% compared to head (04e8222) 55.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4605      +/-   ##
==========================================
+ Coverage   54.57%   55.11%   +0.54%     
==========================================
  Files         917      592     -325     
  Lines       85903    59393   -26510     
==========================================
- Hits        46878    32733   -14145     
+ Misses      35431    24253   -11178     
+ Partials     3594     2407    -1187

Flag	Coverage Δ
unittests	`55.11% <71.60%> (+0.54%)`	⬆️

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

Files Changed	Coverage Δ
engine/access/rpc/backend/backend_network.go	`34.11% <ø> (ø)`
engine/access/rpc/backend/backend_block_headers.go	`24.61% <20.00%> (ø)`
engine/access/rpc/backend/backend_block_details.go	`21.42% <22.22%> (ø)`
engine/access/rpc/backend/backend.go	`66.25% <25.00%> (+0.20%)`	⬆️
engine/access/rpc/backend/backend_scripts.go	`66.52% <25.00%> (+0.84%)`	⬆️
engine/consensus/approvals/approvals_lru_cache.go	`49.18% <50.00%> (-0.82%)`	⬇️
engine/access/rpc/connection/cache.go	`95.55% <100.00%> (ø)`
engine/access/rpc/connection/mock_node.go	`100.00% <100.00%> (ø)`
fvm/storage/derived/derived_chain_data.go	`90.47% <100.00%> (ø)`

... and 335 files with indirect coverage changes

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

peterargue

thanks @nozim! This will clean things up nicely

peterargue · 2023-08-10T18:49:33Z

engine/access/rpc/backend/backend.go

@@ -119,7 +118,7 @@ func New(
 		retry.Activate()
 	}

-	loggedScripts, err := lru.New(DefaultLoggedScriptsCacheSize)
+	loggedScripts, err := lru.New[[16]byte, time.Time](DefaultLoggedScriptsCacheSize)


Suggested change

loggedScripts, err := lru.New[[16]byte, time.Time](DefaultLoggedScriptsCacheSize)

loggedScripts, err := lru.New[[md5.Size]byte, time.Time](DefaultLoggedScriptsCacheSize)

peterargue · 2023-08-10T18:49:45Z

engine/access/rpc/backend/backend_scripts.go

+	connFactory       connection.ConnectionFactory
+	log               zerolog.Logger
+	metrics           module.BackendScriptsMetrics
+	loggedScripts     *lru.Cache[[16]byte, time.Time]


Suggested change

loggedScripts *lru.Cache[[16]byte, time.Time]

loggedScripts *lru.Cache[[md5.Size]byte, time.Time]

peterargue · 2023-08-10T18:52:15Z

engine/access/rpc/connection/connection_test.go

-	"pgregory.net/rapid"
-
-	lru "github.com/hashicorp/golang-lru"
+	lru "github.com/hashicorp/golang-lru/v2"


here and all other places, we try to use the following import group ordering

import ( "std-libs" "external/packages" "onflow/other-repo/packages" "onflow/flow-go/packages" )

the linter will enforce local mode imports

peterargue · 2023-08-10T18:56:41Z

engine/access/rpc/connection/connection_test.go

@@ -129,8 +124,8 @@ func TestProxyAccessAPIConnectionReuse(t *testing.T) {
 	connectionFactory.CollectionGRPCPort = cn.port
 	// set the connection pool cache size
 	cacheSize := 1
-	cache, _ := lru.NewWithEvict(cacheSize, func(_, evictedValue interface{}) {
-		evictedValue.(*CachedClient).Close()
+	cache, _ := lru.NewWithEvict[string, *CachedClient](cacheSize, func(_ string, client *CachedClient) {


nit: how about refactoring a bit to move the duplicated code to a helper?

func getCache(t *testing.T, cacheSize int) *lru.Cache[string, *CachedClient] { cache, err := lru.NewWithEvict[string, *CachedClient](cacheSize, func(_ string, client *CachedClient) { err := client.Close() require.NoError(t, err) }) require.NoError(t, err) return cache }

engine/access/rpc/connection/connection_test.go

jordanschalm · 2023-08-22T20:52:43Z

@nozim The CI is failing because the generated mocks do not exactly match the committed mocks (the imports are not in the same order).

If you run make generate-mocks from the repository root, it should fix the mock files.

jordanschalm · 2023-08-24T22:44:49Z

Hey @nozim, since the last review the diff has grown quite a bit. The additions don't seem related this PR. Was there an inadvertent merge of some other branch? It seems like the extra unrelated changes were added in this commit: c2938e4.

nozim · 2023-08-25T08:58:59Z

@jordanschalm yes, I'll revert those changes after the other one is merged. Thanks for bringing this up 🙏

nozim · 2023-08-28T11:19:35Z

@jordanschalm removed irrelevant changes. Kindly re-review.

nozim · 2023-08-31T11:37:06Z

@jordanschalm kindly help to rerun the ci 🙏

engine/access/rpc/backend/backend.go

nozim · 2023-09-01T11:28:01Z

@peterargue kindly run the ci. Fixed the annotation

nozim · 2023-09-09T19:37:39Z

Recreated this PR on latest master here #4700. So closing this one.

nozim added 10 commits August 3, 2023 12:00

4585 Add more testable backend constructor with unit tests

0f82f8c

4585: Add mock file generation for NodeCommunicator

3d23bcf

4585: Add simple lru caching for transaction results and refactor it'…

f018aca

…s tests

4585: Ignore mock file

8dd08db

4585: Fix goimport issues

b3865c4

Merge branch 'master' into 4585-add-caching-tx-results

ffd8cf7

Merge branch 'master' into 4585-add-caching-tx-results

ecdafd7

Refactor backend constructor, replace old

d3a5145

Upgrade lru cache in backend

8c6fbb4

Merge branch 'master' into upgrade-lru-cache

33410a2

nozim requested a review from peterargue as a code owner August 8, 2023 10:04

nozim changed the title ~~Upgrade lru cache~~ [Access] Upgrade lru cache Aug 8, 2023

nozim added 2 commits August 8, 2023 14:24

Merge branch 'master' into 4585-add-caching-tx-results

cba02b4

Fix goimports

985b558

nozim added 3 commits August 9, 2023 11:57

Goimport fixes

c19b3a9

Merge branch 'master' into upgrade-lru-cache

fe00dfd

Merge branch 'master' into upgrade-lru-cache

9507585

peterargue reviewed Aug 10, 2023

View reviewed changes

nozim added 11 commits August 12, 2023 11:55

Merge branch 'master' into 4585-add-caching-tx-results

eb0257c

Fix failing tests

42897b2

Update backend constructor annotation

9935534

Update mock call

1e60d8d

Remove unused mock setup in backend transactions

0d51ec3

Refine backend transaction tests

da2c6be

Simplify backend transactiont tests

de50262

Rename test case

01d423e

Require mock to be called exactly once

43193d6

Refactor Access backend constructor

77e65b4

Fix backend tests

ed1b3b2

franklywatson and others added 2 commits August 22, 2023 16:22

Merge branch 'master' into 4585-add-caching-tx-results

8e4b0fc

Revert import change in mock folders

689ac6d

nozim requested review from jordanschalm and kc1116 August 23, 2023 13:43

nozim and others added 4 commits August 23, 2023 14:48

Merge branch 'master' into upgrade-lru-cache

c2938e4

Add generated mock files

eb2ba85

Refactor test check for error

8ff9fb8

Merge branch 'master' into upgrade-lru-cache

d03e611

nozim added 6 commits August 25, 2023 11:01

Remove unrelevant changes

e3a49d2

Fix imports in backend scripts

41c81a6

Remove unrelated changes from historical access test

f7805a0

Fix imports

4936f8b

Remove unused mock

9efd131

Fix compilation errors

d6a71cd

franklywatson and others added 2 commits August 30, 2023 14:01

Merge branch 'master' into upgrade-lru-cache

b07879a

Revert mock import changes

eb1808a

Merge branch 'master' into upgrade-lru-cache

e29737b

peterargue approved these changes Sep 1, 2023

View reviewed changes

engine/access/rpc/backend/backend.go Outdated Show resolved Hide resolved

nozim added 2 commits September 1, 2023 12:24

Add nosec annotation to import

9a69798

Merge branch 'master' into upgrade-lru-cache

3ef9bbe

Merge branch 'master' into upgrade-lru-cache

04e8222

nozim mentioned this pull request Sep 9, 2023

[Access] Upgrade lru cache v2 #4700

Merged

nozim closed this Sep 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Access] Upgrade lru cache #4605

[Access] Upgrade lru cache #4605

nozim commented Aug 8, 2023 •

edited

Loading

codecov-commenter commented Aug 8, 2023 •

edited

Loading

peterargue left a comment

peterargue Aug 10, 2023

nozim Aug 22, 2023

peterargue Aug 10, 2023

nozim Aug 22, 2023

peterargue Aug 10, 2023

nozim Aug 22, 2023

peterargue Aug 10, 2023

nozim Aug 22, 2023

jordanschalm commented Aug 22, 2023

jordanschalm commented Aug 24, 2023

nozim commented Aug 25, 2023

nozim commented Aug 28, 2023

nozim commented Aug 31, 2023

nozim commented Sep 1, 2023

nozim commented Sep 9, 2023

	loggedScripts, err := lru.New[[16]byte, time.Time](DefaultLoggedScriptsCacheSize)
	loggedScripts, err := lru.New[[md5.Size]byte, time.Time](DefaultLoggedScriptsCacheSize)

	loggedScripts *lru.Cache[[16]byte, time.Time]
	loggedScripts *lru.Cache[[md5.Size]byte, time.Time]

[Access] Upgrade lru cache #4605

[Access] Upgrade lru cache #4605

Conversation

nozim commented Aug 8, 2023 • edited Loading

codecov-commenter commented Aug 8, 2023 • edited Loading

Codecov Report

peterargue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm commented Aug 22, 2023

jordanschalm commented Aug 24, 2023

nozim commented Aug 25, 2023

nozim commented Aug 28, 2023

nozim commented Aug 31, 2023

nozim commented Sep 1, 2023

nozim commented Sep 9, 2023

nozim commented Aug 8, 2023 •

edited

Loading

codecov-commenter commented Aug 8, 2023 •

edited

Loading