Closed rafaqz closed 10 months ago
Thank you @rafaqz ! It's going to take me a bit of time to digest all of this and to begin working on this, but for now I can share some thoughts and questions.
First of all, I really appreciate you taking the time to review this for me. Most of what I've been doing is scouring old forum posts and asking a question here or there myself. There's a lot of rough edges at the moment in the general design of the code that I've been trying to fix, as this is my first engine, and the first project I've created with Julia. I appreciate the patience to take a look at this.
So for the basic demo game, we want something that can run without any user input? I could probably modify the recent platformer game to run without any interaction for a set amount of time.
So for mutable structs, there's a lot of data that changes with format of my data. Would it be more optimal to create a new struct
with the modified data every time it changes? I believe there is decent amount I can make into const
, but I'm not sure what would make more sense.
As for the OOP style, that's something when I first created the engine that I relied on heavily, but I want to get rid of that as much as possible. I may need help at some point figuring out how to rework entity scripts to handle that change. But it is definitely planned.
I will read through those threads heavily when I get a chance too. Like I said, this will take me some time to digest and understand all of what I need to do here.
Again, thank you so much for taking the time to review this for me!
So for the basic demo game, we want something that can run without any user input? I could probably modify the recent platformer game to run without any interaction for a set amount of time.
Yeah I think that would be amazing.
Don't worry about it too much about these things for now, this package looks amazing already and probably making the code and structs type stable will be 90% of the performance gain and should be pretty easy.
Would it be more optimal to create a new struct with the modified data every time it changes?
Making new struct
is often totally free in practice, but its a bit annoying. You can use Setfield.jl
/Accessors.jl
to change fields more easily (in most of my package e.g. DynamicGrids.jl everything works like this cos it has to be fast and go on GPUs - but it might not be imporant or even good to do here)
Having fixed types on the fields will be more important than if they are mutable or immutatble stucts.
Okay, that makes sense, thank you! So something I was having trouble with is that I have some structs that are circular references. The biggest one is the property parent
in all of the components. For example, in Animator.jl. What I can't wrap my head around is being able to reference the types when they are included after them without just including the file twice here. Is there a better way to do this? This is where a lot of my issues with declaring the types occurs.
Ah right that is tricky! How do you put them in the type when you don't have the object at construction and the code loads later!
If you pass the parent object to the constructor you can use a type parameter parent::P
rather than parent::Any
it will be type stable, but you don't need the type
Whenever we create a new component like a sprite or a sound source, we assign the entity as it's parent through the entity itself. So if I modify the code to something like
component.setParent(this::Entity)
would it would work better? Or does it have to be specifically in the constructor to see any benefit?
If its in the constructor then the type is known so it can be fixed and stable, if its in a setter like that it has to be Any
, or you need a way to load the code beforehand so you can can use the type in the struct definition
Ahhh I see. Then I have some refactoring to do. Thanks again! I'll update any progress and any issues here
I've added a test project that can be ran automatically. I was able to finish up work on the platformer. I modified it and added it here: #37 I added the ability to change scenes since the forum post, so I feel like that's an important event to capture as well.
One issue I'm having now is I can't add ProfileView.jl
. I'm getting this error:
Once I get that fixed, I should be able to run it.
Amazing! looks so good.
VSCode julia plugin also has a built in profiler if you use VSCode.
YoWe will also need to set a profiling sample rate lower than the default so its not too big to load.
profile.txt
Here's some results. I'm not really sure what to look for here, but I noticed a lot of overhead in certain areas like iterate(A::Vector{Any, i::Int64})
Here's also the call to @profview_allocs Platformer.run() sample_rate = 1
FYI I'm also posting this here so I have an easily accessible reference point to look at after making the optimizations.
Cool! Do you have the code to run that profile? I can take a look!
Yup, I got it ready for you right here: https://github.com/Kyjor/JulGame.jl/blob/main/test/projects/ProfilingTest/Platformer/src/Platformer.jl
Thanks!
Hey I got it to run!
Man its so cool how easy that is this is exciting. But: it only runs once on my system! (arch linux) the second time the window pops up then disappears instantly? Is there something I can do to reset it? I have to start a new Julia session currently.
So I could do one profile that has a bunch of compilation mixed in (those big towers in the profile)
but it does look like the main cost is type instability in Entity and Collider, and as you say iteration over Array{Any}
that seem to be in Collider
.
Here red means type instability, turning that red band green is where the performance gains will be:
Yellow is allocation but its often related to type instability too.
But: it only runs once on my system! (arch linux) the second time the window pops up then disappears instantly?
Yeah, that's an issue I'll need to work on. I can prioritize it, but I'm not sure how heavy of a lift it will be yet. Hopefully it's a fairly simple fix. I usually start a new session every time, so I've been used to doing it like that for quite a while. I think that will be something I'll be looking into soon, especially if that's what people are expecting when running this.
As for the profiling itself, wow. That's a lot of red lol. I think the fix for that will be what we were talking about earlier, and probably will be requiring significant refactoring. But I will prioritize that, and put it next on my todo list. Thank you!
Ah ok, no worries. I think the good news is you're going to get 100fps on that handheld if you reduce some of that red, and it looks like mostly a few key places repeated.
Cthulhu.jl is a good package to learn for inspecting type instability when you have time. - you might need to @descend
into internals somewhere rather than the whole game run - in DynamicGrids I have a little helper to run small parts of simulations on their own in Cthulu.jl to check the stability without having to dig through the whole stack of code that runs the simulation where type stability doesn't matter..
I think the good news is you're going to get 100fps on that handheld if you reduce some of that red, and it looks like mostly a few key places repeated.
That would be awesome. And I'll definitely look into Cthulu.jl when I get a chance. This helps a ton!
There is also a descend_clicked
function in ProfileView.jl where you can jump into whereever you last clicked the Profile with Cthulhu.jl, probably that's the best way.
using Cthulhu, PofileView
@profview Platformer.run() # This is with just 5 seconds rather than the whole demo!
# Click somewhere in the profile
descend_clicked()
This drops you into a screen like this where you can look around at the types and code generation:
Wow, that is amazing. So for example, on line 281, Base.getproperty(%54, :parent)::Any
means that the parent property is being resolved to Any
? And if stop using get property and use a function to get that value (with it typed in the constructor) it will fix that?
Yeah exactly - but probably getproperty
is actually fine here and mostly not a problem. The struct fields will be 95% of your performance - here I think the parent
field is typed Any
so any function thatt gets it will return Any
(I mean to say the minimum and massive performance fix is just putting fixed types or type parameters on those unstable fields, no need to change too much)
Ohhh okay! I'll just focus on that for now since it will be the largest gain.
Yeah, circle back to mutable/immutable decisions and less getproperty
later if stable struct fields doesn't do enough
Will do, thanks!
When you get a chance, could you check and see if I'm on the right track here? https://github.com/Kyjor/JulGame.jl/commit/95983927071a6fcf333af0a91fe6961c6e3de32e
So I want people to be able to use a constructor to create the new components. I plan on using the current component names as the "public facing" structs, and prefix the current component structs with "Internal". Then, when I create the mutable struct in the engine, we use the "middleman" to create the real one that will be used.
Looks good!
I do the same trick in DynamicGrids, there are StaticX
objects that are actually used in the simulation internals, but mutable unstable X
objects for user. They're switched out before performance matters (also GPU compilation has to be 100% type stable)
Note Vector{Function}
isnt type stable, may need some handling to make that fast. If they have the same retutn types you can force it. (And Function is still better than Any)
It might also make sense to break up components
into separate vectors for each possible type? Some can be empty. Im not sure how that affects the design (maybe they can be together for users and separated out internally)
Oh okay! Glad to hear I'm on the right track. For the function vector, it's basically a function the user can access in scripts that gets triggered when a collision happens. If it helps, there isn't a return value expected from it, so it's essentially a void function.
And breaking up the components sounds like a good idea. It's something I implemented a while back and haven't revisited. I assume there will be some performance gains from not manually type checking as well?
Yeah, it will be much faster if they are separate single type vectors.
FYI, I've refactored all of the components here.. I'll need to do clean up and fix changes that broke the editor, but this is where it's at right now. I'll plan on slowly fixing types in other places moving forward. I also think I'll try to eliminate get property as much as possible at some point in the near future
I'm having tests fail in that pr for only x64 builds. I'm not sure why.
Likely from Int
meaning Int64
on x64 but Int32
on x86. C libs may want Int32?
Yeah I just updated all Int
s to Int32
and that seems to be working. Thanks! I'm going to give it a couple more days before I merge this because I made so many changes that I'm sure I've broken something somewhere. I might go ahead and write some more tests. Thanks so much for helping me with this. There's still a lot more work to do but I think this is a good start for performance.
From discourse... https://discourse.julialang.org/t/im-creating-a-2d-platformer-using-julia/108057/10
Hopefully these help!
Profiling
It would be good to use
ProfileView.jl
on some running code if possible to find the hot spots. Is there a basic demo game we can use for benchmarking that can e.g. run for 10 seconds then stop? Something that showcases a bunch of commonly used things so we can see where the real costs are.The rest of my comments might be a bit random because I'm not sure what the hot paths are exactly, but they should roughly be helpful!
Use concrete types in struct fields as much as possible
I fell like this is the biggest room for improvement. Unstable struct field propagate type instability through all of your code. For example here you have
Array{SomeType}
in a lot of places, but that isn't type-stable because the dimensionality of the array is not known. So the value will be boxed and julia will need to inspect it at runtime to see if its a matrix or vector.Vector{SomeType}
will be stable, as willArray{SomeType,3}
. ObviouslyAny
andArray{Any}
are also not stable. Knowing if its at lease a small union helps a lot::Union{Nothing,Int}
is much faster to access than::Any
because julia has optimizations for "small unions"https://github.com/Kyjor/JulGame.jl/blob/924d08410ae07e1e2179574bad50a1cbc8be0098/src/Component/CircleCollider.jl#L8-L17
Here you use
Integer
which is and abstract type, whenInt
would be concrete and many times faster to access and usehttps://github.com/Kyjor/JulGame.jl/blob/924d08410ae07e1e2179574bad50a1cbc8be0098/src/Component/Sprite.jl#L12
Put types on vectors
This gives a
Vector{Any}
But if you Know exactly what will go in it, you can instead do:
https://github.com/Kyjor/JulGame.jl/blob/924d08410ae07e1e2179574bad50a1cbc8be0098/src/Main.jl#L334
Try to use regular
struct
rather thanmutable struct
in your really hot paths. (this might be only 1% of type instability tho)Julia can optimize the hell out of
struct
butmutable struct
has state the compiler can't track - so they have to exist on the heap as actual constructed objects. Often regularstruct
never actually exist - they get optimized away to just a few values in registers.You can also mark some fields of
mutable struct
withconst
so the compiler knows they don't change.Also a comment more than something I'm at all sure is an actual problem! You are writing some of the most OOP style julia I have ever seen! It makes sense for game dev where things really are object-like. But leaning on
getpropery
might have some performance and compilation costs too (this is an unproven hunch) because probably compiler people are not targeting this coding style, and julia has a few performance gotchas with variables in closures being boxed or unstable. See:https://discourse.julialang.org/t/closures-over-types-are-type-unstable/82337 https://discourse.julialang.org/t/documenting-when-the-closure-bug-15276-binds-and-how-to-avoid-it-for-introductory-users/12642