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

Inefficient use of database #36

Open
hubertnnn opened this issue Nov 29, 2018 · 2 comments
Open

Inefficient use of database #36

hubertnnn opened this issue Nov 29, 2018 · 2 comments

Comments

@hubertnnn
Copy link

I saw that achievement generation is quite inefficient when it comes to database usage.
The culprit is getModel method that is called 3 times for each achievement progress modification:

  • in constructor
  • in getOrCreateProgressForAchiever to fetch achievement id
  • in getOrCreateProgressForAchiever to associate newly created progress with details model

Then in this getModel method we have 2 database calls: read and save
That makes 6 database calls (8 if there is an update) for each achievement update.

My suggestion would be to make a dedicated command for synchronizing achievement details with database and remove save call from getModel as well as getModel call from constructor (possibly as a config option for backword compatibility) .

An extra improvement would be to reuse results of getModel instead of calling it each time.

Another suggestion would be to add an option for having stable ids
(eg. calculated from md5 of achievement fully qualified class name or just provided by user)
and assume that those will match model's id,
but that is something to be discussed
and since its a super breaking change, definitely require a config option.

@gabs-simon
Copy link
Contributor

Hi @hubertnnn ! Thanks a lot for the feedback. I 100% agree that the current base is not as efficient as it could be, and will be taking a look at this for the next release (scheduled for January).

If you would like to help, please submit a pull request!

@gabs-simon
Copy link
Contributor

Hi @hubertnnn !

The latest version on master implements your suggested changes.

1faf09e

Please let me know if this change works for you!

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

2 participants