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

admin2: cache meta settings #162

Closed
Dezash opened this issue Jul 20, 2019 · 8 comments
Closed

admin2: cache meta settings #162

Dezash opened this issue Jul 20, 2019 · 8 comments

Comments

@Dezash
Copy link
Contributor

Dezash commented Jul 20, 2019

Is your feature request related to a problem? Please describe.
Currently admin2 does not cache any of its meta settings so it is required to call the get function each time the setting is read.
#161 adds new settings so it would be good to look into this issue first.

Describe the solution you'd like
Implement a system that would cache the settings and update them onSettingChange.

Describe alternatives you've considered

  • Keep calling get function each time the setting is read (may downgrade server performance).
  • Replace meta settings with a different system (database, XML files, etc.)

Additional context
This was already implemented in the old admin resource:
https://github.com/multitheftauto/mtasa-resources/blob/master/%5Badmin%5D/admin/server/admin_serverprefs.lua
It could be improved further by adding 'serveronly' attribute so not all settings are sent to the client.

@PlatinMTA
Copy link
Contributor

I don't think that caching the settings is really worth it. Wouldn't the admin need to restart the resource every time he changed some setting? That would kinda suck.

@qaisjp
Copy link
Contributor

qaisjp commented Aug 6, 2019

Wouldn't the admin need to restart the resource every time he changed some setting?

No

@jlillis
Copy link
Contributor

jlillis commented Nov 10, 2020

A PR (#198) which would have addressed this issue was just closed. I think we should also consider marking this issue as wontfix and closing it - alternative 1 (doing nothing) is fine for now unless someone can provide evidence that performance will be noticeably impacted by not implementing a settings cache for the admin resource.

@Dezash
Copy link
Contributor Author

Dezash commented Nov 11, 2020

get function is very slow since it parses meta.xml every time, so it really comes down to how often it will be used
afaik, admin resource checks the settings for almost every action so that's probably why caching was used there

@jlillis
Copy link
Contributor

jlillis commented Nov 13, 2020

get function is very slow since it parses meta.xml every time

1: How slow are we talking?
2: If it's really bad, perhaps the function should be optimized in mtasa-blue instead so it benefits all resources that use the settings system instead.

@Pirulax
Copy link
Contributor

Pirulax commented Nov 13, 2020

Its really that slow, yes.
The reason is parses the xml every time is that one might change meta.xml form the outside, and we need to report that change.
But, im pretty sure theres some kind of WinAPI and Linux hook which calls a function every time a file is changed, since modern IDE/Code editors do detect external file changes.
Please open an issue over at mtasa-blue

@qaisjp
Copy link
Contributor

qaisjp commented Nov 13, 2020

Maybe we should stop supporting live setting changes in meta.xml files in mtasa-blue. The correct way is to use the settings system. meta.xml should only define the settings schema (and defaults). Where does the server store changes to settings via set right now?

@jlillis
Copy link
Contributor

jlillis commented Jan 28, 2021

I'm going to close this because
1: admin does not cache meta settings, and our focus is currently on feature parity
2: this will become unnecessary if/when multitheftauto/mtasa-blue#1863 is resolved

If there are any major objections, feel free to comment here.

@jlillis jlillis closed this as completed Jan 28, 2021
@patrikjuvonen patrikjuvonen closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2022
@jlillis jlillis added this to admin2 Jul 11, 2024
@jlillis jlillis moved this to To explore in admin2 Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants