r98878 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r98877‎ | r98878 | r98879 >
Date:18:00, 4 October 2011
Author:catrope
Status:ok
Tags:
Comment:
[RL2] Followup r96837, prevent requests for nonexistent modules from polluting the gadget names cache by caching nonexistence separately. For more details about the bug this was causing, see CR comments on r96837.

* Introduce LocalGadgetRepo::$missCache that caches nonexistence of gadgets
* Use $data for caching existing gadgets only
* In memcached, continue to cache nonexistence by storing an empty array. This requires some logic to ensure the right object member is updated
* Document the in-object caching vars
* Fix clearInObjectCache() to really clear everything
* When deleting a gadget, set its cache entry to an empty array rather than removing it
* Update $missCache for modifications and deletions too
* Add an optimization where all gadgets that aren't in $data are (rightly) assumed no to exist if $idsLoaded is true
Modified paths:
  • /branches/RL2/extensions/Gadgets/backend/LocalGadgetRepo.php (modified) (history)

Diff [purge]

Index: branches/RL2/extensions/Gadgets/backend/LocalGadgetRepo.php
@@ -3,7 +3,22 @@
44 * Gadget repository that gets its gadgets from the local database.
55 */
66 class LocalGadgetRepo extends GadgetRepo {
 7+ /** Cache for EXISTING gadgets. Nonexistent gadgets must not be cached here,
 8+ * use $missCache instead. Values may be null, in which case only the gadget's
 9+ * existence is cached and the data must still be retrieved from memc or the DB.
 10+ *
 11+ * array( id => null|array( 'json' => JSON blob, 'timestamp' => timestamp ) )
 12+ */
713 protected $data = array();
 14+
 15+ /** Cache for gadget IDs that have been queried and found to be nonexistent.
 16+ *
 17+ * array( id => ignored )
 18+ */
 19+ protected $missCache = array();
 20+
 21+ /** If true, $data is assumed to contain all existing gadget IDs.
 22+ */
823 protected $idsLoaded = false;
924
1025 /** Memcached key of the gadget names list. Subclasses may override this in their constructor.
@@ -54,7 +69,9 @@
5570 }
5671
5772 public function clearInObjectCache() {
58 - $this->data = null;
 73+ $this->data = array();
 74+ $this->missCache = array();
 75+ $this->idsLoaded = false;
5976 }
6077
6178 public function isWriteable() {
@@ -108,6 +125,8 @@
109126 // a clone. If it returned a reference to a cached object, the caller could change
110127 // that object and cause weird things to happen.
111128 $this->data[$id] = array( 'json' => $json, 'timestamp' => $newTs );
 129+ // Remove from the missing cache if present there
 130+ unset( $this->missCache[$id] );
112131 // Write to memc too
113132 $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
114133 if ( $key !== false ) {
@@ -134,12 +153,14 @@
135154
136155 // Remove gadget from in-object cache
137156 unset( $this->data[$id] );
138 - // Remove from memc too
 157+ // Add it to the missing cache
 158+ $this->missCache[$id] = true;
 159+ // Store nonexistence in memc too
139160 $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
140161 if ( $key !== false ) {
141 - $wgMemc->delete( $key );
 162+ $wgMemc->set( $key, array() );
142163 }
143 - // Clear the gadget names array in memc
 164+ // Clear the gadget names array in memc so it'll be regenerated later
144165 if ( $this->namesKey !== false ) {
145166 $wgMemc->delete( $this->namesKey );
146167 }
@@ -253,13 +274,30 @@
254275 // Already loaded, nothing to do here.
255276 return $this->data[$id];
256277 }
 278+ if ( isset( $this->missCache[$id] ) ) {
 279+ // Gadget is already known to be missing
 280+ return array();
 281+ }
 282+ // Need to use array_key_exists() here because isset() returns true for nulls. !@#$ you, PHP
 283+ if ( $this->idsLoaded && !array_key_exists( $id, $this->data ) ) {
 284+ // All IDs have been loaded into $this->data but $id isn't in there,
 285+ // therefore it doesn't exist.
 286+ $this->missCache[$id] = true;
 287+ return array();
 288+ }
257289
258290 // Try cache
259291 $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
260292 $cached = $key !== false ? $wgMemc->get( $key ) : false;
261293 if ( is_array( $cached ) ) {
262294 // Yay, data is in cache
263 - $this->data[$id] = $cached;
 295+ if ( count( $cached ) ) {
 296+ // Cache entry contains data
 297+ $this->data[$id] = $cached;
 298+ } else {
 299+ // Cache entry signals nonexistence
 300+ $this->missCache[$id] = true;
 301+ }
264302 return $cached;
265303 }
266304
@@ -273,11 +311,13 @@
274312 // Gadget doesn't exist
275313 // Use empty array to prevent confusion with $wgMemc->get() return values for missing keys
276314 $data = array();
 315+ // DO NOT store this in $this->data, because it's supposed to contain existing gadgets only
 316+ $this->missCache[$id] = true;
277317 } else {
278318 $data = array( 'json' => $row->gd_blob, 'timestamp' => $row->gd_timestamp );
 319+ // Save to object cache
 320+ $this->data[$id] = $data;
279321 }
280 - // Save to object cache
281 - $this->data[$id] = $data;
282322 // Save to memc
283323 $wgMemc->set( $key, $data );
284324

Follow-up revisions

RevisionCommit summaryAuthorDate
r98879[RL2] Comment edit (Follows-up r98878). Using string instead of blob, like lo...krinkle18:08, 4 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r96837RL2: Reorganize caching in LocalGadgetRepo so we no longer load the entire ga...catrope13:38, 12 September 2011

Status & tagging log