Use custom allocator on Windows #96
Conversation
|
@ncruces would you be able to take a look at this PR? BTW, the only testing I've done is the windows job in the CI here |
internal/alloc/alloc_windows.go
Outdated
| cap = (cap + rnd) &^ rnd | ||
|
|
||
| if max > math.MaxInt { | ||
| // This ensures int(max) overflows to a negative value. |
There was a problem hiding this comment.
This comment could be tweaked: unix.Mmap uses signed int (hence my wording); Windows is using uintptr and SIZE_T (both unsigned).
The code still makes sense: it ensures that on 32-bit platforms a very large value overflows to either -1 (Unix) or MaxUint32 (both of which cause a noisy error, rather than the silent corruption).
internal/alloc/alloc_windows.go
Outdated
| // Update committed memory. | ||
| m.buf = m.buf[:new] | ||
| } | ||
| return m.buf[:size] |
There was a problem hiding this comment.
I recommend (which I've also done on Unix):
// Limit returned capacity because bytes beyond
// len(m.buf) have not yet been committed.
return m.buf[:size:len(m.buf)]That way, Go bounds checks will protect the slice.
| r, _, err := procVirtualFree.Call(m.addr, 0, windows_MEM_RELEASE) | ||
| if r == 0 { | ||
| panic(fmt.Errorf("alloc_windows: failed to release memory: %w", err)) | ||
| } |
There was a problem hiding this comment.
In the other version I'm doing m.buf = nil which ensures subsequent calls return EINVAL.
I dunno if m.addr = 0 would offer the same semantics, but I think it does.
internal/alloc/alloc_windows.go
Outdated
| } | ||
|
|
||
| func (m *virtualMemory) Reallocate(size uint64) []byte { | ||
| if com := uint64(len(m.buf)); com < size { |
There was a problem hiding this comment.
I also tweaked this line to ensure I don't try to over commit (and no overflow):
com := uint64(len(m.buf))
res := uint64(cap(m.buf))
if com < size && size < res {
// ...
}
|
Thanks a lot for the help @ncruces! added the cleanups |
|
I was going over this again, and as the wazero API shifted for a while, I guess this slipped. It turns out that we don't need to commit memory in the constructor unless we really want to, because So it's up to you if you want to use |
Also makes sure to cap max pages on 32-bit systems regardless for extra safety