Improving performance by adding multithreading support #221
Conversation
| var stack = new Stack<IEnumerator>(); | ||
| stack.Push(loader.LoadScene(isMultithreaded: true)); | ||
| // HACK: Force the coroutine to run synchronously in the editor | ||
| var stack = new Stack<IEnumerator>(); |
There was a problem hiding this comment.
formatting seems to be off here. Make sure you're using tabs
There was a problem hiding this comment.
Actually I'm using tabs here, but the rest of this file seems to be using spaces. Should I change the tabs to spaces?
There was a problem hiding this comment.
Probably, I think the majority of the project is using tabs.
There was a problem hiding this comment.
I think in this case whichever keeps the formatting changes to a minimum is best to see the diff.
| @@ -19,7 +19,7 @@ internal struct GLBHeader | |||
| public uint FileLength { get; set; } | |||
| } | |||
|
|
|||
| public static GLTFRoot ParseJson(Stream stream, long startPosition = 0) | |||
| public static void ParseJson(Stream stream, ref GLTFRoot gltfRoot, long startPosition = 0) | |||
There was a problem hiding this comment.
Should probably be out instead of ref
There was a problem hiding this comment.
I was starting to fix it but then noticed you had done it. Thanks!
| @@ -57,7 +59,8 @@ public void LoadGLBFromStream() | |||
| { | |||
| Assert.IsTrue(File.Exists(GLB_PATH)); | |||
| FileStream gltfStream = File.OpenRead(GLB_PATH); | |||
| GLTFRoot gltfRoot = GLTFParser.ParseJson(gltfStream); | |||
| GLTFRoot gltfRoot =null; | |||
| @@ -17,11 +17,14 @@ public class WebRequestLoader : ILoader | |||
| { | |||
| public Stream LoadedStream { get; private set; } | |||
|
|
|||
| public bool HasSyncLoadMethod { get; private set; } | |||
There was a problem hiding this comment.
I noticed you have started CI. Are you OK with leaving it as it is?
There was a problem hiding this comment.
Yeah it's not supported in this version of .NET
| @@ -15,9 +15,12 @@ public class FileLoader : ILoader | |||
| private string _rootDirectoryPath; | |||
| public Stream LoadedStream { get; private set; } | |||
|
|
|||
| public bool HasSyncLoadMethod { get; private set; } | |||
|
@blgrossMS It seems like the CI is failing due to some configuration problems... Could you maybe take a look at it? |
|
Yup I just have to merge master, I fixed it there |
|
Cool, let me know if you need anything from me. |
Hi there,
As some of you may have experienced, currently the UnityGLTF library blocks the main thread when importing models containing complex geometry and/or textures. Although the coroutine approach is used in the project, sometimes multithreading may be a better solution as it provides a non-blocking behavior as well as better performance (in some situations).
Implementation
I noticed a parameter
isMultithreadedinGLTFSceneImporter.LoadScenethat is never actually used, changed it to a public field and used it as the switch. I put some presumably time-consuming work on other threads (rather than the main thread), which includesLoadStream,ParseJson,stream.Read,buildMeshAttributesThread, andGLTFHelpers.BuildMeshAttributes.I have to change the
ILoaderinterface to add support for non-coroutine basedLoadStreamSyncmethod, as coroutine methods cannot be easily executed on threads and doesn't really make sense for created threads anyway. I also have to change the signature ofParseJsoninGLTFParser.csto make it thread-friendly.Performance
HUGE frame rate increase for certain models containing complex geometry and/or textures. I observed up to 500% fps increase for this model on a Windows desktop computer. In general, it should increase your fps and/or decrease the main thread freezing time. It should not make any tangible negative impact to the loading time, if no faster loading is observed.
Comparison Chart
Model 1: Complex link
Model 2: Complex link
Model 3: Simple link
Model 4: Simple link
Test performed 3 times for each model and each importer (original vs proposed).
Test machine: Windows 10 64 bit, i7 7700K, GTX 1070, SSD.
Limitations
As mentioned in #9,
Texture2D.LoadImageis slow and there's no obvious way around it. Same withTexture2D.Apply.Unity Job System?
The recently introduced Unity Job System seems good, but currently it is still limited in a number of ways. Also, I don't believe it is very backward compatible.
Reference: #216