new conc future api

[duckyduckmaster]

An implementation of multiple classes of lock-free futures, in order to fix the scalability problems of using only one class with mutex-locks. There are three different future classes available which are defined in three different source files:

• future.c : futures with multiple get, await and chain operations.
• vanilla_future.c : futures with only one get operation.
• poly_vanilla_future.c : futures with multiple get operations.

Currently the API of the future.c file is the only used and further code is needed in order to enable vanilla and poly vanilla futures, such as compiler analysis:
I forgot to include tests so they may be put inside another PR.

[kikofernandez]

could you provide some brief explanation about the different kinds of futures?

[kikofernandez]

this function seems to be used for all futures. Is this correct?
If so, shouldn't it be placed in a more general future file, instead of in the vanilla future?
(I may be missing the point where this is called and if this is always called)

[duckyduckmaster]

Yes, it is correct that this function is used for all futures, the reason why its placed inside the vanilla future file is because the vanilla future is the future which is most often used, and therefore most calls to handle_future can be inlined.

[kikofernandez]

Before this can be merged, I would like for the author to reply to the feedback :)

[kikofernandez]

Is this condition needed at all? The function above (poly_vanilla_future_mk) seems to do this checking.

[kikofernandez]

Does the __atomic_load_n work similar to acquire and release semantics, i.e., the state between acquire and release can be thought as a "transaction" visible to other threads?

[kikofernandez]

It would be great if the definition of vanilla futures et al is specified in the header files (or somewhere).
From the PR description:

future.c : futures with multiple get, await and chain operations.
vanilla_future.c : futures with only one get operation.
poly_vanilla_future.c : futures with multiple get operations.

it's not clear to me if a vanilla future has a single get operation or if it means that I cannot get multiple times from it (which could happen if there is an optimisation to reuse the future, e.g.)

[kikofernandez]

same as in poly_vanilla. Is this if needed?

[kikofernandez]

many of these methods seem to be exactly the same.
Would it make sense to have a macro that defines these common methods and the specialised methods in the files?

[kikofernandez]

not sure if the removal of this may affect ParTs

[kikofernandez]

I am not sure of what the naming means here. Maybe it's best if you provide some more information.
At the same time, I get the feeling that this discharge is possibly doing too many things. As I understood:

1. Goes through the list of blocked actors and schedules them
2. Starts running chained computations attached to the future, following a LIFO order. I would have expected that computations that are attached first run before the ones attached later (if the chaining operation happens in the same actor, and it's fair to assume any order if the future is shared among actors that may chain computations in this same future)

[duckyduckmaster]

1. The condition isn't needed 2. I am not that into how different atomic operations work, I just heard somewhere that using the flag __ATOMIC_SEQ_CST is the most strict option, reducing the potential errors encountered. 3. Many methods behave similarily, like in this case. I think I sacrificed alot of simplicity in return for an obscure gain in speed from micro-opts, something which could be questioned in the long run. I think both macros and branch structures could be helpful. 4. The comment could stay for educational purposes I think, as long as it's pointed out that locks are no longer used. 5. Yes, I agree. for fairness the stack with closures should be a queue with a FIFO pattern. The same principle could be applied to the stack with actors. 2017-09-15 13:36 GMT+02:00 Kiko <[email protected]>:

…

***@***.**** requested changes on this pull request. Before this can be merged, I would like for the author to reply to the feedback :) ------------------------------ In src/runtime/future/poly_vanilla_future.c <#821 (comment)>: > + +encore_arg_t poly_vanilla_future_get_actor(pony_ctx_t **ctx, poly_vanilla_future_t *fut) +{ + if (!__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { + ENC_DTRACE2(FUTURE_BLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + poly_vanilla_future_block_actor(ctx, fut); + ENC_DTRACE2(FUTURE_UNBLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + } + ENC_DTRACE2(FUTURE_GET, (uintptr_t) *ctx, (uintptr_t) fut); + return fut->value; +} + +static inline void poly_vanilla_future_block_actor(pony_ctx_t **ctx, poly_vanilla_future_t *fut) +{ + perr("future_block_actor"); + if (__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { Is this condition needed at all? The function above ( poly_vanilla_future_mk) seems to do this checking. ------------------------------ In src/runtime/future/poly_vanilla_future.c <#821 (comment)>: > + +encore_arg_t poly_vanilla_future_get_actor(pony_ctx_t **ctx, poly_vanilla_future_t *fut) +{ + if (!__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { + ENC_DTRACE2(FUTURE_BLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + poly_vanilla_future_block_actor(ctx, fut); + ENC_DTRACE2(FUTURE_UNBLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + } + ENC_DTRACE2(FUTURE_GET, (uintptr_t) *ctx, (uintptr_t) fut); + return fut->value; +} + +static inline void poly_vanilla_future_block_actor(pony_ctx_t **ctx, poly_vanilla_future_t *fut) +{ + perr("future_block_actor"); + if (__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { Does the __atomic_load_n work similar to acquire and release semantics, i.e., the state between acquire and release can be thought as a "transaction" visible to other threads? ------------------------------ In src/runtime/future/future.h <#821 (comment)>: > @@ -79,4 +121,37 @@ void future_register_callback(pony_ctx_t **ctx, * puts on hold the processing of this message. */ void future_await(pony_ctx_t **ctx, future_t *fut); + +extern pony_type_t future_type; + +void future_discharge(pony_ctx_t **ctx, future_t *fut); + +/* VANILLA FUTURE API */ It would be great if the definition of *vanilla* futures et al is specified in the header files (or somewhere). From the PR description: future.c : futures with multiple get, await and chain operations. vanilla_future.c : futures with only one get operation. poly_vanilla_future.c : futures with multiple get operations. it's not clear to me if a vanilla future has a single get operation or if it means that I cannot get multiple times from it (which could happen if there is an optimisation to reuse the future, e.g.) ------------------------------ In src/runtime/future/vanilla_future.c <#821 (comment)>: > + +encore_arg_t vanilla_future_get_actor(pony_ctx_t **ctx, vanilla_future_t *fut) +{ + if (!__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { + ENC_DTRACE2(FUTURE_BLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + vanilla_future_block_actor(ctx, fut); + ENC_DTRACE2(FUTURE_UNBLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + } + ENC_DTRACE2(FUTURE_GET, (uintptr_t) *ctx, (uintptr_t) fut); + return fut->value; +} + +static inline void vanilla_future_block_actor(pony_ctx_t **ctx, vanilla_future_t *fut) +{ + perr("future_block_actor"); + if (__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { same as in poly_vanilla. Is this if needed? ------------------------------ In src/runtime/future/vanilla_future.c <#821 (comment)>: > + +encore_arg_t vanilla_future_get_actor(pony_ctx_t **ctx, vanilla_future_t *fut) +{ + if (!__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { + ENC_DTRACE2(FUTURE_BLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + vanilla_future_block_actor(ctx, fut); + ENC_DTRACE2(FUTURE_UNBLOCK, (uintptr_t) *ctx, (uintptr_t) fut); + } + ENC_DTRACE2(FUTURE_GET, (uintptr_t) *ctx, (uintptr_t) fut); + return fut->value; +} + +static inline void vanilla_future_block_actor(pony_ctx_t **ctx, vanilla_future_t *fut) +{ + perr("future_block_actor"); + if (__atomic_load_n(&(fut->fulfilled), __ATOMIC_SEQ_CST)) { many of these methods seem to be exactly the same. Would it make sense to have a macro that defines these common methods and the specialised methods in the files? ------------------------------ In src/runtime/future/future.c <#821 (comment)>: > // =============================================================== // Create, inspect and fulfil // =============================================================== -// -// a future has a lock attached to it. actors may acquire the same lock -// multiple times (re-entrant locks). this does not happen when we work with plain Encore -// however, re-entrant locks are necessary for the ParT runtime library. -// -// Use case: not sure if the removal of this may affect ParTs ------------------------------ In src/runtime/future/future.c <#821 (comment)>: > -void future_await(pony_ctx_t **ctx, future_t *fut) +void future_discharge(pony_ctx_t **ctx, future_t *fut) I am not sure of what the naming means here. Maybe it's best if you provide some more information. At the same time, I get the feeling that this *discharge* is possibly doing too many things. As I understood: 1. Goes through the list of blocked actors and schedules them 2. Starts running chained computations attached to the future, following a LIFO order. I would have expected that computations that are attached first run before the ones attached later (if the chaining operation happens in the same actor, and it's fair to assume any order if the future is shared among actors that may chain computations in this same future) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#821 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYJX01mu_wH6my3JxNzGA2Lp51veBRzDks5simFGgaJpZM4N5AYd> .