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

Further fix on fastArrayCache has #24292

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 18, 2022

Overview

Further fix on fastArrayCache has

We recently got a good performance boost on accessing the metadata (and other fastArray caches) by improving our 'hit-rate' on intercepting requests at the fast array cache #24156

This adds a much smaller boost in intercepts such that the pattern of

if (Civi::cache('metadata')->has($key)) {
  return Civi::cache('metadata')->get($key);
}

results in one lookup of the underlying cache rather than two.

Before

A call to has on a fastArray cache does not result in the value retrieved from the get request that it does being cached to the array so we wind up with two get requests when we do has followed by get - has seems to call get internally anyway - but not in a way the fastArray decorator can access

1660868589.692973 [0 127.0.0.1:44142] "GET" "crm/metadata_5_51_beta2/CRM_Core_BAO_OptionValue_OptionGroupID_2_en_US"
1660868589.693399 [0 127.0.0.1:44142] "GET" "crm/metadata_5_51_beta2/CRM_Core_BAO_OptionValue_OptionGroupID_2_en_US"

After

Using a get internally results in the value being cached & available for a second call

Technical Details

it's hard to think of when a has would be used without a subsequent get

Comments

This won't impact batch job performance except on the very first cache access - which suggests maybe a 120ms improvement on a single contact save on our infra - that's not a tonne & results may vary - but we have removed queries with far less impact & it's hard to see a downside

@civibot
Copy link

civibot bot commented Aug 18, 2022

(Standard links)

@seamuslee001
Copy link
Contributor

this seems ok to me merging

@seamuslee001 seamuslee001 merged commit 755beb5 into civicrm:master Aug 22, 2022
@seamuslee001 seamuslee001 deleted the fast branch August 22, 2022 01:52
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.

2 participants