-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
maps: remove Keys and Values for Go 1.21 #61538
Comments
I'm not clear on the value of using iterators for those methods rather than the normal range loops that exist today. We don't have map, filter, or reduce that work on iterators. Whatever package provides them can also trivially provide a function to adapt any rangeable value to an iterator. |
s/Slices/Values/g? |
We have relevant experience with something like And the reason it's unpleasant is because you never know how many keys are in the map now - each time there is chance of unbounded memory growth, that could not happened during original code review, but things had changed since then and now instead of 10 keys you constantly filtering from 10000 keys. But thats our experience anyway. |
Instead of |
@jimmyfrasche Yes thank you. Changed Keys and Slices -> Keys and Values.
That's true, but if the rangeable value is the current maps.Keys(m), then Keys builds the entire slice in memory even if you only just want to iterate through the keys and look at them. That's O(N) space that could be O(1) space instead, a large and potentially important difference for big maps. I don't think we should lock ourselves into the O(N) iteration strategy for map keys and map values. @DmitriyMV: Yes, exactly, that's the situation I am concerned about. If Keys returned an iterator instead of a []K, you wouldn't have the memory or latency hit of collecting all the keys. You might still have the latency of filtering if you are going to go through all of them, but you avoid the memory hit with a filtered iterator. And if you stop early, you avoid the latency hit too. @changkun Yes, if we keep Keys and Values as they are we will have to introduce some other iterator form for efficiency, maybe called KeysIter and ValuesIter. But I suspect most uses for Keys and Values work better with the iterator form (which would fix @DmitriyMV's example), meaning we should save the good names Keys and Values for them instead of giving them second-rate names. |
Why is the |
I don't see how we would be "locked into" an O(n) space iteration strategy for map keys or values. If I want O(1) space iteration over the keys of a map Of course, I could still do this with a slices.Collect that accepts an iterator. And if that doesn't happen, maps.Keys and maps.Values can of course be added later. It just seems unfortunate to me to revise an already accepted proposal to remove the feature I use most frequently from its experimental version because of a language change that might happen and that might be followed by an API addition that can't even be proposed until that language change is decided. |
Another way to provide a "first-class" name for composable map iterators would be to introduce a package |
More general formulations of algorithms might want to parameterize on something like I'm quickly unsure if or when this is "bad idea" territory, but it's probably not in some languages - it's just that I definitely agree about not getting locked in. |
@rsc I think we can flip this backwards: instead of talking about leaving the preferred name for iterators, think about how not using a suffix will cause confusion in the rest of the standard library. There are things all over that return slices, and with iterators, it could be valuable to add methods and functions that return them. This won't be the only place, so while we can use a suffix and have the new iterators be consistent, we cannot have the other way. Be consistent. Also, "ThingIter" sounds like a name of a thing iterator and "Things" sounds like I already have those things allocated. |
Pretty much everything in the standard library that returns a slice today should return an iterator in a future version. Adding "Iter" suffixes may be OK given the history but it's not where we want to end up. A v2 of those packages might flip the default. Or maybe there are good names available that don't use Iter. Either way, maps and slices are brand new packages. We don't want to go straight to a v2 just to fix this detail. We can avoid making the mistake to begin with if we just leave them out for this release and give ourselves another cycle to get it right. Maybe we'll end up back where we are today, but maybe not. I suspect not. |
I don't think it's entirely a mistake though. There are choices all over where we use slices as a return instead of some sort of iterator and have performance that is just fine. Anything that isn't CPU bound won't have a noticeable improvement either way. However, at any given time, most software developers are fairly junior and the concept of an iterator is more difficult than the concept of a slice. Python has had generators and iterators for quite some time, but most of the time you see people using lists. It's totally unnecessary, but outside shared libraries, it rarely causes a difference. I don't think you'll stop seeing slices used a lot more often than iterators even if we one day have a Go 2 where they are the more common option. That said, I personally haven't seen much indication that we would ever get to a point where such a big break would be worth it, so using words that are less specific in hopes that you can standardize on that eventually doesn't seem particularly helpful |
I think iterators shouldn't be the default choice, in general. They should be something the programmer consciously opts in to when needed. It simply follows the principle of least surprise. It is intuitive and unsurprising that
PS: the title of this issue should be updated too: "remove Keys and Slices" -> "remove Keys and Values" |
Normally you would want the most costly and specific option to be consciously opted in and clearly returning a slice is more costly than returning an Iterator. having the full slice of elements upfront is usefull only when you are sure that you would need all of them and can justify the memory used. The more generic case is that you would need only some of the values and/or potentially exit early from the loop making Iterators better suited for the default choice. You can collect the elements of the iterator in a slice if it's needed but while you still can create an iterator that loops over the slice, the "damage" of the memory use is already done and creating an iterator at that point is almost useless. |
I don’t suspect that. Python 3 changed from returning a list to returning an iterator, and it was often a pain because I needed to wrap the keys method in a list conversion. For maps.Keys today, my most frequent operation is to take the keys and sort them, which requires it being a slice. I think there should be an easy to use slice function. If it’s called KeySlice instead of Keys, that’s fine, but it is so frequently used it really is a pain to have to add a collect step in. |
If you need sorted map keys maybe you need a sorted map instead? Even without such a thing, if you need to collect the keys in a slice once it's not a lot of code (with or without stdlib changes) and if you need it more than once you can throw that in a function. |
And that function could be in the standard library. ;-) |
I'm not opposed to functions that collects the keys/values in a slice, but I agree with @rsc that is less general than the iterator form and hence the short name should go to the iter form. For the specific use case you bring up, I wonder if a more generally useful way to expose that would be a sorting func that takes an iterator as input and returns a slice. |
I agree that this is a release blocker and should be resolved definitively before 1.21 releases, but I also think this is a bit of retreading previous decisions. Iterators were discussed as part of the planning for the maps package. In #47330 (reply in thread) @ianlancetaylor wrote,
Did something change to make us rethink that? |
Reading the keys of a map to obtain a sorted slice of keys is a use case that I have encountered several times. Without the slices and maps package, I would have written
With the slices and maps packages, you can write
but with the change of this proposal I suggest introducing a
declared as
This would be a good improvement, using only the standard library, compared to the current situation. |
A process comment: we say to people that they should write code against release candidates and deploy them to production. At this point these functions have been in RCs for about a month, and I think it's plausible that people have uses functions in their codebase that will break when they update from the last RC to the final release. Are we concerned about the risk of breakage? |
The current version of maps.Keys is impl by me. I think https://go-review.googlesource.com/c/go/+/481435 and https://go-review.googlesource.com/c/go/+/481436. I do not know the detail impl of iters. but I think the current version of maps.Keys/Values could be faster that the Iter version. I think performance is also an important consideration. |
As many have already mentioned, the main use of maps.Keys is for sorting. In this case, what we need is not an iterator, but a slice to be sorted. |
CL 513715 is landed. |
Common cases named map.Keys, maps.Values for Iter named maps.KeyAll ,maps.ValueAll |
Preserve the names in case we want them to return an iterator. Keep the efficient runtime implementations for now, as we will probably want them under some name, perhaps KeysSlice and ValuesSlice. Fixes golang#61538 Change-Id: I6b03010bf071fb4531cb2f967dad46425962fcb8 Reviewed-on: https://go-review.googlesource.com/c/go/+/513476 Run-TryBot: Ian Lance Taylor <iant@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org> Reviewed-on: https://go-review.googlesource.com/c/go/+/513715 Run-TryBot: Russ Cox <rsc@golang.org>
Almost removing `golang.org/x/exp` but not yet completly because of golang/go#61538.
Almost removing `golang.org/x/exp` but not yet completly because of golang/go#61538.
Almost removing `golang.org/x/exp` but not yet completly because of golang/go#61538.
- Remove packages copied from std and change references. - Move functions in [maps] that were dropped by std right before release to maphelper. golang/go#61538 - Use min, max.
Preserve the names in case we want them to return an iterator. Keep the efficient runtime implementations for now, as we will probably want them under some name, perhaps KeysSlice and ValuesSlice. Fixes golang#61538 Change-Id: I6b03010bf071fb4531cb2f967dad46425962fcb8 Reviewed-on: https://go-review.googlesource.com/c/go/+/513476 Run-TryBot: Ian Lance Taylor <iant@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Preserve the names in case we want them to return an iterator. Keep the efficient runtime implementations for now, as we will probably want them under some name, perhaps KeysSlice and ValuesSlice. Fixes golang#61538 Change-Id: I6b03010bf071fb4531cb2f967dad46425962fcb8 Reviewed-on: https://go-review.googlesource.com/c/go/+/513476 Run-TryBot: Ian Lance Taylor <iant@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Can't agree more, please add maps.Keys and maps.Values back to the standard library. |
This issue is closed. See #61900 for the proposal to bring back maps.Keys and maps.Values as iterators. |
Given #61405 and the likelihood of additional operations on those kinds of iterator functions (like Map, Filter, and Reduce), it seems possible that we would want Keys and Values to return iterator functions that can be used with those and do not provoke O(N) storage. I imagine there would also be a slices.Collect that collects values from any iterator into a slice, so existing uses of maps.Keys(m) that still want a slice would become slices.Collect(maps.Keys(m)).
If there's a good chance we're going to want Keys and Values to return iterators in Go 1.22, it seems short-sighted to ship them in Go 1.21 returning slices. I wonder whether we should pull them. It is late in the cycle, but not too late. git grep says there are no mentions of maps.Keys or maps.Values in the standard library.
The text was updated successfully, but these errors were encountered: