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.

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

feat: optimize ledgerv1 #453

Merged
merged 8 commits into from
Aug 18, 2023
Merged

feat: optimize ledgerv1 #453

merged 8 commits into from
Aug 18, 2023

Conversation

gfyrag
Copy link
Contributor

@gfyrag gfyrag commented Aug 14, 2023

No description provided.

@gfyrag gfyrag requested a review from flemzord as a code owner August 14, 2023 14:39
@gfyrag gfyrag changed the base branch from main to release/v1.10 August 14, 2023 14:39
@gfyrag gfyrag force-pushed the feat/some-opti branch 2 times, most recently from 2a94901 to adb0cf4 Compare August 14, 2023 15:01
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #453 (2c4ccbf) into release/v1.10 (d55966b) will decrease coverage by 0.01%.
The diff coverage is 73.98%.

@@                Coverage Diff                @@
##           release/v1.10     #453      +/-   ##
=================================================
- Coverage          64.06%   64.05%   -0.01%     
=================================================
  Files                152      152              
  Lines              14032    14142     +110     
=================================================
+ Hits                8989     9059      +70     
- Misses              4460     4490      +30     
- Partials             583      593      +10     
Files Changed Coverage Δ
pkg/core/account.go 71.42% <50.00%> (-28.58%) ⬇️
pkg/storage/transactional.go 65.62% <61.53%> (-1.69%) ⬇️
pkg/storage/sqlstorage/store_ledger.go 73.07% <61.90%> (-7.57%) ⬇️
pkg/storage/sqlstorage/commit.go 60.00% <63.63%> (-1.30%) ⬇️
pkg/storage/sqlstorage/logs.go 72.02% <64.28%> (-3.13%) ⬇️
pkg/storage/sqlstorage/aggregations.go 69.78% <71.15%> (+1.60%) ⬆️
pkg/storage/sqlstorage/transactions.go 78.99% <78.84%> (-0.45%) ⬇️
pkg/storage/sqlstorage/driver.go 70.93% <85.00%> (+1.05%) ⬆️
pkg/ledger/ledger.go 72.80% <100.00%> (+2.63%) ⬆️
pkg/storage/sqlstorage/accounts.go 77.30% <100.00%> (+1.38%) ⬆️
... and 1 more

... and 3 files with indirect coverage changes

if err := l.store.Close(ctx); err != nil {
return errors.Wrap(err, "closing store")
}
//if err := l.store.Close(ctx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep the store close ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, i've forgot to close! thks

return &Store{
executorProvider: executorProvider,
schema: schema,
onClose: onClose,
onDelete: onDelete,
bloom: bloom.NewWithEstimates(1000000, 0.01), // TODO: Configure
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to make that configurable ?


entry, ok := s.cache.Get(address)
if ok {
account := entry.(*core.AccountWithVolumes)
Copy link
Contributor

@paul-nicolas paul-nicolas Aug 16, 2023

Choose a reason for hiding this comment

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

Is a lock needed here when modifying the cache pointer ?


entry, ok := s.cache.Get(address)
if ok {
account := entry.(*core.AccountWithVolumes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the cache after inserting in the database ?

@gfyrag gfyrag merged commit aa3a91f into release/v1.10 Aug 18, 2023
18 of 19 checks passed
@gfyrag gfyrag deleted the feat/some-opti branch August 18, 2023 08:17
flemzord pushed a commit that referenced this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants