Skip to content
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.

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

Virtual growing arena Out_Of_Memory bug #4834

Open
karl-zylinski opened this issue Feb 12, 2025 · 9 comments · May be fixed by #4836
Open

Virtual growing arena Out_Of_Memory bug #4834

karl-zylinski opened this issue Feb 12, 2025 · 9 comments · May be fixed by #4836

Comments

@karl-zylinski
Copy link
Contributor

karl-zylinski commented Feb 12, 2025

Context

    Odin:    dev-2025-01:1613728a6
    OS:      Windows 10 Professional (version: 22H2), build 19045.5371
    CPU:     Intel(R) Core(TM) i7-6950X CPU @ 3.00GHz
    RAM:     32674 MiB
    Backend: LLVM 18.1.8

Current Behavior

Run this program. It crashes with:
C:/Projects/Odin Playground/playground.odin(15:2) runtime assertion: Out_Of_Memory

package virtual_growing_arena_bug

import vmem "core:mem/virtual"
import "core:mem"
import "core:fmt"

main :: proc() {
	arena: vmem.Arena
	arena_allocator := vmem.arena_allocator(&arena)

	err: mem.Allocator_Error
	_, err = mem.alloc(4193371, 1, arena_allocator)
	fmt.assertf(err == nil, "%v", err)
	_, err = mem.alloc(896, 64, arena_allocator)
	fmt.assertf(err == nil, "%v", err)
}
@karl-zylinski
Copy link
Contributor Author

I think it's something with how 896 + 64 + 4193371 just barely goes over 4 MB, but only because of the 64 bytes alignment. For some reason it then blows up.

@karl-zylinski
Copy link
Contributor Author

Actually, this no longer happens in latest version of Odin. It was fixed by this: 98b3a9e

@karl-zylinski
Copy link
Contributor Author

Actually, now I got this again, with the exact same repro as above. Either I messed something up or it's slightly system-dependent.

@karl-zylinski karl-zylinski reopened this Feb 12, 2025
@Barinzaya
Copy link
Contributor

Barinzaya commented Feb 12, 2025

Actually, this no longer happens in latest version of Odin. It was fixed by this: 98b3a9e

That change only affects resizes. There are no resizes in your repro, so I wouldn't expect that change to have any impact.

@karl-zylinski
Copy link
Contributor Author

karl-zylinski commented Feb 12, 2025

Actually, this no longer happens in latest version of Odin. It was fixed by this: 98b3a9e

That change only affects resizes. There are no resizes in your repro, so I wouldn't expect that change to have any impact.

Yeah, I think it only made it slightly more rare in my real-world case. But like you say, in my repro it shouldn't matter.

@Barinzaya
Copy link
Contributor

It does seem to reproduce consistently for me using the current repro code.

@jasonKercher
Copy link
Contributor

jasonKercher commented Feb 12, 2025

This fixes it:

diff --git a/core/mem/virtual/arena.odin b/core/mem/virtual/arena.odin
index 5191505cf..e2fb36688 100644
--- a/core/mem/virtual/arena.odin
+++ b/core/mem/virtual/arena.odin
@@ -108,6 +108,14 @@ arena_alloc :: proc(arena: ^Arena, size: uint, alignment: uint, loc := #caller_l
        switch arena.kind {
        case .Growing:
                needed := mem.align_forward_uint(size, alignment)
+               if arena.curr_block != nil {
+                       ptr := uintptr(arena.curr_block.base[arena.curr_block.used:])
+                       mask := uintptr(alignment)-1
+                       if ptr & mask != 0 {
+                               needed += uint(uintptr(alignment) - (ptr & mask))
+                       }
+               }
+
                if arena.curr_block == nil || (safe_add(arena.curr_block.used, needed) or_else 0) > arena.curr_block.reserved {
                        if arena.minimum_block_size == 0 {
                                arena.minimum_block_size = DEFAULT_ARENA_GROWING_MINIMUM_BLOCK_SIZE

That is essentially calc_alignment_offset that is embedded into alloc_from_memory_block in virtual.odin.

E: The logic to decide to allocate a new block does not account for aligning the current block forward.

@karl-zylinski
Copy link
Contributor Author

Thank you @jasonKercher! Would you mind making a PR with that fix?

@jasonKercher
Copy link
Contributor

I could. That was just proof. Do we think it would be cleaner to expose calc_alignment_offset to avoid duplicating the logic?

jasonKercher added a commit to jasonKercher/Odin that referenced this issue Feb 12, 2025
The logic that determines whether we need to allocate more space does
not account having to align the end forward for requested alignment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants