Speed of transferring items is too slow

[TheBrenny]

The speed of transferring a batch of items between characters appears too slow, especially when a large number of items are being moved. For clarity: it's not the speed of a single item, but the speed of a batch of items (read more than one).

This is because the inventory.service.dart awaits on every API request, rather than submitting all the requests at once and then awaiting on a list of those promises futures.

Happy to implement this feature myself. Leaving this issue open for tracking.

[joaopmarquesini]

@TheBrenny I think I explored this problem sometime before and hit 2 issues: 1) race conditions (Little Light checks if the inventory have the required space for those transfers and doing them async would lose that and cause some transfers to fail) 2) Bungie API rate limiting (Calling Bungie API without a "delay" between calls may cause throttling, which escalates as we continue to abuse it) Also on 1: there's a lot of edge cases to explore there. For example, when you're moving stuff from postmaster to vault, you need at least one empty space available on that inventory slot so the items can make that trip through the character. LL frees one slot on the character to do that, and that already generate a lot of complaints ("Little Light stole my new godroll") by users that doesn't understand that Little Light needs to do the exact path that's available in-game (Postmaster -> Character -> Vault). To make that "asyncable" you would need a very complex control of "inventory state prediction" or an empty user inventory (which would probably get more users pissed because "Little Light stole my 9 godrolls").

[TheBrenny]

1. race conditions (Little Light checks if the inventory have the required space for those transfers and doing them async would lose that and cause some transfers to fail)

I get that, and I've thought about how we could assess which items to boot and which to keep before we even transfer a Loadout in. It's a logic problem that's fun to think about imo: for example, for a helmet, the user wants 5 slots free, they have 5 slots filled, and the incoming Loadout has 3 items, 1 of which is already on the character. How do you come up with a list of items to boot BEFORE we send the other items from the vault? I've thought about this here and there but never sat down to work it out.

2. ... Rate limiting ...

I did a quick google search before I posted this and one result from someone said about 250 calls per 10 seconds per IP. I don't know if that's the resolution (it could be 25/s or 1500/m), but it seems like a good estimate. Someone else on the same thread from this number said that the Docs state that apps that are seen to abuse the API will be throttled, although I didn't fact check this. Part of my googling I saw a preview that DIM uses an internal throttler (something like 100ms), but again I didn't fact check that either.

Edge cases/race condition: For example, when you're moving stuff from postmaster to vault, you need at least one empty space available on that inventory slot so the items can make that trip through the character.

Completely understand and in this context, awaits would be valid. Without looking into the code, I don't know if this exclusively relies on the inv service or if the postmaster has some intermediary code that can do some awaiting. Thanks for bringing this up though - I definitely didn't think about it!

[joaopmarquesini]

the postmaster logic part of the transfer function the app has a inventory service that has methods like transfer transferMany transferLoadout transfer will guide the item through the entire path from wherever it is to wherever it needs to be Eg:

• checks if it is on postmaster
• if it is, checks if there's available room on that char inventory
• if there isn't, move one item away to make room
• then move the item to character
• checks if the current character was the intended destination
• if it was, stop the process as a success
• checks if the vault have enough space
• if it don't, check if any other character have available space on any slot
• if it don't, fails the transfer with an error
• picks an item for that slot and moves away from the vault
• moves the item to the vault
• checks if the vault was the intended destination
• if it was, completes with success
• checks if the destination character inventory has room for the item
• if not, checks if the vault has room for an item
• goes back to the process of juggling items between characters to make room in the vault
• finally moves the item to the final destination

I think considering that a lot can go wrong (eg a request can fail and break the entire predicted state) maybe the performance gain is not worth it

One thing that could be more easily improved tho is creating per slot queues instead of parallelyzing everything, so eg: instead of time --------------------------------------------------------------------------------------------------------> helmet A -> helmet B -> Gauntlet A -> Chest A -> Helmet C -> Leg A -> Cloak A -> Leg B -> Gauntlet B it would be time --------------------------------------------------------------------------------------------------------> helmet A -> helmet B -> helmetC Gauntlet A -> Gauntlet B Chest A Leg A -> Leg B Cloak A

that way, the only weak point would be the vault But still, it needs to be throughly tested to check if it won't piss bungie throttle guard off

[TheBrenny]

Ah, I understand the concurrency and timing issues a bit more now - I didn't think of it as "juggling" items between containers (inv/vault) that are maxed out.

I think the process you've outlined is pretty verbose though, and could be abstracted by identifying the start and end destinations, and then identifying nodes (intermediate destinations) between the two (like a graph), and then each edge is an action (like postmaster -> TO -> character). This is probably sounding a lot like wishful thinking, but I think this idea has merit lol.

(The graph may be missing a few edges - my B-API knowledge isn't too recent)

Using your example and the above diagram, we could put all the "destinations" as nodes of the graph and as part of the _transfer function, we could test if the destination is full, and if it is, move an item out (using _transfer) and potentially return an array of all the items which we moved away for the calling function to move back if/when allowed to do so (ie transferBetween(postmaster, vault) would keep an array of these items that had to move, and move them back when all the futures resolve.

I think my idea of more concurrency got a little lost: I'm advocating for a way to bulk move from one spot to another at the same time. Using the loadout from vault example, it would:

• ensure there's a space clear for each worn piece by making a space if required
• await Future.wait all the worn items to move .then equip (I think this is current behaviour)
• await Future.wait moving away all items which shouldn't be in the Inv at the end of the process (which is calculated per my first response)
• await Future.wait moving all items from wherever they are to the Inv

The state at the end of this process should be predictable and reproducible even if there are errors. For example, if the user did a cheeky and moved an item from the expected source location BEFORE this method moves it, then each Future should handle that independently depending on what it's purpose is. Ie, if a move away item isn't in the Inv anymore and the Future errors saying the item can't be found, then that's fine and we should resolve as a success anyway. Conversly if a loadout item is missing, then we should query the other containers to find out where it is and handle that appropriately - if it's non-existent, then we can delete its reference from the loadout. The point: we can still do things concurrently and handle errors individually rather than on a "mission" scale - just because one future fails, doesn't mean we can't do anything about it...

I can try and quickly prototype something (if the B-API is easier to navigate than historically) and if it doesn't work, then it doesn't work - but I also don't want to double up work because I remember seeing on other threads that you're working on a rewrite?

[joaopmarquesini]

yeah, the WIP of the rework is on https://github.com/LittleLightForDestiny/littlelight/tree/loadout_mods (the branch was meant to be just the loadout mods feature but I've got a bit carried away) Something that I'm considering in that rewrite is adding a full-blown transfer queue, which could solve this issue greatly, and just adding some limitations maybe limiting to 4 (not a definitive number) concurrent transfers AND avoid using the same slots at the same time (eg: if you're moving 4 kinetic weapons from postmaster to vault, they will be moved one at a time, but weapons from different slots can be parallelized)

[joaopmarquesini]

@TheBrenny https://twitter.com/LittleLightD2/status/1622658263937425426 🤪 need some interface tweaks I think, but may work

[TheBrenny]

That looks sick! Maybe what if it was horizontal and always the smaller size and the status is only indicated by the outline only? The words make it too big lol.

[joaopmarquesini]

https://user-images.githubusercontent.com/305022/235559896-f8bef84f-e869-4b58-b79f-b5317d73f4e6.mp4

Already closing as this is getting close to be published