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

Add async versions of functions #25

Open
not-an-aardvark opened this issue Feb 17, 2017 · 4 comments
Open

Add async versions of functions #25

not-an-aardvark opened this issue Feb 17, 2017 · 4 comments
Assignees

Comments

@not-an-aardvark
Copy link
Contributor

So I realized that whenever we load data that we don't already have cached, we call require on a new file, which does synchronous filesystem IO and blocks the event loop. This prevents anything else from happening (e.g. requests to the server) while that's going on.

We should probably add versions of the functions that load things asynchronously, and use those instead.

@AlMcKinlay
Copy link
Contributor

So, do you have an example of this? I'm not sure I'm 100% sure what you are talking about.

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Mar 1, 2017

Suppose I want to add parsed data to a pokemon in the database (which the porybox server has to do quite a lot, often multiple times per request). In order to retrieve the data, pkparse needs to load the following files:

  • 1 file with data for the species
  • 4 files with data for the moves
  • 1 file with data for the ability
  • 1 file with data for the region
  • 1 file with data for the nature
  • 1 file with data for the item

All of these file accesses use require, which is synchronous. So the server is frozen and literally can't do anything else while pkparse loads 9 files from the filesystem (or for a single box page, 270 files). This is a serious performance issue.

It's mitigated slightly by the fact that require caches things, so if the server is up for long enough it will eventually hold the files it needs in its cache. But we have a lot of separate data files, so this takes awhile.

@AlMcKinlay
Copy link
Contributor

Ah yes, that makes sense.

1 thing we could to that would be simple (but would increase ram usage in the short term) would be using require all to get node to cache all the files. It's a bit of a hack, but it would at least remove the issue partially right now, with very little change.

@Raia
Copy link
Member

Raia commented Dec 3, 2017

@kamronbatman Assigning you here, as for the actual implementation details I'll let you work that out with @not-an-aardvark ✌️

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

No branches or pull requests

4 participants