TechDraw's problems with multi-threading.

[andrecaldas]

It seems that TechDraw makes some heavy computations and because of that, it uses a separate thread.

Now, this is very nice, but the new thread is being passed a bunch of local variables. Of course, the local variables are destroyed as soon as the new thread is launched and the local context is left.

The DrawViewPart::buildGeometryObject is such a function. First, notice that the function is used here. A reference to "localShape" is passed to "buildGeometryObject". Usually, this is not a problem...

... but, because "buildGeometryObject" passes this localObject to a new thread and returns, the localObject is promptly destructed. This is wrong.

Now, it seems to me that as a remedy to this object destruction, the developer decided to store some stuff as member variables, like "m_ScaledShape" and "m_PreparedShape". Also, the QFutureWatcher variables are also used to connect a signal. This happens, for example, in DrawViewSection.

But the fact is that although storing as member functions reduces the chance of accessing a destructed object, this is just a matter of luck. If the calculations take a little longer it does happen that the DrawViewSection object is destructed before the processing is finished!

The local variable destruction is very easy to handle. One just needs to put it in a closure (lambda function). Preferably using a smart pointer, if copy is expensive. (I have discovered that unique_ptr is a little hard to use in this situation, because the lambda is copied to a Function).

However, to solve the problem of the DrawViewSection is destructed before the parallel processing is finished, it would have to be managed by a shared_ptr.

[wandererfan]

What problems have you encountered?

m_scaledShape and m_preparedShape are not there because of threading. They are intermediate results that are saved in case this view is used as a base for additional views.


"it does happen that the DrawViewSection object is destructed before the processing is finished!"
Does it? The destructors of DrawViewPart, DrawViewSection and DrawViewDetail wait for running threads to complete before destroying the object.

I sounds like you are advocating implementing the change described in this comment:
// TODO: we should give the thread its own copy of the shape because the passed one will be
// destroyed when the call stack we followed to get here unwinds and the thread could still be
// running.
// Should we pass a smart pointer instead of const& ??

By all means make a PR!

[andrecaldas]

What problems have you encountered?

1. In DrawViewPart::makeGeometryForShape, a localShape variable is created.
2. A reference to the local variable is passed to DrawViewPart::buildGeometryObject.
3. The buildGeometryObject starts a new thread and passes the reference to localShape.
4. Then it returns without waiting (waiting would be pointless, of course).
5. Then, makeGeometryForShape returns without waiting (naturally).
6. The scope for the localShape is destructed.
7. Actually, the result of getProjectionCS() suffers from the same problem, as it is passed by reference to buildGeometryObject as well. I mean... as bad.

This can be solved through the use of unique_ptr instead of reference, as argument to buildGeometryObject.

I think this is one PR. I can do that.

m_scaledShape and m_preparedShape are not there because of threading. They are intermediate results that are saved in case this view is used as a base for additional views.

I am sorry for the wrong statement.
It is very nice to talk to a veteran developer and have prompt answers.

"it does happen that the DrawViewSection object is destructed before the processing is finished!"
Does it? The destructors of DrawViewPart, DrawViewSection and DrawViewDetail wait for running threads to complete before destroying the object.

