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

Scoped values #50958

Merged
merged 2 commits into from
Sep 16, 2023
Merged

Scoped values #50958

merged 2 commits into from
Sep 16, 2023

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Aug 17, 2023

Alternative proposal to #35833, based on a discussion with @gbaraldi.

ScopedVariables are containers whose observed value depends the current
dynamic scope. This implementation is inspired by https://openjdk.org/jeps/446

A scope is introduced with the scoped function that takes a lambda to
execute within the new scope. The value of a ScopedValue is
constant within that scope and can only be set upon introduction
of a new scope.

Scopes are propagated across tasks boundaries.

In contrast to #35833 the storage of the per-scope data does not require copies
upon scope entry. This also means that libraries can use scoped variables without
paying for scoped variables introduces in other libraries.

Finding the current value of a ScopedValue, involves walking the
scope chain upwards and checking if the scoped variable has a value
for the current or one of its parent scopes. This means the cost of
a lookup scales with the depth of the dynamic scoping. This is amortized
by perform some caching.

https://github.com/vchuravy/ScopedValues.jl provides an implementation
reusing the logstate trick from ContextVariablesX.jl. We could use it
as an implementation from Julia 1.7+ on-wards.

@vchuravy vchuravy added needs docs Documentation for this change is required needs news A NEWS entry is required for this change feature Indicates new feature / enhancement requests labels Aug 17, 2023
@vchuravy vchuravy requested review from c42f and gbaraldi August 17, 2023 16:57
@vchuravy vchuravy added the triage This should be discussed on a triage call label Aug 17, 2023
base/scopedvariables.jl Outdated Show resolved Hide resolved
base/scopedvariables.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

👍 Very good proposal. I think we should use the name ScopedValue (1) to use the same name as Java, (2) to further distinguish it from normal variables, since after all every variable has a scope. To be really pedantic the word "dynamic" should be in there since this is dynamic scope, but I acknowledge that DynamicallyScopedVariable is too long.

@JeffBezanson JeffBezanson added the multithreading Base.Threads and related functionality label Aug 17, 2023
@simonbyrne
Copy link
Contributor

Or a ScopedRef (since it is effectively a Ref, but the value inside depends on the scope)?

@simonbyrne
Copy link
Contributor

Looks very neat! A couple of questions:

  • How should this work with Distributed.jl? If I spawn a task on a remote process, would the scoped variables get passed?
  • Could this be made to work for the RNG state (I believe we currently we split the rng state when creating new tasks)? Though probably the overhead here would be prohibitive.

@vchuravy
Copy link
Member Author

I think we should use the name ScopedValue

Yeah I remarked to Gabriel earlier that I should have named it ScopedValue.

Or a ScopedRef (since it is effectively a Ref, but the value inside depends on the scope)?

It's not mutable and that is what I would expect from Ref.

How should this work with Distributed.jl?

Must it? I think there is something possible, but this is essentially global state and sending that across the wire is messy at best.
One could create a compressed view and then only send that, but this will require some further thought.

Could this be made to work for the RNG state. (I believe we currently we split the rng state when creating new tasks)?

I think this would mean entering a new dynamic scope everytime we create a task. I intentionally choose a coarser granularity (and this is a useful concept without tasks).

