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

Add suspend. (GC is disabled in the process.) #134

Merged
merged 1 commit into from
Apr 22, 2015
Merged

Conversation

albertnetymk
Copy link
Contributor

Similar to futures, suspend has support for both eager and lazy as well. lazy is used by default.

@supercooldave
Copy link

If it's an either or situation (either suspend or GC), then perhaps there should be a flag for people not using suspend (or not requiring GC).

Or does that not make sense.

@albertnetymk
Copy link
Contributor Author

As long as developers don't use suspend in their code, they have full GC support. Is this what you have in mind?

@supercooldave
Copy link

Something like this, though it could be a flag that turns on support for suspend (and off GC support).

@albertnetymk
Copy link
Contributor Author

It's already the case now. GC is disabled for one specific actor from the moment it calls suspend, then it's turned on when that msg runs to completion. Maybe the commit msg is misleading?

@supercooldave
Copy link

Your commit message is fine.
I just interpreted it differently.
Perhaps "GC is disables while processing suspend".

@kikofernandez
Copy link
Contributor

Love your commit, it removes more things than it adds!

As a side note, it would be good if we keep a standard when removing last spaces, either we leave them all or we remove them all. Not that it's an important question to address here, but looks like modifications in the code when they are not relevant.

I'll check your code as soon as I arrive to the office

@albertnetymk
Copy link
Contributor Author

Has anyone else tested this PR? If it's working fine, I would move on to await then.

@supercooldave
Copy link

I'll give it a test. But feel free to move on anyway.

@supercooldave
Copy link

Looks good. Runs fine on my machine.
@kikofernandez?

@kikofernandez
Copy link
Contributor

started with the code review now! the suspend test passes, let's look at the code

kikofernandez pushed a commit that referenced this pull request Apr 22, 2015
Add `suspend`. (GC is disabled in the process.)
@kikofernandez kikofernandez merged commit 99f976b into master Apr 22, 2015
@kikofernandez
Copy link
Contributor

Tested combinations:

  • debug with eager and lazy
  • release with eager and lazy

My only comment is that in encore.h the public function actor_suspend_resume(ucontext_t* ctx) should be made private, since it's only used inside encore.c. As agreed with @albertnetymk he will do this change in the next PR together with the await fixes.

@kikofernandez kikofernandez deleted the suspend branch April 22, 2015 13:12
@supercooldave
Copy link

Maybe Albert should be reminded not to branch encore but rather the do a private branch, unless he wants to buy cake for everyone. (If @supercooldave can do things properly, anyone can!)

@kikofernandez
Copy link
Contributor

👍

@supercooldave
Copy link

Mikael @EagiZ has already been using this for his project work (code larger than test cases) and it does what it should. Great work!

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.

3 participants