Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 13, 2023

This should be less controversial than commit 151a2b1. The overall size of the qa-assets repo is reduced further from 1.9GB to 1.6GB. Also, the runtime to iterate on the resulting folder is reduced further from ~1699s to ~1149s (N=1).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 13, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge
Concept ACK murchandamus

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

ACK fa858d6

@maflcko
Copy link
Member Author

maflcko commented Oct 13, 2023

Further reading: llvm/llvm-project@e6597db

@maflcko
Copy link
Member Author

maflcko commented Oct 13, 2023

Would be nice if one (1) person (or more) could re-check the runtime estimate on their machine. The steps to reproduce were:

  • Clone the current qa-assets repo
  • Call the python script from this pull (and from master) via ./test/fuzz/test_runner.py -j $( nproc ) -l DEBUG --m_dir ../qa-assets/fuzz_seed_corpus/ temp_new_folder_number_00
  • Get the runtime (twice) via /usr/bin/time -f '%M KB, %S + %U' ./test/fuzz/test_runner.py -l DEBUG --par $( nproc ) ./temp_folder...

@murchandamus
Copy link
Contributor

crACK fa858d6

I’m attempting to run the requested verification, but utxo_total_supply is absurdly slow on the merge. Will report back when it finally gets done. Definitely reduced the set on a prior merge I did, though.

@DrahtBot DrahtBot removed the request for review from murchandamus October 13, 2023 19:09
@murchandamus
Copy link
Contributor

murchandamus commented Oct 13, 2023

So much ACK

Running main unmerged:

  1. 622632 KB, 161.30 + 6192.01

Merging main to a fresh directory with the code changes in #28650 (IIRC: src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0):

  1. 460324 KB, 47.46 + 1466.82
  2. 459752 KB, 48.92 + 1469.17

BTW, on the second run here, utxo_total_supply took 734 s for 422 seeds, on the main branch it took 2464 s for 2985 runs.

@maflcko
Copy link
Member Author

maflcko commented Oct 14, 2023

I think you forgot to -merge=1 on the master branch commit, for a fair comparison?

@fanquake fanquake merged commit ab2f531 into bitcoin:master Oct 15, 2023
@maflcko maflcko deleted the 2310-fuzz-set-merge- branch October 16, 2023 10:59
@murchandamus
Copy link
Contributor

Sorry, I misinterpreted your instructions, now running the merge per the old style to get the execution time on that

@murchandamus
Copy link
Contributor

murchandamus commented Oct 16, 2023

After merging main to a fresh directory with src/test/fuzz/fuzz -merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=1 running all seeds took:
592620 KB, 126.62 + 4689.42

After merging main to a fresh directory with src/test/fuzz/fuzz -merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 it took:
560756 KB, 73.80 + 2374.22

@maflcko
Copy link
Member Author

maflcko commented Oct 17, 2023

I think you compared -use_value_profile=, but this pull request is about -set_cover_merge=.

@murchandamus
Copy link
Contributor

murchandamus commented Oct 17, 2023

I did four different things:

  1. Run the seeds on qa-assets/main:

    622632 KB, 161.30 + 6192.01

  2. Merge main to a fresh directory with -set_cover_merge=1 -use_value_profile=0, then run the seeds in the fresh directory:

    460324 KB, 47.46 + 1466.82
    459752 KB, 48.92 + 1469.17

  3. Merge main to a fresh directory with -merge=1 -use_value_profile=1, then run the resulting seeds:

    592620 KB, 126.62 + 4689.42

  4. Merge main to a fresh directory with -merge=1 -use_value_profile=0, then run those seeds:

    560756 KB, 73.80 + 2374.22

So if you’re interested in the comparison of -set_cover_merge and -merge, please refer to 2 and 4, if you are interested in the overall speed-up of both changes (-merge=1 -use_value_profile=1 ↦ -set_cover_merge=1 -use_value_profile=0), please refer to 2 and 3. If you’re interested in how much we could reduce the runtime for the qa-assets/main branch, please refer to 1 and 2. I figured running the main branch qa-assets may also provide a baseline for how fast/slow this system is.

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023
fa858d6 fuzz: Merge with -set_cover_merge=1 (MarcoFalke)

Pull request description:

  This should be less controversial than commit 151a2b1. The overall size of the qa-assets repo is reduced further from 1.9GB to 1.6GB. Also, the runtime to iterate on the resulting folder is reduced further from ~1699s to ~1149s (N=1).

ACKs for top commit:
  murchandamus:
    crACK fa858d6
  dergoegge:
    ACK fa858d6

Tree-SHA512: e23fa93bd48f01d11c551b035004c678bd6d76bc24ac7d0d0a7883060804e6711763cbd0cd0ded3aad3e4c40da764decae81c2703388cc11961def3c89a4f9ba
@bitcoin bitcoin locked and limited conversation to collaborators Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants