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

Assigning nil to fiber storage deletes the association. #7378

Merged
merged 1 commit into from Feb 25, 2023

Conversation

ioquatix
Copy link
Member

  • Reduce memory allocations when looking up Fiber#storage.

- Reduce memory allocations when looking up `Fiber#storage`.
@ioquatix ioquatix changed the title Assigning nil to fiber storage deletes the association. Assigning nil to fiber storage deletes the association. Feb 25, 2023
@ioquatix ioquatix merged commit f94e83f into ruby:master Feb 25, 2023
@ioquatix ioquatix deleted the fiber-storage-nil-assignment branch February 25, 2023 06:27
@eregon
Copy link
Member

eregon commented Feb 27, 2023

Link: https://bugs.ruby-lang.org/issues/19333

@ioquatix
Copy link
Member Author

I'm happy to not specify this behaviour in Ruby Spec if you think it's an impediment to TruffleRuby using shapes. We can explore the performance trade offs. I'm okay with leaving that behaviour as "under specified" or "implementation defined".

@eregon
Copy link
Member

eregon commented Mar 5, 2023

Well, that wouldn't help if user code starts to rely on this.
IMO we shouldn't do this, either have a more explicit method to deal with this, e.g., one taking a block so it's clear it's only set for some duration and cleaned up, or not do it at all since it seems not so much an issue in practice (the same issue exists for Fiber-locals and Thread-locals and is not solved or reported since many years).

The whole design of x-locals with a Symbol key is around having well-named keys and not an automatically-generated part in it.
https://ruby-concurrency.github.io/concurrent-ruby/master/Concurrent/ThreadLocalVar.html is the only real solution for those use-cases of GC-able and anonymous locals.

So my suggestion would be to revert this, because there is no need at this point and it doesn't fully solve the problem either.

@eregon
Copy link
Member

eregon commented Mar 5, 2023

I think my comment in https://bugs.ruby-lang.org/issues/19333#note-2 explains clearly that this is not a worthy trade-off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants