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

go/storage/mkvs/urkel: Figure out why the remove tests are so slow #2044

Closed
Yawning opened this issue Aug 28, 2019 · 3 comments · Fixed by #2051
Closed

go/storage/mkvs/urkel: Figure out why the remove tests are so slow #2044

Yawning opened this issue Aug 28, 2019 · 3 comments · Fixed by #2051
Assignees
Labels
c:bug Category: bug c:performance Category: performance c:security Category: security sensitive c:storage Category: storage golang p:1 Priority: core feature

Comments

@Yawning
Copy link
Contributor

Yawning commented Aug 28, 2019

The CI time consumed by the LevelDB/BadgerDB urkel removal tests is gigantic.

There is a stopgap fix for this in #2018 that reduces the number of k/v pairs used from 1000 to 500 that shaves 5 mins off the typical CI time, but the root cause should be determined, and this should be solved properly if possible.

@Yawning Yawning added p:1 Priority: core feature c:storage Category: storage golang c:performance Category: performance labels Aug 28, 2019
@kostko
Copy link
Member

kostko commented Aug 28, 2019

After a very quick glance the biggest increase comes from write log merging, #1975. Will take a look when I come back.

The other time comes from fsync after every commit which should definitely be disabled for tests.

@Yawning Yawning mentioned this issue Aug 29, 2019
3 tasks
@kostko
Copy link
Member

kostko commented Aug 30, 2019

It seems most of the time is spent fetchng, decoding, re-encoding and storing the write logs during merges (this was introduced in #1975).

While in reality the merge log chains will be much smaller than in this test case (e.g., max 2 steps instead of 500 or 1000 like in this test) and this should be enforced to prevent DoS (see #1914, "limit the depth of forked roots"), there are still things that need to actually be fixed to ensure correctness (while looking into this I figured the current implementation can actually be exploited to make the node lose finalized write logs).

@kostko kostko added c:bug Category: bug c:security Category: security sensitive labels Aug 30, 2019
@kostko
Copy link
Member

kostko commented Sep 2, 2019

After the fixes in #2051 the complete LevelDB test suite takes ~1 sec and Badger around ~2 sec. The difference with CI (and make test-unit) is the race detector which adds a couple of seconds more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:bug Category: bug c:performance Category: performance c:security Category: security sensitive c:storage Category: storage golang p:1 Priority: core feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants