UserStorageService and UserDiscoverer should return streams

[LordRalex]

The current implementations of the UserStorageService (USS) and UserDiscoverer (UD) return Collections when you reference getAll methods. This means that a call to those methods would result in a very large bottleneck if you only need to process X amount of records.

The match service is an example of where this behavior fails. In order to match, it looks at all profiles which match the criteria given. If you have lots of cached players, then it will match each record before returning, preventing a filter from a later stream (a call to stream() by those that use this) from being of performance benefit.

Examples of where this type of hit is expensive is Nucleus's TP command. If you try to tab-complete, it will look to see what players match the criteria. In order to do that, it calls these methods, which result in a massive performance hit (lots of IO) if you have a large player cache.

Streams should be used here so that calls can apply filters and/or defer execution till it has to, in the cases of limits. While the 2 here should not enforce a limit, this type of change would then ensure limits used by callers would actually perform reasonably.

Related lines:
https://github.com/SpongePowered/SpongeCommon/blob/stable-7/src/main/java/org/spongepowered/common/service/user/UserDiscoverer.java#L165
https://github.com/SpongePowered/SpongeCommon/blob/stable-7/src/main/java/org/spongepowered/common/service/user/SpongeUserStorageService.java#L111
https://github.com/NucleusPowered/Nucleus/blob/sponge-api/7/src/main/java/io/github/nucleuspowered/nucleus/argumentparsers/NicknameArgument.java#L208

[dualspiral]

Note that I'm trying to tackle this performance issue in #2124. It's difficult to test with a single player on a server (which is why I haven't done much with it)! Any tests/feedback would be helpful on that! I also haven't worked out the best way to create the cache in that case.

I agree that this would be good to do, it will help in many scenarios. However, with the way the user command element works, it will always pull back every user, regardless - streams won't help in that case.

[LordRalex]

I'm thinking that it's probably wise for those arguments to possibly include their own limit as well, since I don't think it makes sense for it to include all that could match.

I still think that from an interface level, it probably should be a stream, and implementations can decide how it's best handled. Since there's work to get the UserStorage more overrideable, I think it'd be wise to still make it be a stream to help ensure it's clear it probably should care about such requests.

Although, changing it to be a stream would be a breaking change, where-as the cache would be a backend change.

I can't test it myself, but since the tab completion issue has shown up, I did some looking and just found this particular issue, where I decided to at least open a ticket in specific to making it a stream, since even those command elements turn it into a stream.

If a build could be made, then some of those servers that have the issue could help test the fix.

[dualspiral]

I think I can do it as a plugin (with Mixins) at first, with a bit of a luck I'll have that ready soon so people can try just adding a plugin with whatever build they have - less cumbersome than trying the build itself.

As for streams, we could default implement stream versions in the interface, deprecate the currently used versions, and override the default implementations in SC. Once I've gotten the cache stuff done, we could probably bolt this on quite easily. Limiting the arguments to a subset might be harder thanks to the fact they are API implemented and so define a well known behaviour. I would much rather rip the user element to pieces but... eh, that's an API 8 job...

[dualspiral]

For reference: #2124 (comment)

Once that is sorted, we can get this API in.