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 cached GLOBAL_GET macros #60926

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented May 10, 2022

Instead of querying ProjectSettings fully each time, these macros first check a static local version against ProjectSettings. If the version has changed, a static local variable is updated and returned. If the version is up to date, the cached static local variable is returned.

The idea of this PR is to decrease the cost of the rather heavyweight calls to GLOBAL_GET in the codebase.

I'd already added a couple of other methods previously (the project_settings_changed signal etc) in #53296 , and that still represents the recommended way to check project settings in bulk. But this PR adds a slightly more efficient variation of the standard GLOBAL_GET, which can be fine in some circumstances and requires less boiler plate.

Notes

  • If we agree on this technique I can do equivalent PR for 4.x.
  • I'm a complete newbie to lambdas, and I shamelessly ripped off this approach from the SNAME macro in Godot 4, so could do with checking by someone who properly understands this voodoo and actually knows what they are doing.
  • There's probably a few more places that might be worth using the FAST variation, but the PR is kept as simple as possible for bikeshedding.
  • Tested memory use doesn't seem significantly different between the build before and after this PR (it is probably only a couple of Kb of static memory, unless the macros were used e.g. in a recursive situation).
  • Welcome ideas on improvements for naming.
  • This should actually be thread safe (providing the ProjectSettings::get() call is thread safe, which is outside the scope of this PR), despite there being no atomics etc. The only changes to _version are made in _set() which is a _THREAD_SAFE_METHOD. Reading only needs to detect a difference, even if the local version (in the macro) is mangled for 1 frame, the worst that can happen seems to a read of the project setting on 2 adjacent frames. So it doesn't seem worth making _version atomic, or having locks on _version.

@lawnjelly lawnjelly requested review from a team as code owners May 10, 2022 14:55
@lawnjelly lawnjelly added this to the 3.5 milestone May 10, 2022
core/project_settings.h Outdated Show resolved Hide resolved
Instead of querying ProjectSettings fully each time, these macros first check a static local version against ProjectSettings. If the version has changed, a static local variable is updated and returned. If the version is up to date, the cached static local variable is returned.
@lawnjelly lawnjelly force-pushed the global_get_cached branch from 772015d to d4a5329 Compare May 10, 2022 16:04
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jun 28, 2022
@akien-mga
Copy link
Member

We discussed this in a PR review meeting today and @reduz wasn't convinced by the approach. One mentioned issue would be that using this all the time would force keeping a global version of each setting which would stay until the process is killed and possibly cause issues for some of the Variant types involved (IIUC).

Overall we should assess what places would really benefit from something like this, otherwise the recommended approach is still to retrieve the value once in a constructor (and only update based on project settings change if/when needed on a case by case basis).

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

Successfully merging this pull request may close these issues.

5 participants