addrman, refactor: improve stochastic test in AddSingle
#27319
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
This doesn't seem like a big win. Have you benchmarked this to see if it actually makes a difference? (fwiw I think the bigger optimization you are making here is using |
Just updated the description, thanks.
In a general view perhaps we won't see a HUGE difference, didn't benchmark to ensure. However, it changes this algorithm from O(n) - where |
|
Yeah, the left shift should be faster than the iterative approach. Yet I'm not sure how much of an improvement it will make in terms of efficiency (because I'm not sure how big pinfo -> nRefCount could be). But, there are other instances in the same file where the Left shift is already being used for the same purpose. As a result, I see no point in continuing to use the current iterative method for calculating the power of two. It's better to change it for the sake of consistency and better practice. |
|
There hasn't been much activity lately. What is the status here? Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future. |
|
I don't think this is a significant improvement, but the slight improvement seems fine since it's small, easy to check that there's no delta in behavior, and doesn't conflict with other PRs. |
This commit changes this algo to be O(1) instead of O(n) by using `<<`.
a7ba3a2 to
e064487
Compare
|
Force-pushed addressing #27319 (comment). Also updated the description. |
There was a problem hiding this comment.
ACK e064487. simple use of << instead of a loop, didn't observe any behaviour difference before and after.
if pinfo->nRefCount is 0, we created an unnecessary variable (nFactor)
i don't think pinfo->nRefCount can be 0 here though. pinfo->nRefCount can take on values [1, 7] here. the very first time when we add an address, it would go to the else block instead of the current if block.
|
ACK e064487 |
…ddSingle` e064487 addrman, refactor: improve stochastic test in `AddSingle` (brunoerg) Pull request description: This PR changes this algorithm to be O(1) instead of O(n). Also, in the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`), this changes it. the change is relatively simple and does not cause conflicts. ACKs for top commit: achow101: ACK e064487 amitiuttarwar: ACK e064487 stratospher: ACK e064487. simple use of << instead of a loop, didn't observe any behaviour difference before and after. Tree-SHA512: ff0a65155e47f65d2ce3cb5a3fd7a86efef1861181143df13a9d8e59cb16aee9be2f8801457bba8478b17fac47b015bff5cc656f6fac2ccc071ee7178a38d291
…ddSingle` e064487 addrman, refactor: improve stochastic test in `AddSingle` (brunoerg) Pull request description: This PR changes this algorithm to be O(1) instead of O(n). Also, in the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`), this changes it. the change is relatively simple and does not cause conflicts. ACKs for top commit: achow101: ACK e064487 amitiuttarwar: ACK e064487 stratospher: ACK e064487. simple use of << instead of a loop, didn't observe any behaviour difference before and after. Tree-SHA512: ff0a65155e47f65d2ce3cb5a3fd7a86efef1861181143df13a9d8e59cb16aee9be2f8801457bba8478b17fac47b015bff5cc656f6fac2ccc071ee7178a38d291
b70e091 Merge bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test (fanquake) 6d7aa3d Merge bitcoin#29497: test: simplify test_runner.py (fanquake) d0e15d5 Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper conversions (Ava Chow) 045fa5f Merge bitcoin#29514: tests: Provide more helpful assert_equal errors (Ava Chow) bd607f0 Merge bitcoin#29393: i2p: log connection was refused due to arbitrary port (Ava Chow) c961755 Merge bitcoin#29595: doc: Wrap flags with code in developer-notes.md (fanquake) 8d6e5e7 Merge bitcoin#29583: fuzz: Apply fuzz env (suppressions, etc.) when fetching harness list (fanquake) 4dce690 Merge bitcoin#29576: Update functional test runner to return error code when no tests are found to run (fanquake) 910a7d6 Merge bitcoin#29529: fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) fdac2b3 Merge bitcoin#29493: subtree: update crc32c subtree (fanquake) a23b342 Merge bitcoin#29475: doc: Fix Broken Links (fanquake) 92bad90 Merge bitcoin#28178: fuzz: Generate with random libFuzzer settings (fanquake) 9b6a05d Merge bitcoin#29443: depends: fix BDB compilation on OpenBSD (fanquake) 9963e6b Merge bitcoin#29413: fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (fanquake) 3914745 Merge bitcoin#29425: test: fix intermittent failure in wallet_reorgrestore.py (fanquake) b719883 Merge bitcoin#29399: test: Fix utxo set hash serialisation signedness (fanquake) f096880 Merge bitcoin#29377: test: Add makefile target for running unit tests (Ava Chow) 03e0bd3 Merge bitcoin#27319: addrman, refactor: improve stochastic test in `AddSingle` (Ava Chow) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## What was done? See commits ## How Has This Been Tested? built locally; large combined merge passed tests locally ## Breaking Changes Should be none ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b70e091 knst: utACK b70e091 Tree-SHA512: 659a931f812c8a92cf3854abb873d92885219a392d6aa8e49ac4b27fe41e8e163ef9a135050e7c2e1bd33cecd2f7dae215e05a9c29f62e837e0057d3c16746d6
This PR changes this algorithm to be O(1) instead of O(n). Also, in the current implementation, if
pinfo->nRefCountis 0, we created an unnecessary variable (nFactor), this changes it. the change is relatively simple and does not cause conflicts.