cpp: consider thread_local isl::ctx object

[ftynse]

Essentially, isl::ctx is used as a "global" state for thread-safety reasons. We may want to declare one thread_local isl::ctx and use it implicitly in all functions that require a context.

[aisoard]

How much clutter is remaining inside the isl_ctx one all object have been freed? If that does not create any memory leak that seems fine. Note that this means that an isl object from a thread cannot be transferred to an other thread.

[tobiasgrosser]

We could add a method to isl::ctx called:

static isl::ctx getThreadDefaultCtx()

which is a singleton and then expand the code generator to generate two versions of each constructor. One with an isl_ctx argument and one which uses this method to get an isl::ctx.

We probably need one or two more methods alla freeThreadDefaultCtx and setThreadDefaultCtx

Tobias

[ftynse]

Being able to free the thread-default ctx looks like an overly easy way to shoot yourself in the leg. Maybe it's better to be able to swap it with a different, non-null context. AFAIK, you cannot use the context (or objects referring to it) in multiple threads simultaneously. If you move a context into a different thread, objects that reference it should no longer be used in the previous thread. How we handle object movement between threads seems to be the key design consideration here.

…

On 06/08/17 13:06, Tobias Grosser wrote: We could add a method to isl::ctx called: static isl::ctx getThreadDefaultCtx() which is a singleton and then expand the code generator to generate two versions of each constructor. One with an isl_ctx argument and one which uses this method to get an isl::ctx. We probably need one or two more methods alla freeThreadDefaultCtx and setThreadDefaultCtx Tobias — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABcTawpYa94VA4j1MuxZv9NS2JpKfMt2ks5sVZ4igaJpZM4OuQR5>.

[tobiasgrosser]

Indeed, maybe freeing the default context is not a good idea. We could e.g. assume there is always a single default context. Resetting (freeing and creating a new one) makes sense however, to check for memory leaks, e.g.. I just thought about it a little and wanted to share my thoughts as code. Feel free to advance them (or do something entirely different).

[aisoard]

The only safe way I see to transfer object between threads is to have a temporary context in which the source thread transfer the object. Transfer the temporary context. Then the destination thread transfer the object to it's context.

Quite cumbersome.

[aisoard]

If getThreadDefaultCtx reallocate a context if it has been freed, that should be fine, no? Swapping contexts is fine to me too.

Do we add a: isFromThreadDefaultCtx method to all objects too?

[tobiasgrosser]

Swapping context would also be fine for me.

I would not add an extra method to each object. However, we can expose the get_ctx() methods and add a function isThreadDefaultCtx() to isl::ctx, I assume.

I am not sure we actually want to solve the inter thread transfer issue in this discussion. For me the default context is a way to handle the common case of single thread execution. I believe inter context transfer functionality should be rather orthogonal.

[ftynse]

The inter-thread management comes into play as soon as we say the default context is thread_local. We can just make it globally shared, wrapped in a singleton and say it is not thread-safe.

Well, there is no way to move an object into a different context in the C API... The ugly way I would do it is to serialize the object to a string, pass a string between threads and reconstruct the object in the other side's context. The ugliness can be compensated by the fact that this would also work in a distributed environment.

Using a special context that could be passed between threads is not always an option since the objects allocated within one context cannot be used in a function with objects from a different context (maybe add an assert in C++ interface for this?).

For the same reason, implicitly reallocating the default context seems dangerous. You may end up mixing previously allocated objects with news ones from new context in function calls, which is forbidden.

[aisoard]

There was discussions with Sven about the fact that C string serialization does not always produce valid output that can be parsed back (if you name any id: "lb" or "min" or "exists"). Also, it loses any "user" data and associated destructor.

As for using objects from different contexts, this will be a problem indeed, and we could either check that does not happen (with an assert or an actual test), or have automatic context transfer (but I think that is not possible even if we had a way to transfer objects between contexts).

That being said, I don't think there is a risk with implicitly reallocating the default context (when it is non existent anymore) as there cannot be any remaining objects from any previously freed context still alive.

[ftynse]

On 09/08/17 14:48, Alexandre Isoard wrote: There was discussions with Sven about the fact that C string serialization does not always produce valid output that can be parsed back (if you name any id: "lb" or "min" or "exists").

This probably means isl needs a full parser instead of what it uses now.

Also, it loses any "user" data and associated destructor.

That's a good point.

As for using objects from different contexts, this will be a problem indeed, and we could either check that does not happen (with an assert or an actual test), or have automatic context transfer (but I think that is not possible even if we had a way to transfer objects between contexts). That being said, I don't think there is a risk with implicitly reallocating the default context (when it is non existent anymore) as there cannot be any remaining objects from any previously freed context still alive.

There are _not supposed_ to be any live objects referring to that context. But we cannot clean them up magically, and that's the potential danger. ```c++ auto s = isl::set("{:}"); // allocate using current default context isl::move_default_context_to_other_thread(); // reallocates the default context; s.toString(); // context is not in this thread anymore, but s is! auto s2 = isl::set("{:}"); // allocate using the newly re-allocated default context s2 = s2.unite(s); // using different contexts! even worse if the other thread decided to free ctx, we can get an tricky error due to undefined behavior. ``` So the library user should know when the default context was reallocated, which kind of contradicts the idea of default context making life easier. IMHO, if they want movable objects, they should just explicitly create a movable context.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABcTawioBjUAIdwDsoNFSAvFjG80FWBvks5sWaqcgaJpZM4OuQR5>.

[aisoard]

This probably means isl needs a full parser instead of what it uses now.

I am not sure that would help (but would probably clean-up the parser). This is the printing stage which is faulty, it should en-quote "conflicting" identifiers.

So the library user should know when the default context was
reallocated, which kind of contradicts the idea of default context
making life easier. IMHO, if they want movable objects, they should
just explicitly create a movable context.

So, would you prefer the default context to be non-movable (but still reset-able)?