pkg/util/hack: split map ABI for go1.25 and go1.26#66254
pkg/util/hack: split map ABI for go1.25 and go1.26#66254hawkingrei wants to merge 16 commits intopingcap:masterfrom
Conversation
|
✅ Review completed. No issues found. The PR looks good to merge. |
|
|
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
|
@pantheon-ai[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
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. |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
1 similar comment
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66254 +/- ##
================================================
- Coverage 77.6934% 77.5161% -0.1773%
================================================
Files 2006 1928 -78
Lines 548247 535790 -12457
================================================
- Hits 425952 415324 -10628
+ Misses 120635 120460 -175
+ Partials 1660 6 -1654
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| //go:build go1.25 && !go1.26 |
There was a problem hiding this comment.
Where does the build process add go1.25 tag? I didn't find the doc related to it 😂
And seems we only need one version tag?
//go:build go1.25
//go:build go1.26
should be enough
There was a problem hiding this comment.
There was a problem hiding this comment.
Gemini says it's called ReleaseTags, golang will assign all tags <= current version, so we should use
//go:build go1.25 && !go1.26
However I didn't find the official document for it. Do you have more authorized materials for this feature?
As #65761 and this comment, we can pin the version of golang used to compile TiDB. But if golang can't reliably pin a version for developer, as the situation of your development environment @hawkingrei , I'm good for this PR. |
|
/hold
|
It's impossible unless Go itself reverts those map changes. |
seems i have some mis-understanding of the problem we are facing. anyway i hate the idea that we have to file a PR like this every time golang release a new version. i prefer we use or modify the implementation of CRDB directly, if we want to estimate the memory taken by the map, and i check the number of callsites that depends on the memory is not that large |
|
/retest |
| // Number of slots in a group. | ||
| const mapGroupSlots = 1 << mapGroupSlotsBits // 8 | ||
|
|
||
| // $GOROOT/src/internal/runtime/maps/table.go:`type table struct` |
There was a problem hiding this comment.
Can AI tools help to add and check memory size & layout of such struct?
There was a problem hiding this comment.
We can add a new skills for it. we can add this new skills in the next PR.
|
@solotzg: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
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. |
|
/unhold |
It won't work, just like when I upgraded my system and couldn't compile TiDB anymore. |
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pantheon-ai[bot], solotzg, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #65761
Problem Summary:
pkg/util/hack/map_abi.gois hardcoded to the Go 1.25 swiss-map ABI and panics under Go 1.26. We need version-specific ABI definitions to keep the memory-aware map implementation working across both toolchains.What changed and how does it work?
pkg/util/hack:map_abi.gois now guarded by//go:build go1.25 && !go1.26map_abi_go126.gois guarded by//go:build go1.26 && !go1.27go1.26.pkg/util/hack/BUILD.bazelto include the new source file.Check List
Tests
test it with go1.26.0
#66256
It can compile with go1.26
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.