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 tracing for future value. #166

Merged
merged 1 commit into from
May 18, 2015
Merged

Fix tracing for future value. #166

merged 1 commit into from
May 18, 2015

Conversation

albertnetymk
Copy link
Contributor

No description provided.

@kikofernandez kikofernandez self-assigned this May 15, 2015
@@ -88,17 +88,12 @@ struct future
future_t *parent;
closure_entry_t *children;
actor_list *awaited_actors;
future_t *next;
Copy link
Contributor

Choose a reason for hiding this comment

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

the PR seems ok. I would only argue that we are adding the pointer next because it's needed by the actor that creates a linked list of futures. Futures per se don't need this pointer in the sense that this is bookkeeping of the actor to gc the future.
I believe it could be better if we create a simple linked list structure
that gets added to the actor fields, instead of the fut* my_future, and a couple of operations to go back and forth in the list (your future_set_next() and future_get_next()). This will give us a better separation of concerns since, as mentioned before, futures do not need to do any bookkeeping, it's the actors that need to.
Opinions on this are welcome. If no one replies before the Hackathon, I'll merge it

@albertnetymk
Copy link
Contributor Author

next is used to keep track of the futures owned by the current actor to check if finalizer needs to run. It would be retired once new PonyRT is merged, for it provides finalizers for passive objects.

Disabled `largestream`, for it causes stack overflow, which, I believe,
is fixed in the upstream.

Disabled `async_force_gc` and `async_foreach`, for the fix is on the
way.
@albertnetymk
Copy link
Contributor Author

Reviewer: Planning to merge this one? Any objections?

@albertnetymk
Copy link
Contributor Author

Author of this PR: One second, needs to be rebased.

@albertnetymk
Copy link
Contributor Author

Author of this PR: Rebased, good to go.

albertnetymk added a commit that referenced this pull request May 18, 2015
@albertnetymk albertnetymk merged commit a47e15a into parapluu:master May 18, 2015
@kikofernandez kikofernandez deleted the trace-future-value branch May 18, 2015 13:09
@eastlund eastlund mentioned this pull request Jun 8, 2015
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