Skip to content

Conversation

@roman-khimov
Copy link
Contributor

A part of from #967, the test itself isn't added here, it's rather trivial and likely not the best way to check these problems, but it was sufficient for the purpose.

Seems like it was more useful before 263e75d, but now it's only used for statistics which can easily be managed in a different way. I see no other valid purposes for this list, a reference can have some value for GC, but if DB user loses a reference to transaction that is not closed there is not much DB can do. This improves ConcurrentView test from

workers samples min avg 50% 80% 90% max
1 10 49.323µs 974.287µs 1.068978ms 1.112882ms 1.131938ms 1.131938ms
10 100 32.592µs 685.315µs 980.5µs 1.125385ms 1.137678ms 1.169789ms
100 1000 31.49µs 219.084µs 77.427µs 353.651µs 656.916µs 1.785808ms
1000 10000 30.668µs 1.639366ms 99.128µs 3.086665ms 5.031354ms 16.315849ms
10000 100000 30.818µs 40.893475ms 36.963667ms 78.650583ms 111.553136ms 302.412177ms

to

workers samples min avg 50% 80% 90% max
1 10 78.358µs 964.847µs 1.059159ms 1.073256ms 1.07551ms 1.07551ms
10 100 32.802µs 304.922µs 80.924µs 674.54µs 1.069298ms 1.220625ms
100 1000 30.758µs 304.541µs 64.192µs 397.094µs 1.101991ms 2.183302ms
1000 10000 30.558µs 1.05711ms 92.426µs 2.111896ms 3.317894ms 11.790014ms
10000 100000 30.548µs 10.98898ms 90.742µs 21.740659ms 33.020076ms 135.33094ms

Seems like it was more useful before 263e75d,
but now it's only used for statistics which can easily be managed in a
different way. I see no other valid purposes for this list, a reference can
have some value for GC, but if DB user loses a reference to transaction that
is not closed there is not much DB can do. This improves ConcurrentView test
from

workers samples min             avg             50%             80%             90%             max
1       10      49.323µs        974.287µs       1.068978ms      1.112882ms      1.131938ms      1.131938ms
10      100     32.592µs        685.315µs       980.5µs         1.125385ms      1.137678ms      1.169789ms
100     1000    31.49µs         219.084µs       77.427µs        353.651µs       656.916µs       1.785808ms
1000    10000   30.668µs        1.639366ms      99.128µs        3.086665ms      5.031354ms      16.315849ms
10000   100000  30.818µs        40.893475ms     36.963667ms     78.650583ms     111.553136ms    302.412177ms

to

workers samples min             avg             50%             80%             90%             max
1       10      78.358µs        964.847µs       1.059159ms      1.073256ms      1.07551ms       1.07551ms
10      100     32.802µs        304.922µs       80.924µs        674.54µs        1.069298ms      1.220625ms
100     1000    30.758µs        304.541µs       64.192µs        397.094µs       1.101991ms      2.183302ms
1000    10000   30.558µs        1.05711ms       92.426µs        2.111896ms      3.317894ms      11.790014ms
10000   100000  30.548µs        10.98898ms      90.742µs        21.740659ms     33.020076ms     135.33094ms

Signed-off-by: Roman Khimov <[email protected]>
@k8s-ci-robot
Copy link

Hi @roman-khimov. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ahrtr
Copy link
Member

ahrtr commented Jun 6, 2025

/ok-to-test

@ahrtr
Copy link
Member

ahrtr commented Jun 6, 2025

cc @Elbehery @fuweid @ivanvc @tjungblu

@Elbehery
Copy link
Member

Elbehery commented Jun 6, 2025

I thought @tjungblu wanted to proceed on refactoring the freeList

I recall we have found a bug, which was the motivation for this refactoring iiuc

@ahrtr
Copy link
Member

ahrtr commented Jun 6, 2025

I thought @tjungblu wanted to proceed on refactoring the freeList

Yes, but it doesn't conflict with this cleanup.

@Elbehery
Copy link
Member

Elbehery commented Jun 6, 2025

ok, then if it was used only for stats, we can remove it

LGTM 👍🏽

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM & thx

@ahrtr ahrtr merged commit f33bb13 into etcd-io:release-1.4 Jun 9, 2025
11 checks passed
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, roman-khimov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr
Copy link
Member

ahrtr commented Jun 9, 2025

Opss, I didn't realize that this PR is against release-1.4. We should avoid making such change in stable release. We should only make the change on main branch instead.

Action: revert the change in release-1.4, and deliver a PR on main instead. cc @roman-khimov @Elbehery

@roman-khimov
Copy link
Contributor Author

That was one of the questions of #967. I'll rebase to main and open a new PR.

@Elbehery
Copy link
Member

Elbehery commented Jun 9, 2025

I will revert it now 👍🏽

Elbehery added a commit to Elbehery/bbolt that referenced this pull request Jun 9, 2025
Elbehery added a commit to Elbehery/bbolt that referenced this pull request Jun 9, 2025
This reverts commit f33bb13, reversing
changes made to cc41cbb.

Signed-off-by: Mustafa Elbehery <[email protected]>
ahrtr added a commit that referenced this pull request Jun 10, 2025
…-list

[release-1.4] Revert "Merge pull request #969 from nspcc-dev/drop-unused-txs"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants