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

[Truffle] Add ThreadGroup layout and nodes #4110

Merged
merged 5 commits into from
Aug 26, 2016

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Aug 24, 2016

  • Layout needs review, Atomic/Volatile maybe needed

@bjfish bjfish added the truffle label Aug 24, 2016
@eregon
Copy link
Member

eregon commented Aug 24, 2016

Just wondering, why does this need to be done in Java and not just Ruby?
Probably we need a group field in ThreadLayout but what else?

@bjfish
Copy link
Contributor Author

bjfish commented Aug 24, 2016

@eregon I can't think of any issues of doing it in ruby, I'll try it in ruby

@bjfish
Copy link
Contributor Author

bjfish commented Aug 24, 2016

@eregon I've updated now to use Ruby

@@ -425,7 +440,8 @@ public DynamicObject allocate(
new AtomicReference<>(null),
new AtomicReference<>(null),
new AtomicBoolean(false),
new AtomicInteger(0));
new AtomicInteger(0),
currentGroup);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to set the group in Thread#initialize method but I don't think initialize method was being called. Is allocate okay to set this for now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that sounds fine.

@eregon
Copy link
Member

eregon commented Aug 25, 2016

Well done, this looks much cleaner and smaller now :)

@eregon
Copy link
Member

eregon commented Aug 25, 2016

Let's get this merged once the CI passes!

end
Truffle.invoke_primitive :thread_set_group, Thread.current, ThreadGroup::Default
Copy link
Member

Choose a reason for hiding this comment

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

This is probably causing the CI failure.
@chrisseaton So should thread_set_group not have UnsafeGroup.THREADS since it's a primitive only called internally or is there another solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, no need for UnsafeGroup.THREADS on thread_set_group since it doesn't really do anything unsafe (IO, syscalls, creating new threads, etc).

@eregon eregon merged commit ebb6fb7 into truffle-head Aug 26, 2016
@eregon eregon deleted the truffle-thread-group-layout branch August 26, 2016 15:51
@eregon
Copy link
Member

eregon commented Aug 26, 2016

Build is green, merged! Thanks for going through all my comments!

@enebo enebo added this to the truffle-dev milestone Aug 26, 2016
@enebo enebo added this to the Invalid or Duplicate milestone Dec 7, 2017
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.

None yet

4 participants