odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.17k stars 550 forks source link

memory alignment bug in core:mem.dynamic_pool_alloc_bytes() in cases where no padding is needed? #3692

Closed DerTee closed 1 week ago

DerTee commented 1 month ago

Context

I tried writing a test for the mem.Dynamic_Pool allocator to understand how it works exactly. But it adds 8 bytes of unnecessary padding for aligned data sizes. For unaligned ones it works as expected.

Operating System & Odin Version:

Odin:    dev-2024-06:fcfc1cb97
OS:      Windows 11 Home Basic (version: 23H2), build 22631.3593
CPU:     AMD Ryzen 5 2600 Six-Core Processor
RAM:     16335 MiB
Backend: LLVM 17.0.1

Expected Behavior

I expect the dynamic pool allocator to use the minimal amount of memory for data sizes that are multiples of the allocators alignment and a maximum of number_of_alignment_bytes - 1 for misaligned ones.

My guess is that this would solve the problem:

Current version, maybe broken in dynamic_pool_alloc_bytes():

    n := bytes
    extra := p.alignment - (n % p.alignment)
    n += extra

Possibly a fix (it works in for my test code below):

     n := align_formula(bytes, p.alignment)

Current Behavior

Unaligned data works correctly, aligned data reserves the number of bytes of alignment extra. Maybe this is intentional and I don't understand why.

Failure Information (for bugs)

Please help provide information about the failure if this is a bug. If it is not a bug, please remove the rest of this template.

Steps to Reproduce

Run odin test on this snippet:

package test_mem

import "core:testing"
import "core:mem"

try_pool_allocation :: proc(t: ^testing.T, expected_used_bytes, num_bytes, alignment: int) {
    pool: mem.Dynamic_Pool
    mem.dynamic_pool_init(pool = &pool, alignment = alignment)
    pool_allocator := mem.dynamic_pool_allocator(&pool)

    _, _ = mem.alloc(num_bytes, alignment, pool_allocator)

    expected_bytes_left := pool.block_size - expected_used_bytes
    testing.expectf(t, pool.bytes_left == expected_bytes_left,
        "Allocated data with size %v bytes, expected %v bytes left, got %v bytes left, off by %v bytes",
        num_bytes, expected_bytes_left, pool.bytes_left, expected_bytes_left - pool.bytes_left)

    mem.dynamic_pool_destroy(&pool)
}

@(test)
test_dynamic_pool_alloc_aligned :: proc(t: ^testing.T) {
    try_pool_allocation(t, expected_used_bytes = 8, num_bytes = 8, alignment=8)
    try_pool_allocation(t, expected_used_bytes = 32, num_bytes = 32, alignment=8)
}

@(test)
test_dynamic_pool_alloc_unaligned :: proc(t: ^testing.T) {
    try_pool_allocation(t, expected_used_bytes =   8,   num_bytes=1, alignment=8)
    try_pool_allocation(t, expected_used_bytes =   16,  num_bytes=9, alignment=8)
}

Failure Logs

D:\dev\Odin>odin.exe test tests/core/mem
[INFO ] --- [2024-06-06 07:25:59] Starting test runner with 2 threads. Set with -define:ODIN_TEST_THREADS=n.
[INFO ] --- [2024-06-06 07:25:59] The random seed sent to every test is: 259699811096020. Set with -define:ODIN_TEST_RANDOM_SEED=n.
[INFO ] --- [2024-06-06 07:25:59] Memory tracking is enabled. Tests will log their memory usage if there's an issue.
[INFO ] --- [2024-06-06 07:25:59] < Final Mem/ Total Mem> <  Peak Mem> (#Free/Alloc) :: [package.test_name]
[ERROR] --- [2024-06-06 07:25:59] [test_mem_dynamic_pool.odin:15:try_pool_allocation()] Allocated data with size 16 bytes, expected 65520 bytes left, got 65512 bytes left, off by 8 bytes
asd  [||                      ]         2 :: [package done] (1 failed)
Finished 2 tests in 3.6901ms. 1 test failed.
 - asd.test_dynamic_pool_alloc_aligned          Allocated data with size 16 bytes, expected 65520 bytes left, got 65512 bytes left, off by 8 bytes