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

Tortoise increasing memory #1675

Merged
merged 27 commits into from
Jan 16, 2020
Merged

Tortoise increasing memory #1675

merged 27 commits into from
Jan 16, 2020

Conversation

almogdepaz
Copy link
Contributor

Motivation

fix tortoise increasing memory issue

@@ -156,6 +158,7 @@ func defaultBaseConfig() BaseConfig {
PoETServer: "127.0.0.1",
Hdist: 5,
GenesisActiveSet: 5,
BlockCacheSize: 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is too small; used to be 100 (maybe also not justified)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was the one to set the 100, i dont think it needs to be bigger then the window in regards to the tortoise, beacon is not important, how far back does the atx's forBlockInView go ?

Copy link
Contributor

Choose a reason for hiding this comment

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

full epoch, but if we don't need cache for that it doesn't matter.

return err
}

if !valid {
ni.With().Warning("block is contextually invalid", log.BlockId(bid.String()))
ni.With().Warning("block is contextually invalid", log.BlockId(b.String()))
Copy link
Contributor

Choose a reason for hiding this comment

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

what does b.String() outputs? if we only need the block id, would b.Id().String works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

basically "its ok" but ill do it more explicit

@@ -150,6 +163,7 @@ func (ni *NinjaTortoise) PersistTortoise() error {
return err
}
return ni.Persist(mesh.TORTOISE, ni.ninjaTortoise)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

@almogdepaz almogdepaz Jan 9, 2020

Choose a reason for hiding this comment

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

oops, thats what happens when you trust the merge tool

tortoise/ninja_tortoise.go Show resolved Hide resolved
tortoise/ninja_tortoise.go Show resolved Hide resolved
func (ni *NinjaTortoise) initTallyToBase(base votingPattern, p votingPattern, windowStart types.LayerID) {
mp := make(map[BlockIDLayerTuple]vec, len(ni.TTally[base]))
for b, v := range ni.TTally[base] {
if b.Layer() < windowStart {
Copy link
Contributor

@barakshani barakshani Jan 11, 2020

Choose a reason for hiding this comment

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

Something doesn't add up here. This condition seems to always be satisfied (except maybe for the very first layers), because b.Layer() < p_base <= windowStart.
More generally, understanding the leak cause, it seems that this entire function is unnecessary: we never need to actually copy p_base's opinion, as it is irreversible, and in addition we should have done SaveOpinion to p_base opinion by here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well your kinda right, i guess that now that we treat everything beneath pbase as irreversible and we already evicted all the "irreversible" blocks from the maps, we only need to consider blocks above pbase, need to remember that the case where windowstart > pbase is not handled.
but what about pbase - hdist we have explicit votes there, and i think we have to consider them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should switch to
windowStart = Max(ni.PBase.Layer()-ni.Hdist, newlyr.Index()-Window+1)

return err
}

if !valid {
ni.With().Warning("block is contextually invalid", log.BlockId(bid.String()))
ni.With().Warning("block is contextually invalid", log.BlockId(b.BlockID.String()))
Copy link
Contributor

Choose a reason for hiding this comment

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

here you use b.BlockID and in other places you use b.Id() why not unify ?

Copy link
Contributor Author

@almogdepaz almogdepaz Jan 12, 2020

Choose a reason for hiding this comment

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

barak asked for this, im not about to refactor the whole thing, anyway barak is going through this i dont see why you should too

TEffectiveToBlocks map[votingPattern][]types.BlockID //inverse blocks effective pattern
TSupport map[votingPattern]int //for pattern p the number of blocks that support p
TComplete map[votingPattern]struct{} //complete voting Patterns
TEffectiveToBlocks map[votingPattern][]BlockIDLayerTuple //inverse blocks effective pattern
Copy link
Contributor

Choose a reason for hiding this comment

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

not suggesting we should do it now be technically it could also be
map[votingPattern]map[types.LayerID][]types.BlockID right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could but its simpler this way

@@ -55,6 +55,10 @@ func AddCommands(cmd *cobra.Command) {

cmd.PersistentFlags().IntVar(&config.GenesisActiveSet, "genesis-active-size",
config.GenesisActiveSet, "The active set size for the genesis flow")

cmd.PersistentFlags().IntVar(&config.BlockCacheSize, "block-cache-size",
Copy link
Contributor

Choose a reason for hiding this comment

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

please add this in config.toml as well just for reference.

@almogdepaz
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Jan 15, 2020
## Motivation
fix tortoise increasing memory issue
@bors
Copy link

bors bot commented Jan 15, 2020

Build failed

@almogdepaz
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Jan 15, 2020
## Motivation
fix tortoise increasing memory issue
@bors
Copy link

bors bot commented Jan 15, 2020

Build failed

@almogdepaz
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Jan 15, 2020
## Motivation
fix tortoise increasing memory issue
@almogdepaz
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Jan 16, 2020
## Motivation
fix tortoise increasing memory issue
@bors
Copy link

bors bot commented Jan 16, 2020

Canceled

@almogdepaz
Copy link
Contributor Author

bors merge

1 similar comment
@almogdepaz
Copy link
Contributor Author

bors merge

@almogdepaz almogdepaz merged commit 66e9a7e into develop Jan 16, 2020
@bors bors bot deleted the tortoise_increasing_memory branch January 16, 2020 13:43
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