Skip to content

icall: replace map with slice #76

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.

Sign up for GitHub

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

Merged
visualfc merged 3 commits into from
May 14, 2026
Merged

icall: replace map with slice #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b82b751
cmd/icall_gen: replace map with slice
visualfc May 14, 2026
da6963c
generate icall
visualfc May 14, 2026
d1cb97b
workflows
visualfc May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/go.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ jobs:
Test-MacOS:
strategy:
matrix:
go-version: [1.18.x, 1.19.x, 1.20.x, 1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x]
go-version: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x]
runs-on: macos-latest
steps:
- uses: actions/checkout@v6
Expand All @@ -29,7 +29,7 @@ jobs:
Test-Linux:
strategy:
matrix:
go-version: [1.18.x, 1.19.x, 1.20.x, 1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x]
go-version: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x]

@fennoai fennoai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI breakage: Linux matrix not updated. This job still includes 1.18.x, 1.19.x, 1.20.x, but go.mod now requires Go 1.21 and clear() (added in 1.21) is used throughout — these versions will fail at compile time. The MacOS matrix was updated in this PR but Linux (here) and Windows (line ~54) were not. Both should be updated to match: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x].

runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
Expand All @@ -51,7 +51,7 @@ jobs:
Test-Windows:
strategy:
matrix:
go-version: [1.18.x, 1.19.x, 1.20.x, 1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x]
go-version: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x]

@fennoai fennoai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CI breakage: Windows matrix not updated. Same issue as the Linux job above — still includes Go 1.18/1.19/1.20 which will fail to compile this module.

runs-on: windows-latest
steps:
- uses: actions/checkout@v6
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*.dll
*.so
*.dylib
.DS_Store

# Test binary, built with `go test -c`
*.test
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Golang reflect package hack tools

### Go Version

- Go1.18 ~ Go1.26
- Go1.21 ~ Go1.26

@fennoai fennoai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The top-level Go version line is correctly updated, but the ABI table a few lines below still documents per-platform behaviour starting at Go 1.18 and Go 1.19 — both below the new minimum. Those rows will never apply to users of this module now. Consider removing or collapsing them.

- macOS Linux Windows WebAssembly

### ABI
Expand Down
32 changes: 22 additions & 10 deletions cmd/icall_gen/_data/icall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
//go:build !go1.17 || (go1.17 && !go1.18 && !goexperiment.regabireflect) || (go1.18 && !go1.19 && !goexperiment.regabireflect && !amd64) || (go1.19 && !go1.20 && !goexperiment.regabiargs && !amd64 && !arm64 && !ppc64 && !ppc64le) || (go1.20 && !goexperiment.regabiargs && !amd64 && !arm64 && !ppc64 && !ppc64le && !riscv64) || (go1.22 && !goexperiment.regabiargs && !amd64 && !arm64 && !ppc64 && !ppc64le && !riscv64 && !loong64)
// +build !go1.17 go1.17,!go1.18,!goexperiment.regabireflect go1.18,!go1.19,!goexperiment.regabireflect,!amd64 go1.19,!go1.20,!goexperiment.regabiargs,!amd64,!arm64,!ppc64,!ppc64le go1.20,!goexperiment.regabiargs,!amd64,!arm64,!ppc64,!ppc64le,!riscv64 go1.22,!goexperiment.regabiargs,!amd64,!arm64,!ppc64,!ppc64le,!riscv64,!loong64
//go:build !goexperiment.regabiargs && !amd64 && !arm64 && !ppc64 && !ppc64le && !riscv64 && !(go1.23 && loong64)
// +build !goexperiment.regabiargs
// +build !amd64
// +build !arm64
// +build !ppc64
// +build !ppc64le
// +build !riscv64
// +build !go1.23 !loong64

package $pkgname

Expand All @@ -13,12 +19,14 @@ import (
const capacity = $max_size

type provider struct {
used map[int]*abi.MethodInfo
used []*abi.MethodInfo
n int
}

func (p *provider) Insert(info *abi.MethodInfo) (ifn unsafe.Pointer, index int) {
for i := 0; i < capacity; i++ {
if _, ok := p.used[i]; !ok {
if p.used[i] == nil {
p.n++
p.used[i] = info
fn := icall_array[i]
return unsafe.Pointer(reflect.ValueOf(fn).Pointer()), i
Comment on lines 26 to 32

@fennoai fennoai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Insert is O(capacity) with no free-slot hint. Every call scans from index 0, so if the first free slot is near the end (e.g. after many removes), performance degrades to O(capacity). For the 20480-slot provider this is up to 20480 pointer comparisons per insertion. A single nextFree int field that advances past occupied slots (with wrap-around) would give O(1) amortized Insert without complicating the rest of the implementation.

Expand All @@ -28,30 +36,34 @@ func (p *provider) Insert(info *abi.MethodInfo) (ifn unsafe.Pointer, index int)
}

func (p *provider) Available() int {
return capacity - len(p.used)
return capacity - p.n
}

func (p *provider) Remove(indexs []int) {
for _, n := range indexs {
delete(p.used, n)
if n < capacity && p.used[n] != nil {
p.used[n] = nil
p.n--
}
}
Comment on lines 43 to 48

@gemini-code-assist gemini-code-assist Bot May 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.

	for _, n := range indexs {
		if p.used[n] != nil {
			p.used[n] = nil
			p.n--
		}
	}

}

func (p *provider) Used() int {
return len(p.used)
return p.n
}

func (p *provider) Cap() int {
return len(icall_array)
return capacity
}

func (p *provider) Clear() {
p.used = make(map[int]*abi.MethodInfo)
clear(p.used)

@fennoai fennoai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clear(p.used) vs clear(p.used[:]) inconsistency. icall.go template uses clear(p.used) while icall_regabi.go uses clear(p.used[:]). Both are semantically identical for a slice, but the inconsistency suggests independent edits rather than uniform code generation. Standardising to one form in the templates avoids confusion.

p.n = 0
}

var (
mp = &provider{
used: make(map[int]*abi.MethodInfo),
used: make([]*abi.MethodInfo, capacity),
}
)

Expand Down
24 changes: 15 additions & 9 deletions cmd/icall_gen/_data/icall_regabi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//go:build (go1.17 && goexperiment.regabireflect) || (go1.19 && goexperiment.regabiargs) || (go1.18 && amd64) || (go1.19 && arm64) || (go1.19 && ppc64) || (go1.19 && ppc64le) || (go1.20 && riscv64) || (go1.23 && loong64)
// +build go1.17,goexperiment.regabireflect go1.19,goexperiment.regabiargs go1.18,amd64 go1.19,arm64 go1.19,ppc64 go1.19,ppc64le go1.20,riscv64 go1.23,loong64
//go:build goexperiment.regabiargs || amd64 || arm64 || ppc64 || ppc64le || riscv64 || (go1.23 && loong64)
// +build goexperiment.regabiargs amd64 arm64 ppc64 ppc64le riscv64 go1.23,loong64

package $pkgname

Expand All @@ -18,7 +18,8 @@ type methodUsed struct {
}

type provider struct {
used map[int]*methodUsed
used []*methodUsed
n int
}

func i_x(c unsafe.Pointer, frame unsafe.Pointer, retValid *bool, r unsafe.Pointer, index int) {
Expand All @@ -33,7 +34,7 @@ func unspillArgs()
func (p *provider) Insert(info *abi.MethodInfo) (unsafe.Pointer, int) {
var index = -1
for i := 0; i < capacity; i++ {
if _, ok := p.used[i]; !ok {
if p.used[i] == nil {
index = i
break
}
Expand Down Expand Up @@ -74,35 +75,40 @@ func (p *provider) Insert(info *abi.MethodInfo) (unsafe.Pointer, int) {
fun: fn,
ptr: (*struct{ typ, ptr unsafe.Pointer })(unsafe.Pointer(&fn)).ptr,
}
p.n++

@gemini-code-assist gemini-code-assist Bot May 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The counter p.n is incremented here regardless of whether a valid slot was found in the initial loop. If index is -1, this increment is incorrect (though the subsequent code will likely panic). Consider incrementing p.n only when a slot is successfully allocated, similar to the implementation in icall.go.

icall := icall_fn[index]
return unsafe.Pointer(reflect.ValueOf(icall).Pointer()), index
}

func (p *provider) Remove(indexs []int) {
for _, n := range indexs {
delete(p.used, n)
if n < capacity && p.used[n] != nil {
p.used[n] = nil
p.n--
}
}
Comment on lines 84 to 89

@gemini-code-assist gemini-code-assist Bot May 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.

	for _, n := range indexs {
		if p.used[n] != nil {
			p.used[n] = nil
			p.n--
		}
	}

Comment on lines 83 to 89

@fennoai fennoai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same Remove issues as in icall.go — no nil-guard and no bounds check. Both need the same fix in this template file.

}

func (p *provider) Available() int {
return capacity - len(p.used)
return capacity - p.n
}

func (p *provider) Used() int {
return len(p.used)
return p.n
}

func (p *provider) Cap() int {
return capacity
}

func (p *provider) Clear() {
p.used = make(map[int]*methodUsed)
clear(p.used)
p.n = 0
}

var (
mp = &provider{
used: make(map[int]*methodUsed),
used: make([]*methodUsed, capacity),
}
)

Expand Down
65 changes: 4 additions & 61 deletions cmd/icall_gen/_data/icall_regabi_amd64.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
//go:build (go1.17 && goexperiment.regabireflect) || (go1.18 && !go1.21)
// +build go1.17,goexperiment.regabireflect go1.18,!go1.21

// Copyright 2012 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
Expand All @@ -23,74 +20,20 @@
#define LOCAL_RETVALID 32
#define LOCAL_REGARGS 40

TEXT ·spillArgs(SB),NOSPLIT,$0-0
MOVQ AX, 0(R12)
MOVQ BX, 8(R12)
MOVQ CX, 16(R12)
MOVQ DI, 24(R12)
MOVQ SI, 32(R12)
MOVQ R8, 40(R12)
MOVQ R9, 48(R12)
MOVQ R10, 56(R12)
MOVQ R11, 64(R12)
MOVQ X0, 72(R12)
MOVQ X1, 80(R12)
MOVQ X2, 88(R12)
MOVQ X3, 96(R12)
MOVQ X4, 104(R12)
MOVQ X5, 112(R12)
MOVQ X6, 120(R12)
MOVQ X7, 128(R12)
MOVQ X8, 136(R12)
MOVQ X9, 144(R12)
MOVQ X10, 152(R12)
MOVQ X11, 160(R12)
MOVQ X12, 168(R12)
MOVQ X13, 176(R12)
MOVQ X14, 184(R12)
RET

// unspillArgs loads args into registers from a *internal/abi.RegArgs in R12.
TEXT ·unspillArgs(SB),NOSPLIT,$0-0
MOVQ 0(R12), AX
MOVQ 8(R12), BX
MOVQ 16(R12), CX
MOVQ 24(R12), DI
MOVQ 32(R12), SI
MOVQ 40(R12), R8
MOVQ 48(R12), R9
MOVQ 56(R12), R10
MOVQ 64(R12), R11
MOVQ 72(R12), X0
MOVQ 80(R12), X1
MOVQ 88(R12), X2
MOVQ 96(R12), X3
MOVQ 104(R12), X4
MOVQ 112(R12), X5
MOVQ 120(R12), X6
MOVQ 128(R12), X7
MOVQ 136(R12), X8
MOVQ 144(R12), X9
MOVQ 152(R12), X10
MOVQ 160(R12), X11
MOVQ 168(R12), X12
MOVQ 176(R12), X13
MOVQ 184(R12), X14
RET

// makeFuncStub is the code half of the function returned by MakeFunc.
// See the comment on the declaration of makeFuncStub in makefunc.go
// for more details.
// No arg size here; runtime pulls arg map out of the func value.
// This frame contains two locals. See the comment above LOCAL_RETVALID.
// amd64 argframe+8(FP) offset to func from method
#define MAKE_FUNC_FN(NAME,INDEX) \
TEXT NAME(SB),(NOSPLIT|WRAPPER),$312 \
NO_LOCAL_POINTERS \
LEAQ LOCAL_REGARGS(SP), R12 \
CALL ·spillArgs(SB) \
CALL runtime·spillArgs(SB) \
MOVQ 24(SP), DX \
MOVQ DX, 0(SP) \
LEAQ argframe+8(FP), CX \
LEAQ argframe+16(FP), CX \
MOVQ CX, 8(SP) \
MOVB $0, LOCAL_RETVALID(SP) \
LEAQ LOCAL_RETVALID(SP), AX \
Expand All @@ -101,5 +44,5 @@ TEXT NAME(SB),(NOSPLIT|WRAPPER),$312 \
MOVQ AX, 32(SP) \
CALL ·i_x(SB) \
LEAQ LOCAL_REGARGS(SP), R12 \
CALL ·unspillArgs(SB) \
CALL runtime·unspillArgs(SB) \
RET
5 changes: 1 addition & 4 deletions cmd/icall_gen/_data/icall_regabi_arm64.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
//go:build (go1.18 && goexperiment.regabireflect) || (go1.19 && !go1.21)
// +build go1.18,goexperiment.regabireflect go1.19,!go1.21

// Copyright 2012 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
Expand Down Expand Up @@ -35,7 +32,7 @@ TEXT NAME(SB),(NOSPLIT|WRAPPER),$432 \
ADD $LOCAL_REGARGS, RSP, R20 \
CALL runtime·spillArgs(SB) \
MOVD 32(RSP), R26 \
MOVD R26, 8(RSP) \
MOVD R26, 16(RSP) \
MOVD $argframe+0(FP), R3 \
MOVD R3, 16(RSP) \
MOVB $0, LOCAL_RETVALID(RSP) \
Expand Down
16 changes: 0 additions & 16 deletions cmd/icall_gen/_data/icall_regabi_go118.go
View file
Open in desktop

This file was deleted.

51 changes: 0 additions & 51 deletions cmd/icall_gen/_data/icall_regabi_go121_amd64.s
View file
Open in desktop

This file was deleted.

Loading
Loading