I can think of some situations where this could be a problem.
(Disclaimer: I don't really know what I am talking about! )

1. What happens if it is not finished processing and you call it again?
2. If the processing is very, very heavy... and if you destruct the object... Will the application freeze, waiting for the thread to terminate?

If FreeCAD used shared_ptr to manage DocumentObject, then most of the complications when dealing with this kind of thing would simply vanish away.

I sounds like you are advocating implementing the change described in this comment:
// TODO: we should give the thread its own copy of the shape because the passed one will be
// destroyed when the call stack we followed to get here unwinds and the thread could still be
// running.
// Should we pass a smart pointer instead of const& ??

By all means make a PR!

Yes. This is one (very important) thing. I wonder why it does not crash...

Can you answer any of those following questions? Or, maybe, comment on alternatives...

1. If I already have a shape in a local variable (ls), the recommended way to put it in a unique_ptr would be "std::make_unique<TopoDS_Shape>(std::move(ls))"?

2. Is "gp_Ax2" very cheap to copy?

3. What is the difference between those two?
3.1 TopoDS_Shape c{shape.copy()};
3.2 TopoDS_Shape c{shape};

[wmayer]

A reference to "localShape" is passed to "buildGeometryObject". Usually, this is not a problem...

A TopoDS_Shape class acts like boost's intrusive pointer and its perfectly possible to share it with different threads. Passing the local TopoDS_Shape to the QtConcurrent function forwards it to the executed function where the reference counter is incremented and later decremented. Because at this point the reference counter won't be 0 the underlying data won't be destroyed.

When looking at the OCC code then the whole reference counting mechanism comes down to the class Standard_Transient. Looking at its implementation shows that an atomic int is used for incrementing & decrementing the reference counter.

[andrecaldas]

A reference to "localShape" is passed to "buildGeometryObject". Usually, this is not a problem...

A TopoDS_Shape class acts like boost's intrusive pointer and its perfectly possible to share it with different threads. Passing the local TopoDS_Shape to the QtConcurrent function forwards it to the executed function where the reference counter is incremented and later decremented. Because at this point the reference counter won't be 0 the underlying data won't be destroyed.

When looking at the OCC code then the whole reference counting mechanism comes down to the class Standard_Transient. Looking at its implementation shows that an atomic int is used for incrementing & decrementing the reference counter.

I don't understand much about the QtConcurrent::run. But it seems to me that this reference counter is never incremented in this process.

[andrecaldas]

When looking at the OCC code then the whole reference counting mechanism comes down to the class Standard_Transient. Looking at its implementation shows that an atomic int is used for incrementing & decrementing the reference counter.

Does that mean that
TopoDS_Shape shape1{shape2}
is very fast to do because nothing new is produced... only the reference counter is incremented?

If this is so, then something like passing a lambda (by copy) to QtConcurrent::run shall do the trick.

[wmayer]

I don't understand much about the QtConcurrent::run. But it seems to me that this reference counter is never incremented in this process.

It's not dome by the QtConcurrent stuff but by creating a TopoDS_Shape instance.

Does that mean that
TopoDS_Shape shape1{shape2}
is very fast to do because nothing new is produced... only the reference counter is incremented?

Yes.

[andrecaldas]

I don't understand much about the QtConcurrent::run. But it seems to me that this reference counter is never incremented in this process.

It's not dome by the QtConcurrent stuff but by creating a TopoDS_Shape instance.

Yes... but we do not create any TopoDS_Shape instance besides the local one that gets destructed. So, my only hope was that QtConcurrent::run did. Since the function arguments are all reference to this local variable, it will get destructed. Unless some copy is created in the process.

Does that mean that
TopoDS_Shape shape1{shape2}
is very fast to do because nothing new is produced... only the reference counter is incremented?

Yes.

This is very good to know!
Sorry to change the subject... but does this mean that shape2 can change if we change shape1? If it does, is this the reason we have methods to copy the shapes?

[wandererfan]

but does this mean that shape2 can change if we change shape1?

Maybe. Multiple TopoDS_Shape(location and orientation) can share the same TopoDS_TShape(the topological description). If something changes the TopoDS_TShape, then it will change for all the TopoDS_Shapes that share it.

If you use TopoDS_Shape1 = TopoDS_Shape2, then they will share the TShape. If you make a real copy, you get a new TShape.

[andrecaldas]

@wandererfan: Thank you!

I am waiting for my patch to compile... my computer is quite slow.
Now, I am passing a closure (lambda function) to QtConcurrent::run. I think that there is no need to wait for this "future" at destruction, any more.

Shall I remove this wait, too?

PS: A nice side effect is that calling a function in QtConcurrent::run is just the same in Qt5 and Qt6.