Skip to content

Conversation

rm155
Copy link
Contributor

@rm155 rm155 commented Jul 8, 2021

It is important that the ENV object be accessible in Ractors. To make it Ractor-safe, its methods that get/set its values are now synchronized with this modification. This is done by placing a mutex around each of these methods. With this, the ENV object is now Ractor-safe and shareable. Additionally, tests are added to ensure that its behavior is correct in Ractors.

@ioquatix
Copy link
Member

ioquatix commented Jul 8, 2021

Is there any performance impact for non-Ractor code?

@ioquatix
Copy link
Member

ioquatix commented Jul 8, 2021

Also, does this mean you can use ENV for message passing between Ractors!?

@ioquatix ioquatix requested a review from ko1 July 8, 2021 04:37
@rm155
Copy link
Contributor Author

rm155 commented Jul 9, 2021

Is there any performance impact for non-Ractor code?

There is some performance impact, but the level of impact varies between methods. For some methods, it seems negligible. For others, there is a visible impact on speed, but for many of these the impact is comparatively insignificant when ENV is larger (because the overhead from the locks remains constant).

@rm155
Copy link
Contributor Author

rm155 commented Jul 9, 2021

Also, does this mean you can use ENV for message passing between Ractors!?

Yes, it would be possible to have one Ractor store a value/message in ENV and have another Ractor read it.

Comment on lines +5400 to +5411
static void
copy_env_pairs(const char **arr, int size)
{
char **env;
env = GET_ENVIRON(environ);
for (int i = 0; i < size; i++) {
const char *p = *env;
arr[i] = p;
env++;
}
FREE_ENVIRON(environ);
}
Copy link
Member

Choose a reason for hiding this comment

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

Copied entries may no longer be valid once other threads change any envs.

Copy link
Member

Choose a reason for hiding this comment

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

More accurately, this function doesn't copy the env contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your time in reviewing this!

I agree that the function is not creating a deep copy of the environment contents. However, in my tests, simply copying the pointer to the original environment was enough. know that inconsistencies in the data are possible, but I believe that ENV already has inconsistent behavior when used in simultaneous threads. Is there a case I missed that causes an error in the program?

Thanks again!

Copy link
Member

Choose a reason for hiding this comment

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

Therefore ENV isn't ractor-safe.
Before the ractor, GVL prevented it from the inconsistencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I will think about it some more.

@ko1
Copy link
Contributor

ko1 commented Dec 14, 2021

Discussion with @nobu we found that the shallow copy of environ is not enough for atomic operation. To avoid full-copy, we decided to create Ruby objects while locking. So I made new patch on this PR patches. #5263

@rm155 thank you for great contribution, I can't make new patch without your work.

@ko1 ko1 closed this Dec 14, 2021
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.

4 participants