Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#43506 closed defect (bug) (fixed)

Ensure a network's `notoptions` cache is an array to reduce lookup overhead

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-unit-tests has-patch
Focuses: multisite Cc:

Description

In every call to get_network_option() the global {$network_id}:notoptions cache is requested. However the function only sets a value for that cache if both the individual network option's cache lookup and its database lookup failed failed, which in most cases will (and should) not be the case.

This isn't a significant problem, but when using an external object cache, this causes that cache to be called every time get_network_option() is called. If instead get_network_option() ensured an empty array is set on a cache miss, that would only happen once per request, as the local cache would afterwards have that value stored.

It's admittedly a very minor performance improvement, but I think it's worth it, as there should be no downside to it.

Attachments (1)

43506.diff (2.4 KB) - added by flixos90 6 years ago.

Download all attachments as: .zip

Change History (12)

#1 @flixos90
6 years ago

Related: #31147

@flixos90
6 years ago

#2 @flixos90
6 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to flixos90
  • Status changed from new to assigned

43506.diff fixes this by always setting the {$network_id}:notoptions cache key in get_network_option() in case it is not an array. This ensures that on subsequent requests the value is stored in the (local) cache.

Two tests have been added to verify the correct behavior.

#3 @spacedmonkey
6 years ago

I don’t think this is the way we should go with this. At WordCamp US we agreed to go ahead with using the meta api for these functions. See #37181 . Funny enough, the main reason for me to move ahead with using the meta api, is performance. This ticket is no longer an issue once we switch to the meta api as there is no use of the notoption cache key.

Lets close this ticket out, and go ahead with #37181 as agreed.

#4 @flixos90
6 years ago

@spacedmonkey There's still not a 100% agreement if we should move over to using the meta API, so this fix here should go in regardless IMO, since it might take a bit longer for a decision regarding the meta API. It doesn't do any harm, it just fixes a minor issue.

#5 follow-up: @spacedmonkey
6 years ago

@flixos90 At WordCamp US, the meeting of the multisite, team we all agreed that moving to the moving the meta api for network options was a good idea. What we didn't agree to was having the meta function, like get_network_meta in core. This doesn't stop us as from going ahead with #37181 , which does not functionality change anything and can be seen as a simple refactor of the code.

If you spend your time on this ticket, you will have to write unit tests which then will be removed once #37181 goes in. It feels like a massive waste of time. Why not use that time to work on #37181 , write a unit test, that proves that if issue is not a there in that.

If this goes in, then it will delay #37181 , blocking it's release in 5.0.0.

#6 in reply to: ↑ 5 @flixos90
6 years ago

Replying to spacedmonkey:

If you spend your time on this ticket, you will have to write unit tests which then will be removed once #37181 goes in. It feels like a massive waste of time. Why not use that time to work on #37181 , write a unit test, that proves that if issue is not a there in that.

There are two tests already that should be sufficient for this, and it wasn't much work at all to write them.

If this goes in, then it will delay #37181 , blocking it's release in 5.0.0.

Why is it blocking that? It just fixes a small issue with what we have now. Migrating to the meta API is not affected by this change at all.

#7 @flixos90
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42833:

Multisite: Ensure the {$network_id}:notoptions array is set in cache in get_network_option().

Prior to this change, the {$network_id}:notoptions cache would only be fetched, but not set, unless the actual database lookup would be unsuccessful. This enhancement slightly improves performance by preventing unnecessary external object cache lookups if one is used.

Fixes #43506.

#8 @ocean90
6 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 @flixos90
6 years ago

In 42834:

Multisite: Add missing group annotations to tests included in [42833].

This ensures tests are skipped correctly when not using multisite.

See #43506.

#10 @flixos90
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed via the above commit.

#11 @flixos90
5 years ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.