Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi-signal emission in case of fused functor data source callbacks #293

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

meyerj
Copy link
Member

@meyerj meyerj commented May 16, 2019

This is a bunch of patches in toolchain-2.9 that have not yet been on any pull request so far:

From 619fce2 (by @psoetens):

signals: fix multi-signal emission in case of fused functor data source callbacks

When an operation was called twice in a row, and a signal callback was installed
using datasource semantics (with produceSignal() ), both callbacks would most
likely receive twice the same data, being equal to the last operation call's values,
since before the callbacks are dispatched, the data sources are already filled in.
The last write wins and the first call-back sees the data of the last.

This patch fixes that by defining a data_store_type in create_sequence with a
store() and a load() function. The fused signal callback can now first store
the args of the operation, then dispatch and retrieve them later on in the
callback function.

A later patch removed the new DataStore<T> class defined in CreateSequence.hpp again. Instead we use AStore<T> already defined in BindStorage.hpp involved in operation calls.

The last patch, ca0c42b, removes a using namespace std directive in FusedFunctorDataSource.hpp, probably a leftover from debugging 619fce2. This commit was cherry-picked directly to toolchain-2.9 as 23f826a.

Peter Soetens and others added 6 commits May 16, 2019 20:20
…ce callbacks

When an operation was called twice in a row, and a signal callback was installed
using datasource semantics (with produceSignal() ), both callbacks would most
likely receive twice the same data, being equal to the last operation call's values,
since before the callbacks are dispatched, the data sources are already filled in.
The last write wins and the first call-back sees the data of the last.

This patch fixes that by defining a data_store_type in create_sequence with a
store() and a load() function. The fused signal callback can now first store
the args of the operation, then dispatch and retrieve them later on in the
callback function.

Signed-off-by: Peter Soetens <peter@thesourceworks.com>
…d TypeInfo usage for conversion

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
…> and internal::DataStore<T>

These types are not used in a const context and therefore const correctness is not an issue.
This patch partially reverts ccf678c.

For consistency with the get() accessors the return type of internal::AStore<T> and internal::DataStore<T>
has been changed to T&. Unfortunately this change alone does not fix the compile error reported in
#196.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
…ady defined in BindStorage.hpp

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
…orage.hpp

This was required without the fix in e0dd34b, but can be simplified now that the
implicit conversion operator directly returns a reference.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
Probably a leftover from debugging e7ede9f.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
@meyerj meyerj added this to the 2.9 milestone May 16, 2019
T arg;
AStore() : arg() {}
AStore(T t) : arg(t) {}
AStore(AStore const& o) : arg(o.arg) {}

T& get() { return arg; }
void operator()(T a) { arg = a; }
operator T() { return arg;}
operator T&() { return arg; }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psoetens Was there a good reason why the operator did not return a reference in the original code? I think not: The object is already copied at least once when creating the AStore<T> instance or via operator()(T a). So no reason to copy it again to read it. For references we use the specialization below instead.

Even without the reference return type the object was not copied for the actual operation call because before this patch .get() was used to access it in BindStorageImpl<ToBind> below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that I tested this code with objects which did a cout in a copy constructor to count the number of copies made, and then introduced references to reduce the number of copies. That said, I don't remember if there was a formal place in the architecture where a copy 'must' be made, but the AStore seems to be the best place to do it, if required. AStore was meant to mimic the classical C++ function argument behavior.

#endif
if (mmeth)
retv.exec( boost::bind(mmeth, boost::ref(a1.get()) ) );
retv.exec( boost::bind(mmeth, a1 ) );
Copy link
Member Author

@meyerj meyerj May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be checked: Does boost::bind actually copy the AStore<T> instance and indirectly the argument to the functor object or does it invoke operator T&() here and only stores a reference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like boost::bind is always copying the arguments itself, and only invokes the conversion operator at the time the functor is actually called: https://stackoverflow.com/questions/11255144/why-does-boostbind-store-arguments-of-the-type-passed-in-rather-than-of-the-ty

In this case the patch in a5857b2 (this PR) and a2ccd79 (in toolchain-2.9) is actually wrong. The .get() can be removed, but the boost::ref() call should have been kept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants