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

[BurstCompile] for all existing jobs #2421

Conversation

andrew-raphael-lukasik
Copy link
Contributor

@andrew-raphael-lukasik andrew-raphael-lukasik commented Sep 6, 2022

Hi! This PR is part of #2416 saga.

What

Burst compilation enabled for all existing jobs. That's it, nothing else.

Why

Burst is not a magical wand necessarily. It is powerful when there is a lot of math calculations but it won't fix a memory access-bound code (L2 cache misses).

Results

It sped up these existing jobs by around a factor of 10x and nearly *halves* my loading time for exteriors (20-40% reduction)

  • before: 5 seconds total of work (left), after [BurstCompile]: 0.4 second total of work (right)

I mean... optimization is never this easy, so merge while PR is still hot :T

@Interkarma
Copy link
Owner

Nice one! This one should be fine to just merge. I'll aim to get this into next release for general testing. Thank you. :)

@@ -219,7 +222,7 @@ public void Execute(int index)
}
// Beach texture
// Adds a little +/- randomness to threshold so beach line isn't too regular
if (height <= beachElevation + (JobRand.Next(-15000000, 15000000) / 10000000f))
if (height <= beachElevation + Unity.Mathematics.Random.CreateFromIndex((uint)index).NextFloat(-1.5f, 1.5f))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, could you explain more about this change please? The JobRand helper class providing basic thread safe access to system random was the best solution I could find when migrating the original code into Jobs.

Why are you changing it here? Is the helper class not compatible with burst?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you changing it here? Is the helper class not compatible with burst?

Yes sir. System.Random, being a managed type, is not compatible with Burst so this forced me to look for a substitute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before ending with this particular solution I dig into this and run some tests to figure out how to generate random numbers within an IJobParallelFor job that can run on many cores at once and without a single memory allocation to maintain a seed (like System.Random does, I presume). You can look through these numerical results in my post here although those are not super interesting.

My conclusion was that Random.CreateFromIndex(index) is good enough to generate non-random sequence of numbers.

@Interkarma Interkarma merged commit 3904855 into Interkarma:master Sep 22, 2022
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