I managed to get the overhead downs a fair bit (see https://github.com/vchuravy/ScopedValues.jl for some numbers for a more clever implementation than in this PR.), but a scope creation is still ~70.2 ns and 112 bytes over 4 allocs.

@vchuravy vchuravy changed the title RFC: Scoped variables RFC: Scoped values Aug 18, 2023
@vchuravy vchuravy removed the needs docs Documentation for this change is required label Aug 18, 2023
@vchuravy vchuravy changed the title RFC: Scoped values Scoped values Aug 18, 2023
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
@rfourquet
Copy link
Member

FWIW, not knowing Java, I don't think I would guess what "scoped value" is, whereas "dynamic variable" is more directly obvious. Sure DynamicallyScopedVariable is too long, but maybe this could be shortened, e.g. Dynamic, DynamicVariable or DynamicVar.

@vchuravy
Copy link
Member Author

FWIW, not knowing Java, I don't think I would guess what "scoped value" is, whereas "dynamic variable" is more directly obvious.

I do think there is some value in using the same name as Java (where this concept is also very new).

I think the larger point is that it is actually not a variable,
but rather a value that has different values in different dynamic scopes. So the scope part is more important than the dynamic part for me. (Also isn't Ref a dynamic value?)

Copy link
Member

@jpsamaroo jpsamaroo left a comment

Choose a reason for hiding this comment

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

Awesome! I'm super excited for this. Various tweaks and nitpicks; feel free to ignore them if you don't agree.

doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
doc/src/base/scopedvalues.md Outdated Show resolved Hide resolved
@simonbyrne
Copy link
Contributor

How should this work with Distributed.jl?

Must it? I think there is something possible, but this is essentially global state and sending that across the wire is messy at best.

The context of this was #48121. I think you're right, in that you don't want to ship global state across the wire. I'm not sure what is a more general solution.

@jpsamaroo
Copy link
Member

How should this work with Distributed.jl?

Must it? I think there is something possible, but this is essentially global state and sending that across the wire is messy at best.

The context of this was #48121. I think you're right, in that you don't want to ship global state across the wire. I'm not sure what is a more general solution.

Note that this is something you can do with Dagger.jl, which uses ContextVariablesX.jl (but I would switch to ScopedValues once they're available). Dagger doesn't do this for all scoped variables, but only for its own variables for which propagating their values makes sense (and this works quite well for the composability of nested task spawning). But I wouldn't say a simple model like what Distributed.jl exposes should propagate anything by default.

@vchuravy
Copy link
Member Author

Yeah I think we could definitely implement a distributed dynamic scope on top of this.

@vchuravy
Copy link
Member Author

Based on the feedback from triage and some more discussion with @gbaraldi I improved the implementation.

  1. Based on ImmutableDict (but inlined)
  2. I was trying to use (too clever) caching but the fastest I got implementations where ~80-110ns
  3. ImmutableDict lookup is O(n) but linear search is really fast. Scope depth of 32 is ~26ns (https://github.com/vchuravy/ScopedValues.jl/blob/main/perf.md)
  4. Gabriel and I think that we can use HAMT or CTrie as a datastrcture, but those are complex and it is not yet clear that it is worth yet.

In previous versions each scope contained an immutable dict. This allowed implementing scoped(svar1 => 1, svar =>2) as a single scope entry,
by inlining the immutable dict, scoped(scar1 =>1, svar2 =>2) is now identical to:

scoped(svar1 => 1) do
   scoped(svar2 =>2) do
   end
end

This saved a pointer lookup, an allocation and a couple of bytes.

@vchuravy vchuravy removed the triage This should be discussed on a triage call label Sep 14, 2023
@vchuravy
Copy link
Member Author

From triage: ScopedValue and with is okay.

I will clean this up and get it over the line.

@LilithHafner
Copy link
Member

I still dislike the name ScopedValue, but not enough to try to block this and I don't have a great alternative. Go for it.

base/scopedvalues.jl Outdated Show resolved Hide resolved
base/scopedvalues.jl Outdated Show resolved Hide resolved
ScopedValues are containers whose observed value depends the current
dynamic scope. This implementation is inspired by https://openjdk.org/jeps/446

A scope is introduced with the `scoped` function that takes a lambda to
execute within the new scope. The value of a `ScopedVariable` is
constant within that scope and can only be set upon introduction
of a new scope.

Scopes are propagated across tasks boundaries.

Variable storage is implemented using a persistent dictionary. Upon
scope entry a small amount of data is copied and the unchanged pieces
are shared.
base/scopedvalues.jl Outdated Show resolved Hide resolved
@vchuravy vchuravy merged commit 27c24f6 into master Sep 16, 2023
1 check passed
@vchuravy vchuravy deleted the vc/scopedvariables branch September 16, 2023 12:51
NHDaly pushed a commit that referenced this pull request Sep 20, 2023
ScopedVariables are containers whose observed value depends the current
dynamic scope. This implementation is inspired by
https://openjdk.org/jeps/446

A scope is introduced with the `scoped` function that takes a lambda to
execute within the new scope. The value of a `ScopedValue` is
constant within that scope and can only be set upon introduction
of a new scope.

Scopes are propagated across tasks boundaries.

Storage is implemented using a persistent dictionary.
@cscherrer
Copy link

Just came across this. We use with in NestedTuples.jl. This takes e.g. a named tuple and a type-level expression, and evaluates the result. "Named tuple" can be replaced by an iterator over these, resulting in a iterator of results.

Would this PR make with a keyword? That would cause problems for me. Should be fine if it's just adding a function to Base, then I can still add methods.

@vchuravy
Copy link
Member Author

It's a new Base export, not a keyword.

@KristofferC
Copy link
Member

Should be fine if it's just adding a function to Base, then I can still add methods.

Note you should only add a method to Base.with if you are adding another implementation of the same generic function. That seems unlikely here so users should then decide which one to use by either explicitly prefixing with the module or by importing the symbol from the package.

@cscherrer
Copy link

Looking closer, I export @with, but not with, so maybe this won't be an issue.

Still, it would have been nice if someone had done a quick grep for uses of this in registered packages before agreeing on the name. This is a breaking change to any package that exports with, since downstream users would get ambiguity errors.

@vchuravy
Copy link
Member Author

Looking closer, I export @with, but not with, so maybe this won't be an issue.

Ah Base also exports @with.

it would have been nice if someone had done a quick grep for uses of this in registered packages

I tried, but didn't find anything obvious.

@cscherrer
Copy link

I tried, but didn't find anything obvious.

Looks like there are plenty of others:
https://juliahub.com/ui/Search?q=with&type=symbols

@oschulz
Copy link
Contributor

oschulz commented Oct 25, 2023

Any chance of backporting this to v1.10? I know it's very (very) late in the game - it's just that this is one of those things that might influence the basic approach packages take to certain aspects, and depending on which Julia version might become the next LTS ...

@vchuravy
Copy link
Member Author

We don't backport features on principle. Especially not for the reason that 1.10 may be an LTS (LTS decisions are made after an release not before it)

That being said ScopedValues.jl is fully functional on 1.8+ and can be used with some care (e.g. you need to use it's logstate method)

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 25, 2023

That being said ScopedValues.jl is fully functional on 1.8+

I see it does:

export ScopedValue, with

if isdefined(Base, :ScopedValues)
    import Base.ScopedValues: ScopedValue, with, Scope, current_scope, get

so that it works, also on 1.11. But can't people e.g. define with in their code (for something unrelated) and you end up with trouble (i.e. code that's not forward compatible with that package nor Julia 1.11)?!

Does it make sure to define in 1.10 at least something like:

with() = error("install ScopedValues.jl or upgrade to 1.11")?

I'm not sure about this new functionality, it seems very important, though clearly optional since we've managed without until now. I'm thinking we want all code that works for LTS, assuming 1.10 will become that, to work in future versions. Of course we can't backport everything, but there are seemingly justifiable exceptions for LTS (at least eventually).

@oschulz
Copy link
Contributor

oschulz commented Oct 25, 2023

So ScopedValues.jl could basically be used as a "Compat" here?


const ScopeStorage = Base.PersistentDict{ScopedValue, Any}

mutable struct Scope
Copy link
Member

Choose a reason for hiding this comment

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

Why is this mutable? I don't see any place where we actually mutate it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that's an artifact from an earlier implementation, should be changed or values should be const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.