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

FEATURE: add interface compatible constant version of thread local var #823

Closed
wants to merge 2 commits into from

Conversation

SamSaffron
Copy link

ThreadLocalVar can be expensive as it spins a thread in finalizers. In some
cases consumers may want to swap in a constant value implementations that
is very cheap.

Concurrent::ConstantThreadLocalVar is interface compatible with
AbstractThreadLocalVar, however does not allow any changing of .value

See more context in: rails/rails#36949

@pitr-ch how do you feel about this? Would you prefer this here or in Rails?

@jdantonio
Copy link
Member

I'm not actively involved in this gem anymore, but I think this PR would be a good addition, FWIW.

@pitr-ch pitr-ch added the enhancement Adding features, adding tests, improving documentation. label Feb 10, 2020
@pitr-ch
Copy link
Member

pitr-ch commented Feb 10, 2020

@SamSaffron Sorry for overlooking the PR. I'll update it and have a look at merging it.

SamSaffron and others added 2 commits February 10, 2020 20:54
ThreadLocalVar can be expensive as it spins a thread in finalizers. In some
cases consumers may want to swap in a constant value implementations that
is very cheap.

`Concurrent::ConstantThreadLocalVar` is interface compatible with
AbstractThreadLocalVar, however does not allow any changing of .value

def default
if @default_block
@default_block.call
Copy link
Member

Choose a reason for hiding this comment

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

This evaluates the @default_block on each value read, so it may return different values each time value is called. I think that if the default_block is provided the ConstantThreadLocalVar should still store the computed value per thread (since it might differ). So the implementation could inherit from ThreadLocalVar, disallow using value= , and it would be good if it optimises @default case (the value is equal for all threads).

@chrisseaton
Copy link
Member

@SamSaffron is it still worth me getting this merged?

@SamSaffron
Copy link
Author

I am mixed Chris ... I ended up working around the issue here: rails/rails#37200

The finalizer approach though is a bit problematic in many ways, but the workaround in pure Ruby is simple enough.

The problem with adding the API here though is that we have no real world consumers for it anymore.

@chrisseaton
Copy link
Member

Closing as no immediate need and we're trying to get on top of the backlog of this gem.

Feel free to re-suggest in the future.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants