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

Resources release functions and leaks fixes to the gl_backend #428

Merged
merged 4 commits into from
May 10, 2024

Conversation

eloraiby
Copy link
Contributor

@eloraiby eloraiby commented May 4, 2024

Currently, pipelines and programs are not deleted and stay in memory. This pull request release the gl shader used for linking the program and add a function to release the resource.
Added a resource manager based on HashMaps to ease management (addition/removal) as well.

Note: Apple Metal backend seems to have far more leaks, but these should be addressed in a PR on their own as they require far more substantial work.

…ers, buffers, pipelines, render passes in GL based renderer

Moved resources management from Vec to HashMap
Added glDetach
@eloraiby
Copy link
Contributor Author

eloraiby commented May 7, 2024

@not-fl3 can you review please?

@not-fl3
Copy link
Owner

not-fl3 commented May 8, 2024

Hi, sorry for delay!

Great PR, big issue that required some fix for a loooong time!

I am totally into a part of the PR about deleting GPU resources. However, I am a bit conflicted about HashMaps. In most of my applications, I do access those resources very often, but I almost never reallocate things. And even if I do - those arrays do not have much data, the real data is on the GPU anyway.

I do not have any profiling data to make a decision here :(
The issue needs to be solved, the leaks are real, but, HashMap's taxes on access speed vs some memory overhead of Vec is not an easy decision to make.

@eloraiby
Copy link
Contributor Author

eloraiby commented May 9, 2024

Fedor, according to a quick benchmark, for a Billion access:

Element Count; Default HashMap(ms); NoHasher HashMap(ms); HashMap capacity; Vector(No Indirection)(ms); Vector(Indirection)(ms)
1024         ; 18043              ; 6416                ; 1792            ; 4113                      ; 4116
2048         ; 18633              ; 6514                ; 3584            ; 4114                      ; 4114
4096         ; 19541              ; 6854                ; 7168            ; 4114                      ; 4114
8192         ; 20210              ; 7130                ; 14336           ; 4116                      ; 4120
16384        ; 21053              ; 7602                ; 28672           ; 4116                      ; 4118
32768        ; 22041              ; 8157                ; 57344           ; 4115                      ; 4125
65536        ; 24464              ; 9066                ; 114688          ; 4116                      ; 4118
131072       ; 29943              ; 11907               ; 229376          ; 4117                      ; 4298
262144       ; 58424              ; 26281               ; 458752          ; 4341                      ; 5267
524288       ; 112066             ; 45359               ; 917504          ; 4495                      ; 13472
1048576      ; 149189             ; 59474               ; 1835008         ; 9884                      ; 23261

A single access using vanilla hashmap is 18ns vs 4ns, or 6ns vs 4ns for a NoHash hashmaps.

a draw call shadows that by many orders of magnitude :)

@eloraiby
Copy link
Contributor Author

@not-fl3 what's going to be?

I have high doubts that any scene will have more than 32k objects held or accessible at any frame and there are more than 100k read access. If it's more, something is going really bad there (major design flaw, bug, ...).

While we can switch to HashMap<usize, Resource, NoHash>, I d still vote for vanilla HashMap. The performance "penalty" doesn't justify the added complexity and for >> 95% of cases, no one needs more than that.

@sokorototo
Copy link

@eloraiby a sort of tangential question, but rather than using HashMap<usize, Resource, NoHash> why not use BTreeMap<usize, Resource>

@not-fl3 not-fl3 merged commit 8a15083 into not-fl3:master May 10, 2024
10 checks passed
@not-fl3
Copy link
Owner

not-fl3 commented May 10, 2024

Sorry for delay!

I got paranoid that real life delays would differ from your benchmark. But nope, HashMaps are very fast indeed, I failed to build any test case with any noticable difference between Vec and HashMap.

Thanks for PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants