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

Load JSON files from folder #27

Merged
merged 4 commits into from
Jan 25, 2021
Merged

Load JSON files from folder #27

merged 4 commits into from
Jan 25, 2021

Conversation

jmigual
Copy link
Contributor

@jmigual jmigual commented Dec 6, 2020

Fixes #8. Basically what it does is:

  • Load all JSON files from a folder and store them by file name to avoid loading it multiple times.
  • Check if the title field of any loaded JSON file for the folder matches the file name
  • If so return the JSON file

I am a bit concerned though about the memory usage that this may cause so suggestions are welcome.

@TheLastGimbus
Copy link
Owner

TheLastGimbus commented Dec 7, 2020

Hm, so I see you did some caching of jsons - could you try to measure perfomance with caching vs just checking every json file inside folder? Maybe it's not that different, and we could simplify this code...

Ps. I edited the description, because we will get this to fix it, not just try 💪

@jmigual
Copy link
Contributor Author

jmigual commented Dec 9, 2020

Sure, I'll try to do it during the weekend 👍

@TheLastGimbus
Copy link
Owner

Okay, so I tested this on my small 4gb sample and:

  • number of "json not found" went from ~450 to 435 - some files just don't have json 🤷
  • memory usage is sometimes same, sometimes raises form 40mb originally to 70mb - even x10, 700mb is not that bad 😅
  • time is identical
  • nothing bad changed - number of copied files is same, removed dups count is same - everything alright 👍

Thanks for this, I'm sending it to people to test it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON naming too long?
2 participants