Navigation Menu

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

Fixing action move tests #1473

Merged
merged 2 commits into from Apr 22, 2015
Merged

Fixing action move tests #1473

merged 2 commits into from Apr 22, 2015

Conversation

hkaiser
Copy link
Member

@hkaiser hkaiser commented Apr 21, 2015

This fixes the action move test count problems introduced recently

@sithhell
Copy link
Member

Future serialization is really getting out of hand :/ Those changes also mean a breaking change for user code, correct?
I would by far prefer to just wait in the future serialization code until it is ready instead of relying on those traits which are neither documented and also look very cumbersome to use.

@sithhell
Copy link
Member

After going through the requested changes, the only difference between waiting directly in the future serialization code and the currently mechanism in place is the creation of a new thread. This is an important difference and not having such a separate 'waiting' thread might lead to deadlocks.

A proper solution could be to have various serialization passes with special archive types. The one checking for non-ready futures could have a shared state to which the non ready futures register themselves. Instead of a buffer, the result of this serialization would a future that marks the completion of all futures which are about to be serialized. This avoids the need for a special trait completely.

The same mechanism can be employed to calculate the exact size needed in a buffer to serialize a parcel at hand, again, without the need for a special trait.

@sithhell
Copy link
Member

With all that said above, this doesn't affect this pull request as it fixes a problem we have right now. So I think it is good to be merged.

sithhell added a commit that referenced this pull request Apr 22, 2015
@sithhell sithhell merged commit b0346d4 into master Apr 22, 2015
@sithhell sithhell deleted the fixing_action_move_tests branch April 22, 2015 06:31
@hkaiser
Copy link
Member Author

hkaiser commented Apr 22, 2015

Future serialization is really getting out of hand :/

I agree.

Those changes also mean a breaking change for user code, correct?

No changes in user code are required.

I would by far prefer to just wait in the future serialization code until it is ready instead of relying on those traits which are neither documented and also look very cumbersome to use.

I'd prefer that as well, although I seem to remember that we agreed its not possible because of the limited amount of network connection we have available (are those still acquired before serialization?). If it is possible now, I'd be happy to remove all of that serialize_as_future trait.

@hkaiser
Copy link
Member Author

hkaiser commented Apr 22, 2015

After going through the requested changes, the only difference between waiting directly in the future serialization code and the currently mechanism in place is the creation of a new thread. This is an important difference and not having such a separate 'waiting' thread might lead to deadlocks.

I'm not sure to understand. Could you elaborate, please?

A proper solution could be to have various serialization passes with special archive types. The one checking for non-ready futures could have a shared state to which the non ready futures register themselves. Instead of a buffer, the result of this serialization would a future that marks the completion of all futures which are about to be serialized. This avoids the need for a special trait completely.

This would mean introducing a lot of overhead in most situations to be able to resolve single cases.

@sithhell
Copy link
Member

On 04/22/2015 02:19 PM, Hartmut Kaiser wrote:

I would by far prefer to just wait in the future serialization code until it is ready instead of relying on those traits which are neither documented and also look very cumbersome to use.

I'd prefer that as well, although I seem to remember that we agreed its not possible because of the limited amount of network connection we have available (are those still acquired before serialization?). If it is possible now, I'd be happy to remove all of that serialize_as_future trait.

Should be possible now, but probably needs to be tested extensively
before merging that change.

@sithhell
Copy link
Member

After going through the requested changes, the only difference between waiting directly in the future serialization code and the currently mechanism in place is the creation of a new thread. This is an important difference and not having such a separate 'waiting' thread might lead to deadlocks.

I'm not sure to understand. Could you elaborate, please?

possible deadlock in the parcelport due to a limited number of
connections, but that should be handled properly now.

A proper solution could be to have various serialization passes with special archive types. The one checking for non-ready futures could have a shared state to which the non ready futures register themselves. Instead of a buffer, the result of this serialization would a future that marks the completion of all futures which are about to be serialized. This avoids the need for a special trait completely.

This would mean introducing a lot of overhead in most situations to be able to resolve single cases.

We already have a overhead for that single case. But I certainly agree
that it is heavier than what we have right now. But it would make
everything correct, not just the cases where the trait is defined correctly.

@hkaiser
Copy link
Member Author

hkaiser commented Apr 22, 2015

We already have a overhead for that single case. But I certainly agree
that it is heavier than what we have right now. But it would make
everything correct, not just the cases where the trait is defined correctly.

This scheme would work for vanilla serialization function in user code only (i.e. only for things like ar & val1 & val2 & ...;). Anything beyond this would break or create un-necessary overhead. For things like zero-copy serialization this would still require some kind of mechanism to handle things correctly, though (such like an additional function besides serialize(). which would be used by the special archives).

@hkaiser
Copy link
Member Author

hkaiser commented Apr 22, 2015

possible deadlock in the parcelport due to a limited number of
connections, but that should be handled properly now.

Is the keyword 'should', or are we sure of that?

@sithhell
Copy link
Member

possible deadlock in the parcelport due to a limited number of
connections, but that should be handled properly now.

Is the keyword 'should', or are we sure of that?

We didn't have a single deadlock in distributed for quite some time now due
to the serialization suspending the running thread (caused by credit
splitting), so I'm fairly certain that we got rid of that issue.

@hkaiser
Copy link
Member Author

hkaiser commented Apr 22, 2015

We didn't have a single deadlock in distributed for quite some time now due
to the serialization suspending the running thread (caused by credit
splitting), so I'm fairly certain that we got rid of that issue.

Does that mean we could get rid of the whole serialize_as_future business? Let's create a ticket for that.

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

Successfully merging this pull request may close these issues.

None yet

2 participants