eXaminator / kanka-foundry

FoundryVTT module to import information from kanka.io
MIT License
22 stars 8 forks source link

[BUG] Chrome crashing while importing too many entities at once. #133

Closed SesuUisu closed 2 months ago

SesuUisu commented 2 months ago

In regard of the "time out" issue mentioned in #132:

If the category has to many entities to be imported, it will crash the Chrome tab with Foundry. Most likely because of insufficient memory.

eXaminator commented 2 months ago

I can try to improve the "loading" state, but given the amount of entities you are talking about, the wait time would be really long anyway. The Kanka API has rate limits, based on whether you are paying for Kanka or not. If I remember correctly, the limits are either 60 requests per minute or 90. That being said, the module needs to make at least one request per entity and tries its best to stay within the rate limit. So importing 1000 entities would take at least 10-15 minutes.

Additionally, Foundry itself is not a great fan of having too many objects active in a world, because everything has to be loaded when connecting to the world, which increases load times for all players.

So, with all that in mind (and your bug), I will most likely disable the sync-all button if there are more than maybe 200 entities, just because it wouldn't make a ton of sense with how the API and the module work right now. Anything else would just take too long.

I'll go and ask the Kanka dev if they have any numbers they can share with me so that I maybe can find a good middle ground for performance (aka loading time and memory usage) and the ability for as many Kanka campaigns as possible to import their stuff in one go. But I feel that it's likely that the numbers you mentioned in your other issue (2000 per category) will very likely be outside of the scope, even if it's just because of the rate limit.

SesuUisu commented 2 months ago

The API limits are 30 (free) and 90 (paid). I'm fully aware of that (also on the limitation of Foundrys issues with too many objects). I'm testing the sweet spot for the campaign (and probably will delete those, which are not needed as often).

I tested a bit with the import. The memory is not the issue as the timeout happened over 4.2 GB RAM, but the Foundry Chrome tab couldn't get any keep-alive signal anymore and Chrome asked for "wait or close".

A solution could be to splitt the import in smaller junks. As every entity has an API request, clearing the cache every few hundreds requests and force an interaction of the module with the server should solve that issue. It will not save time, but that not my concern anyway.

eXaminator commented 2 months ago

Right. I'll have to think about ways to improve this properly. I might add a warning if you import a lot of entities in one go and maybe even remove the "link all" button for the whole campaign (at least if you have a lot of entities, it's not THAT much more work to do each category individually).

I also got a few numbers from Kanka and it seems that about 90% of all campaigns (only counted those with 10+ entities, so basically ignoring most "test" campaigns, otherwise these numbers would probably be even higher) have less than 300 entities and 80% have less that 200. Anything beyond 1000 and we're looking at something like 0.5% of all campaigns. And only an even smaller subset of those are probably using Foundry and this module. So I'm hesitant to invest a lot of time to solve / improve this issue for anything beyond maybe 300 entities, especially since this could be detrimental to Foundrys User Experience quickly anyways. So I might be inclined to also disable the "link all" button for categories with a larger number of entities, even if this would probably not be the solution you were hoping for.

But I'll first investigate other ways to maybe improve this and see where we'll end up :)

SesuUisu commented 2 months ago

I can totally understand, if you just disable the button for crazy people like me 😄

If I can support you in anyway, please let me know.

eXaminator commented 2 months ago

I have just released an update that should reduce the necessary amount of requests by a lot and thus improve the speed when importing a lot of entities at once. Please check if this improves your use case to a usable amount or if I still need to add limits somewhere.

SesuUisu commented 2 months ago

Thanks for the fast update! It works perfect now.

Some data from my Kanka world / Foundry server: It just needed a couple of minutes longer than a "perfect" run of requests (runtime of 1:16h instead of 1:08h for a "perfect" run - 2037 entities with 30 requests per minute, so my runtime had app. 27 requests per minute). The memory usage was by far lower (RAM maxed at 1.8 GB - previously a time out happened at 4.2 GB). The ping was most of the time between 2000ms and 3200ms.

In total I can say that I don't think you could improve this module much in regard of huge Kanka worlds.

I will see, if I will keep all the entities in Foundry. But first I will need some tests, how bad the performance will be with this amount of data :D

eXaminator commented 2 months ago

I theory it should use less requests now because I use the list endpoints to load all entities of a type at once (paginated). If I remember correctly you can change the number of entities per page in your regular kanka settings which will carry over to the API (it's not something I can change in the module), where the largest amount would be 45 entities per page. So in theory with 2037 entities it should use 1 request to load your campaigns when opening the KankaBrowser, then 46 requests to load the list of all entities. From there it depends on how the entities are split over the different categories. But if ~20% of entities are characters and another 20% locations and the rest spreads equally over the remaining 8 categories, we would land at another 52 requests. Which means – in theory – the whole import could be done in about 2-3 minutes (52 requests / 30, though it would depend on how many "slots" are free from the previous population of the KankaBrowser list).

But I will close this issue now as I think the situation is improved enough. I'll think about whether I can show some warning somewhere if the pagination is set below 45 (which probably isn't the default in Kanka).

eXaminator commented 2 months ago

I just want to correct my previous comment by saying that the API seemingly always uses a page size of 45, independent of the users setting.