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

[Extensive tests required] acf_get_field() performance bottleneck on local fields (~40%) #258

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acf-extended
Copy link
Collaborator

@acf-extended acf-extended commented Nov 30, 2019

Hello,

I was working on a faster alternative to get_field() function when I discovered a performance bottleneck on local fields.

Description:

The problem come from acf_get_field() function, which is called every time get_field() is invoked. What it does for each fields:

  • Look for the field
  • If the field is found (local or not):
    • Apply acf_validate_field()
    • Apply $field['prefix'] = 'acf';
    • Apply acf/load_field filters
    • Push the field into the buffering store('fields') for the next call.

I was able to gain ~40% performance on get_field() calls with local fields by doing so:

If the field is local: Do not process validate, prefix, acf/load_field in acf_get_field().
This process is injected earlier, during the local fields registration, in acf_add_local_fields().

Finally, in acf_get_field(), if the field is local, then simply return it.

I ran a benchmark on local environment, on a page with 200 get_field() / 200 different fields (local PHP field group):

  • 0.0272 sec
  • 0.0308 sec
  • 0.0260 sec

With the fix in this pull request:

  • 0.0179 sec
  • 0.0185 sec
  • 0.0167 sec

I ran some tests on acf/load_field filter, and it works as expected.

Let me know if you need more informations :)

Regards.

Edit: Also performed a benchmark on get_fields() (which also call acf_get_field()):

  • 0.0223 sec.
  • 0.0203 sec.
  • 0.0231 sec.

With the fix:

  • 0.0121 sec.
  • 0.0134 sec.
  • 0.0124 sec.

@elliotcondon
Copy link
Contributor

Hi @acf-extended

Thanks for your contribution.
Elliot here - ACF dev.

The 40% performance boost here is impressive, but also quite confusing. I suspect this boost might be a "false positive" gained by reducing functionality/compatibility. Let me explain.

The "acf/load_field" filter is an extremely important one. It allows complex field types such as the Repeater and Flexible Content to load their sub field, amongst other functionality. It is only ever run once per field, when that field is first loaded, even if that field is "loaded" multiple times.

It is important that this filter is run after the ACF field types have been registered, otherwise, the field type (Repeater / Flex) will not receive their chance to modify the $field before it is placed in temp-cache storage.

I suspect that your PR is unintentionally preventing these classes from modifying the $field variable during "acf/load_field" due to the change in timing. I may be wrong here - as I have not yet tested this code.

If my suspicions are correct, this PR could cause many PHP errors due to incomplete $field properties and missing sub field data.

Logically, it does not seem correct for a "performance boost" to be achieved by running extra functionality on page load. This PR forces the "acf/load_field" filter to be run on every registered field, whether that field is actually loaded on the page. The current implementation would not doe this, which leads me to think this is a "false positive" type situation.

Can you please perform some more testing with this PR?

  • Is there a difference in performance between registering fields in root functions.php file vs "acf/include_fields" action?
  • Does this PR cause individually added fields (acf_add_local_field()) to be lost when the parent field is rendered/loaded?

Thanks mate. Interested in your results :)

@acf-extended
Copy link
Collaborator Author

Hello,

Thanks for taking the time to write some explanations. I'll run extensive tests on complex fields and test the cases you described.

Meanwhile, I rename the PR, and will get back ASAP.

Regards.

@acf-extended acf-extended changed the title Fix get_field() performance bottleneck on local fields (~40%) [Extensive tests required] acf_get_field() performance bottleneck on local fields (~40%) Dec 2, 2019
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.

2 participants