Submitting my feedback in chunks. This is the first one.

I think besides my comments the code looks good.

I also measured a 30% improvement on a release build in the elephant scene on my computer. (1.1 seconds to load -> 0.75 seconds to load).

However I noticed that when looking at the work distribution that the convertion to bmesh takes half of the time:

The first three seconds are spent mostly just doing the BM_mesh_bm_from_me convertion.
But we can also see that the rest of the loading operation now seems to thread quite nicely. It manages to saturate the threads so most are continuously working on tasks.

So I think that we should fix the TODO you have written in the code and thread the mesh conversion as well.

I know you had some issues with this, but I don't remember what they were.
However from looking at the code, I think you could perhaps reuse the same lineart_geometry_load_assign_thread logic for BMesh convertion as well.

So instead of converting the meshes in the DEG_OBJECT_ITER_BEGIN loop, you can queue them up and do it in parallel after all objects have been dealt with.

The only place currently in that loop where you use BMesh data is for the lineart_geometry_load_assign_thread call. And for that, you can easily replace the bm->totface with me->totface (as the face count of the Mesh and BMesh data is the same).

In D11288#289425, @Sebastian Parborg (zeddb) wrote:

I think besides my comments the code looks good.

I also measured a 30% improvement on a release build in the elephant scene on my computer. (1.1 seconds to load -> 0.75 seconds to load).

However I noticed that when looking at the work distribution that the convertion to bmesh takes half of the time:

The first three seconds are spent mostly just doing the BM_mesh_bm_from_me convertion.
But we can also see that the rest of the loading operation now seems to thread quite nicely. It manages to saturate the threads so most are continuously working on tasks.

So I think that we should fix the TODO you have written in the code and thread the mesh conversion as well.

I know you had some issues with this, but I don't remember what they were.
However from looking at the code, I think you could perhaps reuse the same lineart_geometry_load_assign_thread logic for BMesh convertion as well.

So instead of converting the meshes in the DEG_OBJECT_ITER_BEGIN loop, you can queue them up and do it in parallel after all objects have been dealt with.

The only place currently in that loop where you use BMesh data is for the lineart_geometry_load_assign_thread call. And for that, you can easily replace the bm->totface with me->totface (as the face count of the Mesh and BMesh data is the same).

Right... the conversion needs to be in parallel for best performance, but the problem with that is that the me is only available in the loop, and can't be accessed later. Maybe I can copy a me there, that could be faster than converting to bm but it's gonna take a lot more memory.

The best best way to do this is to have it in a deg evaluation stage, where there's a special tag for evaluating line art data, that way everything is properly threaded and we don't need to worry about managing those ourselves, during this loading call we simply grab those data.

In D11288#290653, @YimingWu (NicksBest) wrote:

Fixed stuff in comment.

The double pointer list thing I'll need to fix in a later patch because it affects a lot of stuff.

The issues brought up in the review should be fixed before it is committed.
For somethings it might be ok to do later, but I don't think this is one of them.

Otherwise we will accept a lot of crappy code with the promise "I'll fix it later".
There is a very high risk of the "fixing it later" never happening.

Just to attach the latest benchmark here: