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

Drop Delay from global config #460

Open
jdantonio opened this issue Nov 2, 2015 · 8 comments
Open

Drop Delay from global config #460

jdantonio opened this issue Nov 2, 2015 · 8 comments
Assignees
Labels
enhancement Adding features, adding tests, improving documentation. looking-for-contributor We are looking for a contributor to help with this issue.

Comments

@jdantonio
Copy link
Member

One of the biggest barriers to using autoload w/i this gem is the circular reference caused by the use of Delay within the global config. I no longer feel that we need to use Delay in the global config and I would like to remove it. I'd like some feedback from the community on this.

A bit of history...

The earliest versions of our thread pools spawned threads on construction. This meant that applications using c-r would always spawn a few background threads, even when they never post jobs (Rails and Sidekiq are good examples). To prevent this I used Delay to lazy-load the thread pools. This added a slight performance hit every time a job was post to a global thread pool because Delay objects are synchronized, but that seemed like a fair price to pay.

Later I decided to make the global thread pools configurable. This eventually lead us to add a reconfigure option on Delay. This, in turn, lead to the addition of a timeout option on Delay's #value call. To be consistent with everything else in this gem, Delay was updated to use the common executor options helper and default to a global thread pool. Hence, the circular reference.

Two things have changed since then that completely change the nature of this problem:

  1. Our thread pools now lazy-load threads so no threads are created until the first job is post
  2. We've removed the ability for the user to change the global thread pools (preferring dependency injection on the individual classes)

Subsequently, the use of Delay in the global config seems to provide little value. I think it can safely be removed. Removing it should give a tiny performance improvement every time a job is post to a global thread pool. More importantly, I can remove the circular reference that's been plaguing me.

Thoughts?

@jdantonio jdantonio added the question An user question, does not change the library. label Nov 2, 2015
@jdantonio jdantonio added this to the 1.0.0 Release milestone Nov 2, 2015
@jdantonio jdantonio self-assigned this Nov 2, 2015
@pitr-ch
Copy link
Member

pitr-ch commented Nov 3, 2015

Great sum-up. I am for removing it. It makes perfect sense. If we decide to migrate to autoload we can use it to load the thread pool instance lazily as well (they would be stored in a constant then) without paying the performance hit on access.

@jdantonio jdantonio modified the milestones: 2.0.0 Release, 1.0.0 Release Nov 5, 2015
@jdantonio
Copy link
Member Author

I quickly attempted this in PR #465 and many tests failed. It's not something I'm going to attempt right now, with the 1.0 release being so close. We'll make this part of the 2.0 release.

@eregon
Copy link
Collaborator

eregon commented Nov 6, 2015

Just for info, autoload is also still unsafe on many implementations, so I would not recommend it for concurrent-ruby.

@pitr-ch
Copy link
Member

pitr-ch commented Nov 8, 2015

@eregon Thanks for warning, do you know which ones? I thought it was fixed on MRI and JRuby.

@eregon
Copy link
Collaborator

eregon commented Nov 9, 2015

Well, recent MRI has a couple autoloading bugs, there is a RubySpec which fails sporadically for years, a few bug/deadlock reports for JRuby, JRuby+Truffle and new implementations most likely do not handle it too well, etc.

@pitr-ch
Copy link
Member

pitr-ch commented Nov 9, 2015

Ah ok, better not to use it then. I thought MRI and JRuby is ok, so we would just fix JRuby+Truffle.

@jdantonio
Copy link
Member Author

I was under the impression, from a prior discussion with @headius, that autoload was thread safe on both MRI and JRuby. Both the atomic and thread_safe gems used autoload extensively, and both have been merged into c-r.

@jdantonio jdantonio removed their assignment Oct 12, 2016
@pitr-ch pitr-ch added enhancement Adding features, adding tests, improving documentation. looking-for-contributor We are looking for a contributor to help with this issue. and removed question An user question, does not change the library. labels Jul 6, 2018
@pitr-ch pitr-ch changed the title Question: Drop Delay from global config Drop Delay from global config Jul 6, 2018
@pitr-ch pitr-ch removed this from the 2.0.0 milestone Jul 6, 2018
@pitr-ch pitr-ch added this to Chores in Hackathon Aug 24, 2019
@eregon
Copy link
Collaborator

eregon commented Jan 23, 2023

I solved some of the cycles for #934, but this specific cycle remains.
Yeah, it sounds like we should avoid using Delay there, seems pretty straightforward.

I would avoid using autoload for anything, there seems to be no need now that we have fine-grained dependencies between files.

@eregon eregon self-assigned this Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation. looking-for-contributor We are looking for a contributor to help with this issue.
Projects
Hackathon
Small / Chores
Development

No branches or pull requests

3 participants