Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Apr 8, 2020

No description provided.

@NikVolf NikVolf requested a review from cecton April 8, 2020 15:09
@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

/bench import

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: nv-fix-bench-setup

Benchmark: Import Benchmark (random transfers)

Master: 697.50 ms
Branch: 20.91 ms
Change: -97.00%

@NikVolf NikVolf force-pushed the nv-fix-bench-setup branch from ceebd31 to 7471cf9 Compare April 8, 2020 15:11
@NikVolf NikVolf added the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Apr 8, 2020
@NikVolf NikVolf added this to the 2.0 milestone Apr 8, 2020
@cecton
Copy link
Contributor

cecton commented Apr 8, 2020

It feels weird that this value (128) is hardcoded at so many different places.

@NikVolf NikVolf added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). labels Apr 8, 2020
@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

hmm.. according to parity-common/kvdb-rocksdb, cache size is in MiB, but it does not explain performance degradation by setting it to 128

probably something is getting rounded to 0 somewhere

@cecton
Copy link
Contributor

cecton commented Apr 8, 2020

I mean, maybe we should put a const somewhere with the value (or parametrize it in sc_service::Configuration)... because right now if we need to change the value we have to do it in 5 different places.

@NikVolf NikVolf force-pushed the nv-fix-bench-setup branch from 7471cf9 to 6d42c16 Compare April 8, 2020 15:53
@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

/bench import

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: nv-fix-bench-setup

Benchmark: Import Benchmark (random transfers)

Master: 697.35 ms
Branch: 19.54 ms
Change: -97.20%

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

I mean, maybe we should put a const somewhere with the value (or parametrize it in sc_service::Configuration)... because right now if we need to change the value we have to do it in 5 different places.

this is rather out of scope of this pr, I want to resolve issue with benchmarks reporting degradation of 10x caused by #5271

And at the moment, I am not sure that only benchmarks are affected

@bkchr
Copy link
Member

bkchr commented Apr 8, 2020

Before the linked pr the value was set to None. This meant we used the default size per column that kvdb sets. And that is way higher as 128MB per column.

If we set 128MB, it means that all columns needs to share this and the state gets 90% of this.

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

Before the linked pr the value was set to None. This meant we used the default size per column that kvdb sets. And that is way higher as 128MB per column.

If we set 128MB, it means that all columns needs to share this and the state gets 90% of this.

I think there is error in calculations (rounding) somewhere

Now setting cache-size = 128 basically disables rocksdb cache and leads to this 10x-20x degradation ^^^

@bkchr
Copy link
Member

bkchr commented Apr 8, 2020

Yes we round these numbers: https://github.com/paritytech/substrate/blob/master/client/db/src/utils.rs#L236

However, this is not your problem. You have the problem that I explained above. 128MB is for all columns, while the state column gets 90% of it.

Before when the value was None, it set the default value here in kvdb.

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

Well now if we run --db-cache=64 we get 0 Mb cache for all columns except state column, which is not correct (cache should never be disabled for any column, and 0 disables it).

But it still does not explain such degradation with 128 value.

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

You have the problem that I explained above.

This does not explain degradation. Such degradation is only possible when cache is disabled.

@bkchr
Copy link
Member

bkchr commented Apr 8, 2020

Why only when the cache is disabled? Even when you have much less cache, the performance could degrade heavily as stuff is constantly paged out of cache. A small cache leads to the same behaviors as a disabled cache.

@NikVolf NikVolf force-pushed the nv-fix-bench-setup branch from a5450d3 to 52493c3 Compare April 8, 2020 16:50
@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

Why only when the cache is disabled? Even when you have much less cache, the performance could degrade heavily as stuff is constantly paged out of cache. A small cache leads to the same behaviors as a disabled cache.

True, but have no idea how to profile it, I think node performance might be affected as well with these settings. One or several of the column cache might also be too small. Probably not the state one, since it takes 90%. I think setting it to some minimum value (5MiB) might help.

@bkchr
Copy link
Member

bkchr commented Apr 8, 2020

We set the value 128MB after some testing. The point was to have validators running without using abusive amounts of memory.
When syncing you need to increase this value to get better performance, but this is expected.

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

We set the value 128MB after some testing. The point was to have validators running without using abusive amounts of memory.
When syncing you need to increase this value to get better performance, but this is expected.

I know, still there is no explanation for some observations.

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

/bench import

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: nv-fix-bench-setup

Benchmark: Import Benchmark (random transfers)

Master: 693.28 ms
Branch: 697.59 ms
Change: +0.62%

@NikVolf NikVolf force-pushed the nv-fix-bench-setup branch from f4688c8 to 44b8c48 Compare April 8, 2020 17:32
@NikVolf NikVolf changed the title Fix tests & benchmarks: cache_size is in bytes, not in megabytes Fix benchmarks: set cache_size to higher value Apr 8, 2020
@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

/bench import

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: nv-fix-bench-setup

Benchmark: Import Benchmark (random transfers)

Master: 693.53 ms
Branch: 694.02 ms
Change: +0.07%

@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

Ok, observations show that this is state cache that is not enough (for benchmarks at least). This is probably indication of some weird behaviour when specifying memory_budget for kvdb-rocksdb. Because before we didn't specify it and should've got 128Mb per for state column, now we specify 256Mb that should get more than 128Mb for state column and still we get regression (which is eliminated by specifying 512 🤷‍♀️).

@NikVolf NikVolf force-pushed the nv-fix-bench-setup branch from 44b8c48 to 52493c3 Compare April 8, 2020 17:46
@NikVolf
Copy link
Contributor Author

NikVolf commented Apr 8, 2020

/bench import

@parity-benchapp
Copy link

parity-benchapp bot commented Apr 8, 2020

Finished benchmark for branch: nv-fix-bench-setup

Benchmark: Import Benchmark (random transfers)

Master: 693.30 ms
Branch: 19.53 ms
Change: -97.18%

@NikVolf NikVolf added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 8, 2020
@bkchr bkchr merged commit 1009678 into master Apr 9, 2020
@bkchr bkchr deleted the nv-fix-bench-setup branch April 9, 2020 08:37
@bkchr bkchr added the B0-silent Changes should not be mentioned in any release notes label Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants