Search Unity

jobs in coroutine = poor performance?

Discussion in 'Data Oriented Technology Stack' started by laurentlavigne, Jan 19, 2018.

  1. laurentlavigne

    laurentlavigne

    Joined:
    Aug 16, 2012
    Posts:
    2,020
    I'm starting to wonder if jobs are usable in coroutine, have a look at this profiler, that's a LOT in the main thread.

    Unity_2018-01-19_14-24-39.png

    Code (CSharp):
    1. using System.Collections;
    2. using System.Collections.Generic;
    3. using UnityEngine;
    4. using Unity.Jobs;
    5. using Unity.Collections;
    6.  
    7. public class JobSimple : MonoBehaviour {
    8.  
    9.     struct FillTheArray : IJobParallelFor
    10.     {
    11.         public NativeArray<float> output;
    12.  
    13.         public void Execute(int i)
    14.         {
    15.             output[i] = Mathf.Log10( i);
    16.         }
    17.     }
    18.  
    19.     struct CalculateThings : IJobParallelFor
    20.     {
    21.         [ReadOnly]
    22.         public NativeArray<float> input;
    23.  
    24.         public NativeArray<float> output;
    25.  
    26.         public void Execute(int i)
    27.         {
    28.             output[i] = Mathf.Sin( input[i]);
    29.         }
    30.     }
    31.  
    32.     void OnEnable()
    33.     {
    34.         StartCoroutine(JobCompute());
    35.     }
    36.  
    37.     public int computeSize=1000000, batchSize = 100;
    38.     NativeArray<float> input, output;
    39.     JobHandle handleCalculate;
    40.     IEnumerator JobCompute()
    41.     {
    42.         while (true)
    43.         {
    44.             input = new NativeArray<float>(computeSize, Allocator.Persistent);
    45.             output = new NativeArray<float>(computeSize, Allocator.Persistent);
    46.  
    47.             var time = Time.time;
    48.  
    49.             var jobFiller = new FillTheArray()
    50.             {
    51.                 output = input
    52.             };
    53.             var handleFiller = jobFiller.Schedule(computeSize, batchSize);
    54.  
    55.             var job = new CalculateThings()
    56.             {
    57.                 input = input,
    58.                 output = output
    59.             };
    60.             handleCalculate = job.Schedule(input.Length, batchSize, handleFiller);
    61.             yield return new WaitWhile(() => handleCalculate.IsCompleted);
    62.             handleCalculate.Complete();
    63.             Debug.Log(Time.time - time);
    64.             input.Dispose();
    65.             output.Dispose();
    66.         }
    67.     }
    68.  
    69.     void OnDisable()
    70.     {
    71.         handleCalculate.Complete();
    72.         input.Dispose();
    73.         output.Dispose();
    74.     }
    75. }
     
  2. Tautvydas-Zilys

    Tautvydas-Zilys

    Unity Technologies

    Joined:
    Jul 25, 2013
    Posts:
    6,452
    Code (csharp):
    1. yield return new WaitWhile(() => handleCalculate.IsCompleted);
    Looks like the condition is inverted? Shouldn't it be this instead?

    Code (csharp):
    1. yield return new WaitWhile(() => !handleCalculate.IsCompleted);
     
  3. laurentlavigne

    laurentlavigne

    Joined:
    Aug 16, 2012
    Posts:
    2,020
    HAHAHAHHA - jeez thanks!
     
  4. laurentlavigne

    laurentlavigne

    Joined:
    Aug 16, 2012
    Posts:
    2,020
    err... when the thread is done I see some spikes here, any idea?

    Unity_2018-01-19_15-56-54.png
     
  5. Tautvydas-Zilys

    Tautvydas-Zilys

    Unity Technologies

    Joined:
    Jul 25, 2013
    Posts:
    6,452
    Looks like it's spending time in your function. You could try putting in some profiler blocks.
     
  6. laurentlavigne

    laurentlavigne

    Joined:
    Aug 16, 2012
    Posts:
    2,020
    I ran a deep profiler because with profiler blocks don't show up in the profiler. Might be a coroutine thing, maybe time too short to show up?

    it's CustomYieldInstruction that's taking time which is handleCalculate.IsCompleted
     
  7. CDF

    CDF

    Joined:
    Sep 14, 2013
    Posts:
    771
    maybe try

    Code (CSharp):
    1. while (!handleCalculate.IsCompleted) {
    2.  
    3.     yield return null;
    4. }    
     
  8. Joachim_Ante

    Joachim_Ante

    Unity Technologies

    Joined:
    Mar 16, 2005
    Posts:
    4,671
    First a general comment...
    Why are you using a coroutine for this specifically?
    It seems to me that:
    * Complicates the code
    * Makes intent less clear & readable
    * allocates GC memory
    an Update() method with
    if (!jobHandle.IsCompleted())
    return;

    Will do just fine... keep it simple... (Rant over...)


    For the most likely cause of the spike when the job completes, you are scheduling a new iteration.
    For this you are allocating a two new NativeArray with 1000000 elements. This means we
    1. allocate 4mb of memory
    2. clear all that memory with zeros by default

    Allocating 4mb of memory is relatively cheap (compared to your 7ms spike - the size of the allocated memory generally doesn't make it significantly more expensive)

    However clearing 4mb of memory to zeros on my laptop takes around 1-2 ms for each array. (My laptop has a bandwidth per core of around 3GB/second) Fortunately there is an option on NativeArray where you can tell it to leave the memory uninitialized:
    See NativeArrayOptions on the constructor. So essentially you allocate on main thread and then fill the memory on job. Skipping the cost of 2 on the main thread.

    Naturally if you are not carefully and don't gurantee to write to every element before reading them in your code you get randomly initialized memory.

    Its not clear what your code is trying to achieve so potentially keeping the native array around instead of allocating it on each iteration would be desirable?
     
    starikcetin, mh114, Ostwind and 4 others like this.
  9. Joachim_Ante

    Joachim_Ante

    Unity Technologies

    Joined:
    Mar 16, 2005
    Posts:
    4,671
    Btw. did you know about
    Profiler.Begin("Some block");
    ...
    Profiler.End();

    Usually when i try to find out why something is slow, i put those around all the different places that might cause the performance hit. With coroutines you need to be careful not to cross yield boundaries with begin / end.
     
  10. Krajca

    Krajca

    Joined:
    May 6, 2014
    Posts:
    88
    Can someone tell me why everyone is using Allocator.Persistent instead of Allocator.Temp, like in video that shows us jobsystem?
     
  11. Peter77

    Peter77

    Joined:
    Jun 12, 2013
    Posts:
    4,036
    My best guess would be "lack of documentation", which I expect to change, once UT officially announces the release of its new tech and provides all the documentation one needs to make the best use of it.
     
  12. laurentlavigne

    laurentlavigne

    Joined:
    Aug 16, 2012
    Posts:
    2,020
    Because I'm lazy.
    This example is silly but in the real thing I set up the jobs, schedule them then wait on the job to complete and after that I work on the result. For this sort of linear progression that yield to a result, coroutines are just easier.
    In an update I would have to break it down in two if blocks, one for jobRunning==false (because a handle doesn't have progress), the other one for handle.isCompleted.

    Could we get a status on the handle? I'm thinking handle.progress, To mirror AsyncOperation. And even if calculating progress isn't trivial (well... for the JobFor it is) jumping from 0 to 1 is good enough.

    Also it would be great if you could rename handle.isCompleted to handle.isDone so it matches AsyncOperation.isDone, because I notice that the Unity API has many different words for the same thing and it is not ideal. (request.done I am looking at you)

    I got rid of the allocation and now it is smooth - I have questions dependencies so I'll open a new thread.

    NativeArrayOptions.None... brilliant!
     
  13. Joachim_Ante

    Joachim_Ante

    Unity Technologies

    Joined:
    Mar 16, 2005
    Posts:
    4,671
    True story... not sure how NativeArrayOptions.None happened, but API has been fixed for beta 5.
     
    hippocoder likes this.
  14. hippocoder

    hippocoder

    Digital Ape Moderator

    Joined:
    Apr 11, 2010
    Posts:
    25,485
    I am not sure I can agree. Done is slightly a different meaning to Completed. There is also the tense to worry about like wasCompleted vs isComplete and wasDone vs isDone.

    I would probably recommend using .done or .finished instead.

    Every API has some broken English in it though, not just Unity :)

    So it would make sense to use either .done or .finished property for everything if everyone is looking for consistency across different parts of the API and doesn't mind so much about chronological accuracy. I'm sure if we went down this rabbit hole then the work would never be isDone or isCompleted.

    I'm done! All finished!