CoownedPointer not thread safe

[Milosz Tanski]

It there a reason that ResultOrVoid (or ResultOrOwned) do not use std::shared_ptr instead of CoownedPointer? CoownedPointer is not thread safe and the way it's implemented it makes ResultOr* also not thread safe. Even with what like obvious synchronization this is a problem.

Take this example

Thread 1.

setter(std::promise<ResultOrOwned<MyValue>>& result)

{

ResultOrOwned<MyValue> my_error = supersonic::Failure(new Exception(...)); 

result.set_value(result);

}

Thread2.

getter(std::future<ResultOrOwned<MyValue>>& input)

{

   ResultOrOwned<MyValue> my_value = input.get();

}

I was experiencing this issue from time to time on my application either crashing in the CoownedPointer::~CoownedPointer or hanging CoownedPointer::operator=. CoownedPointer uses a circular linked list and doesn't take any kind of threading into account. My future example could be just as well replaced with global ResultOrOwned<MyValue> protected by a mutex. Since the destructor for any instance of CoownedPointer is modifying a linked list will eventually crash.

Does anything in the supersonic rely on the semantics FailureOr* being implemented with CoownedPointer or can I safely use std::shared_ptr instead to implement it.

[ptab]

1. Supersonic in general is single threaded. The only multithreaded operation is ParallelUnionAll. So we didn't faced the multithreaded access problem. 

2. AFAIK nothing depends on internal's of ResultOr implementation. CoownedPointer seemed to be generally faster as it doesn't require potential synchronization/atomic access/ref-counting. 

3. I can accept CL to the main branch if it shows that there is no significant performance regression on intel's architectures. 

Piotr

[Milosz Tanski]

Piotr,

I'm guessing ParallelUnionAll is internal, since my codebase does not
have that operation. We're using multiple threads when it comes to
partial re-aggregating results from networked nodes. There's an
operation that can read from a Go like channel that's being feed by
multiple network threads.

I have changed CoownedPointer pointer to use std::atomics, the test
work, our application death test works. I can probably squeeze the
maximum amount of performance out of it if I investigate the use of
relaxed atomics, I haven't investigated that yet but I'll keep working
on it.

Having said that is there an external facing benchmark that you guys
have that I can use to test it. In our benchmarks it doesn't seam to
impact the speed at all but there's a lot of other things going on
like index scanning, decompression, disk io, network traffic. It's
possible in a more synthetic test there might be some impact.

I've also attached my changes as patch.

- Milosz

> --
> You received this message because you are subscribed to the Google Groups
> "Supersonic Query Engine Group" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to supersonic-query-...@googlegroups.com.
> To post to this group, send an email to
> supersonic-...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.



--
Milosz Tanski
CTO
10 East 53rd Street, 37th floor
New York, NY 10022

p: 646-253-9055
e: mil...@adfin.com