Skip to content

Upgrade to fast IAVL with multi batches#531

Merged
tac0turtle merged 5 commits into
cosmos:masterfrom
giskook:multi-batches-upgrade
Sep 8, 2022
Merged

Upgrade to fast IAVL with multi batches#531
tac0turtle merged 5 commits into
cosmos:masterfrom
giskook:multi-batches-upgrade

Conversation

@giskook

@giskook giskook commented Aug 10, 2022

Copy link
Copy Markdown
Contributor

When we auto upgrade our huge (289G) data. We met OOM. so splite to multi batches to upgrade the fast storage.

@giskook giskook changed the title upgrade to fast IAVL with multi batches Upgrade to fast IAVL with multi batches Aug 10, 2022
@robert-zaremba robert-zaremba requested a review from p0mvn August 10, 2022 20:35
Comment thread mutable_tree.go Outdated
"github.com/cosmos/iavl/internal/logger"
)

var commitGap uint64 = 10000000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's add a comment here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

had add a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

have you tried different numbers to see results?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

have you tried different numbers to see results?

not yet. maybe I can use a different value to comare with this. do you have any sugguestion? maybe 5,000,000?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yea lets try it

@giskook giskook Sep 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

with 5m, we got lower memory usage and faster speed ;P
image

@tac0turtle tac0turtle Sep 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

amazing, lets run with it, thank you for doing the research

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, had change the commitGap to 5000000, and remove the timing-gc related code.

Comment thread mutable_tree.go
@faddat

faddat commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

very interesting!

@robert-zaremba

Copy link
Copy Markdown
Contributor

@p0mvn you have been in charge for the fast cache - would you have time to review this PR?

@p0mvn p0mvn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

utACK.

Thank you for this change. That makes sense.

As a side note for future work, do you think we can now remove this garbage collection logic:

iavl/mutable_tree.go

Lines 688 to 712 in 807f8c5

go func() {
timer := time.NewTimer(time.Second)
var m runtime.MemStats
for {
// Sample the current memory usage
runtime.ReadMemStats(&m)
if m.Alloc > 4*1024*1024*1024 {
// If we are using more than 4GB of memory, we should trigger garbage collection
// to free up some memory.
runtime.GC()
}
select {
case <-timer.C:
timer.Reset(time.Second)
case <-done:
if !timer.Stop() {
<-timer.C
}
return
}
}
}()

That logic was added to mitigate OOM but I think the solution in this PR might remove the need for the garbage collection.

We suspect that this garbage collection might be increasing the setup time in our unit tests in Osmosis.

ref: osmosis-labs/osmosis#2148 (comment)

@giskook

giskook commented Sep 7, 2022

Copy link
Copy Markdown
Contributor Author

utACK.

Thank you for this change. That makes sense.

As a side note for future work, do you think we can now remove this garbage collection logic:

iavl/mutable_tree.go

Lines 688 to 712 in 807f8c5

go func() {
timer := time.NewTimer(time.Second)
var m runtime.MemStats
for {
// Sample the current memory usage
runtime.ReadMemStats(&m)
if m.Alloc > 4*1024*1024*1024 {
// If we are using more than 4GB of memory, we should trigger garbage collection
// to free up some memory.
runtime.GC()
}
select {
case <-timer.C:
timer.Reset(time.Second)
case <-done:
if !timer.Stop() {
<-timer.C
}
return
}
}
}()

That logic was added to mitigate OOM but I think the solution in this PR might remove the need for the garbage collection.

We suspect that this garbage collection might be increasing the setup time in our unit tests in Osmosis.

ref: osmosis-labs/osmosis#2148 (comment)

@p0mvn
yes, I think we can remove this garbage colection logic.

I do upgrade comparison on our aws machine(16c64g).

data set
with two tree:a tree with 166439284 nodes, the other with 16201149 nodes.

four ways to upgrade

  1. multi batches
  2. multi batches without timing gc
  3. single batch
  4. single batch without timing gc

I got the two conclusion:

  1. with multi batches we got lower memory usage and faster speed.
  2. timing gc has less effective when we upgrade with multi batches.

image

Comment thread mutable_tree.go
@giskook giskook requested a review from a team September 8, 2022 09:10

@tac0turtle tac0turtle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thank you for this pr

@tac0turtle tac0turtle merged commit 32b1cbc into cosmos:master Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants