r101469 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r101468‎ | r101469 | r101470 >
Date:13:13, 1 November 2011
Author:catrope
Status:deferred
Tags:
Comment:
[RL2] Factor the caching logic out of LocalGadgetRepo into CachedGadgetRepo, which is between GadgetRepo and LocalGadgetRepo in the inheritance tree. This explicitly splits the caching logic from the DB logic, which makes the code nicer, but it also allows ForeignAPIGadgetRepo (which I will implement very soon) to share the caching code.

* CachedGadgetRepo implements getGadgetIDs() and getGadget(), and requires that the subclass implement loadDataFor() and loadAllData()
** loadDataFor() was present in LocalGadgetRepo before but has been repurposed to only do the DB query, the caching logic has been moved out
** Ditto for loadAllData(), except that it was previously called loadIDs()
* Moved cache update logic for modifications and deletions to CachedGadgetRepo::updateCache()
* Renamed getLoadIDsQuery() to getLoadAllDataQuery() in LocalGadgetRepo and ForeignDBGadgetRepo
* Removed GadgetRepo::clearInObjectCache(), was unused
* TODO: the memc key naming scheme and the position of getMemcKey() need to be reconsidered
Modified paths:
  • /branches/RL2/extensions/Gadgets/Gadgets.php (modified) (history)
  • /branches/RL2/extensions/Gadgets/backend/CachedGadgetRepo.php (added) (history)
  • /branches/RL2/extensions/Gadgets/backend/ForeignDBGadgetRepo.php (modified) (history)
  • /branches/RL2/extensions/Gadgets/backend/GadgetRepo.php (modified) (history)
  • /branches/RL2/extensions/Gadgets/backend/LocalGadgetRepo.php (modified) (history)

Diff [purge]

Index: branches/RL2/extensions/Gadgets/Gadgets.php
——@@ -126,6 +126,7 @@
127127 $wgAutoloadClasses['ApiQueryGadgets'] = $dir . 'api/ApiQueryGadgets.php';
128128 $wgAutoloadClasses['ApiGetGadgetPrefs'] = $dir . 'api/ApiGetGadgetPrefs.php';
129129 $wgAutoloadClasses['ApiSetGadgetPrefs'] = $dir . 'api/ApiSetGadgetPrefs.php';
 130+$wgAutoloadClasses['CachedGadgetRepo'] = $dir . 'backend/CachedGadgetRepo.php';
130131 $wgAutoloadClasses['ForeignDBGadgetRepo'] = $dir . 'backend/ForeignDBGadgetRepo.php';
131132 $wgAutoloadClasses['Gadget'] = $dir . 'backend/Gadget.php';
132133 $wgAutoloadClasses['GadgetsHooks'] = $dir . 'Gadgets.hooks.php';
Index: branches/RL2/extensions/Gadgets/backend/CachedGadgetRepo.php
——@@ -0,0 +1,194 @@
 2+<?php
 3+// TODO: Reconsider memc key naming
 4+/**
 5+ * Abstract class that enhances GadgetRepo with caching. This is useful for
 6+ * repos that obtain gadget data from a database or a REST API. Currently, all
 7+ * repos use this.
 8+ */
 9+abstract class CachedGadgetRepo extends GadgetRepo {
 10+ /*** Protected members ***/
 11+
 12+ /** Cache for EXISTING gadgets. Nonexistent gadgets must not be cached here,
 13+ * use $missCache instead. Values may be null, in which case only the gadget's
 14+ * existence is cached and the data must still be retrieved from memc or the DB.
 15+ *
 16+ * array( id => null|array( 'json' => JSON string, 'timestamp' => timestamp ) )
 17+ */
 18+ protected $data = array();
 19+
 20+ /** Cache for gadget IDs that have been queried and found to be nonexistent.
 21+ *
 22+ * array( id => ignored )
 23+ */
 24+ protected $missCache = array();
 25+
 26+ /** If true, $data is assumed to contain all existing gadget IDs.
 27+ */
 28+ protected $idsLoaded = false;
 29+
 30+ /**
 31+ * Memcached key of the gadget names list. Subclasses must set this in their constructor.
 32+ * This could've been a static member if we had PHP 5.3's late static binding.
 33+ *
 34+ * Set to false if memcached is not available.
 35+ *
 36+ * TODO: make this an abstract getter, and redesign the whole memc key naming scheme
 37+ */
 38+ protected $namesKey;
 39+
 40+ /*** Abstract methods ***/
 41+
 42+ /**
 43+ * Load the full data for all gadgets.
 44+ * @return array( id => array( 'json' => JSON string, 'timestamp' => timestamp ) )
 45+ */
 46+ abstract protected function loadAllData();
 47+
 48+ /**
 49+ * Load the full data for one gadget.
 50+ * @param $id string Gadget ID
 51+ * @return array( 'json' => JSON string, 'timestamp' => timestamp ) or empty array if the gadget doesn't exist.
 52+ */
 53+ abstract protected function loadDataFor( $id );
 54+
 55+ /*** Protected methods ***/
 56+
 57+ /**
 58+ * Update the cache to account for the fact that a gadget has been
 59+ * added, modified or deleted.
 60+ * @param $id string Gadget ID
 61+ * @param $data array array( 'json' => JSON string, 'timestamp' => timestamp ) if added or modified, or null if deleted
 62+ */
 63+ protected function updateCache( $id, $data ) {
 64+ global $wgMemc;
 65+ $toCache = $data;
 66+ if ( $data !== null ) {
 67+ // Added or modified
 68+ // Store in in-object cache
 69+ $this->data[$id] = $data;
 70+ // Remove from the missing cache if present there
 71+ unset( $this->missCache[$id] );
 72+ } else {
 73+ // Deleted
 74+ // Remove from in-object cache
 75+ unset( $this->data[$id] );
 76+ // Add it to the missing cache
 77+ $this->missCache[$id] = true;
 78+ // Store nonexistence in memc as an empty array
 79+ $toCache = array();
 80+ }
 81+
 82+ // Write to memc
 83+ $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
 84+ if ( $key !== false ) {
 85+ $wgMemc->set( $key, $toCache );
 86+ }
 87+ // Clear the gadget names array in memc so it'll be regenerated later
 88+ if ( $this->namesKey !== false ) {
 89+ $wgMemc->delete( $this->namesKey );
 90+ }
 91+ }
 92+
 93+ /*** Public methods inherited from GadgetRepo ***/
 94+
 95+ public function getGadgetIds() {
 96+ $this->maybeLoadIDs();
 97+ return array_keys( $this->data );
 98+ }
 99+
 100+ public function getGadget( $id ) {
 101+ $data = $this->maybeLoadDataFor( $id );
 102+ if ( !$data ) {
 103+ return null;
 104+ }
 105+ return new Gadget( $id, $this, $data['json'], $data['timestamp'] );
 106+ }
 107+
 108+ /*** Private methods ***/
 109+
 110+ /**
 111+ * Populate the keys in $this->data. Values are only populated if loadAllData() is called,
 112+ * when loading from memc, all values are set to null and are lazy-loaded in loadDataFor().
 113+ * @return array Array of gadget IDs
 114+ */
 115+ private function maybeLoadIDs() {
 116+ global $wgMemc;
 117+ if ( $this->idsLoaded ) {
 118+ return array_keys( $this->data );
 119+ }
 120+
 121+ // Try memc
 122+ $cached = $this->namesKey !== false ? $wgMemc->get( $this->namesKey ) : false;
 123+ if ( is_array( $cached ) ) {
 124+ // Yay, data is in cache
 125+ // Add to $this->data , but let things already in $this->data take precedence
 126+ $this->data += $cached;
 127+ $this->idsLoaded = true;
 128+ return array_keys( $this->data );
 129+ }
 130+
 131+ $this->data = $this->loadAllData();
 132+ // For memc, prepare an array with the IDs as keys but with each value set to null
 133+ $toCache = array_combine( array_keys( $data ), array_fill( 0, count( $this->data ), null ) );
 134+
 135+ $wgMemc->set( $this->namesKey, $toCache );
 136+ $this->idsLoaded = true;
 137+ return array_keys( $this->data );
 138+ }
 139+
 140+ /**
 141+ * Populate a given Gadget's data in $this->data . Tries memc first, then falls back to loadDataFor()
 142+ * @param $id string Gadget ID
 143+ * @return array( 'json' => JSON string, 'timestamp' => timestamp ) or empty array if the gadget doesn't exist.
 144+ */
 145+ private function maybeLoadDataFor( $id ) {
 146+ global $wgMemc;
 147+ if ( isset( $this->data[$id] ) && is_array( $this->data[$id] ) ) {
 148+ // Already loaded, nothing to do here.
 149+ return $this->data[$id];
 150+ }
 151+ if ( isset( $this->missCache[$id] ) ) {
 152+ // Gadget is already known to be missing
 153+ return array();
 154+ }
 155+ // Need to use array_key_exists() here because isset() returns true for nulls. !@#$ you, PHP
 156+ if ( $this->idsLoaded && !array_key_exists( $id, $this->data ) ) {
 157+ // All IDs have been loaded into $this->data but $id isn't in there,
 158+ // therefore it doesn't exist.
 159+ $this->missCache[$id] = true;
 160+ return array();
 161+ }
 162+
 163+ // Try cache
 164+ // FIXME getMemcKey is defined in LocalGadgetRepo but not abstract. Cache key naming needs redesign
 165+ $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
 166+ $cached = $key !== false ? $wgMemc->get( $key ) : false;
 167+ if ( is_array( $cached ) ) {
 168+ // Yay, data is in cache
 169+ if ( count( $cached ) ) {
 170+ // Cache entry contains data
 171+ $this->data[$id] = $cached;
 172+ } else {
 173+ // Cache entry signals nonexistence
 174+ $this->missCache[$id] = true;
 175+ }
 176+ return $cached;
 177+ }
 178+
 179+ $data = $this->loadDataFor( $id );
 180+ if ( !$data ) {
 181+ // Gadget doesn't exist
 182+ // Use empty array to prevent confusion with $wgMemc->get() return values for missing keys
 183+ $data = array();
 184+ // DO NOT store $data in $this->data, because it's supposed to contain existing gadgets only
 185+ $this->missCache[$id] = true;
 186+ } else {
 187+ // Save to object cache
 188+ $this->data[$id] = $data;
 189+ }
 190+ // Save to memc
 191+ $wgMemc->set( $key, $data );
 192+
 193+ return $data;
 194+ }
 195+}
\ No newline at end of file
Property changes on: branches/RL2/extensions/Gadgets/backend/CachedGadgetRepo.php
___________________________________________________________________
Added: svn:eol-style
1196 + native
Index: branches/RL2/extensions/Gadgets/backend/LocalGadgetRepo.php
——@@ -2,30 +2,8 @@
33 /**
44 * Gadget repository that gets its gadgets from the local database.
55 */
6 -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 string, 'timestamp' => timestamp ) )
12 - */
13 - protected $data = array();
 6+class LocalGadgetRepo extends CachedGadgetRepo {
147
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 - */
23 - protected $idsLoaded = false;
24 -
25 - /** Memcached key of the gadget names list. Subclasses may override this in their constructor.
26 - * This could've been a static member if we had PHP 5.3's late static binding.
27 - */
28 - protected $namesKey;
29 -
308 /*** Public static methods ***/
319
3210 /**
——@@ -40,6 +18,35 @@
4119 return $instance;
4220 }
4321
 22+ /*** Protected methods inherited from CachedGadgetRepo ***/
 23+
 24+ protected function loadAllData() {
 25+ $query = $this->getLoadAllDataQuery();
 26+ $dbr = $this->getDB();
 27+ $res = $dbr->select( $query['tables'], $query['fields'], $query['conds'], __METHOD__,
 28+ $query['options'], $query['join_conds'] );
 29+
 30+ $data = array();
 31+ foreach ( $res as $row ) {
 32+ $data[$row->gd_id] = array( 'json' => $row->gd_blob, 'timestamp' => $row->gd_timestamp );
 33+ }
 34+ return $data;
 35+ }
 36+
 37+ protected function loadDataFor( $id ) {
 38+ $query = $this->getLoadDataForQuery( $id );
 39+ $dbr = $this->getDB();
 40+ $row = $dbr->selectRow( $query['tables'], $query['fields'], $query['conds'], __METHOD__,
 41+ $query['options'], $query['join_conds']
 42+ );
 43+ if ( !$row ) {
 44+ // Gadget doesn't exist
 45+ return array();
 46+ } else {
 47+ return array( 'json' => $row->gd_blob, 'timestamp' => $row->gd_timestamp );
 48+ }
 49+ }
 50+
4451 /*** Public methods inherited from GadgetRepo ***/
4552
4653 /**
——@@ -51,29 +58,10 @@
5259 $this->namesKey = $this->getMemcKey( 'gadgets', 'localreponames' );
5360 }
5461
55 - public function getGadgetIds() {
56 - $this->loadIDs();
57 - return array_keys( $this->data );
58 - }
59 -
60 - public function getGadget( $id ) {
61 - $data = $this->loadDataFor( $id );
62 - if ( !$data ) {
63 - return null;
64 - }
65 - return new Gadget( $id, $this, $data['json'], $data['timestamp'] );
66 - }
67 -
6862 public function getSource() {
6963 return 'local';
7064 }
7165
72 - public function clearInObjectCache() {
73 - $this->data = array();
74 - $this->missCache = array();
75 - $this->idsLoaded = false;
76 - }
77 -
7866 public function isWriteable() {
7967 return true;
8068 }
——@@ -119,23 +107,7 @@
120108 return Status::newFatal( 'gadgets-manager-modify-conflict', $id, $ts );
121109 }
122110
123 - // Update our in-object cache
124 - // This looks stupid: we have an object that we could be caching. But I prefer
125 - // to keep $this->data in a consistent format and have getGadget() always return
126 - // a clone. If it returned a reference to a cached object, the caller could change
127 - // that object and cause weird things to happen.
128 - $this->data[$id] = array( 'json' => $json, 'timestamp' => $newTs );
129 - // Remove from the missing cache if present there
130 - unset( $this->missCache[$id] );
131 - // Write to memc too
132 - $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
133 - if ( $key !== false ) {
134 - $wgMemc->set( $key, $this->data[$id] );
135 - }
136 - // Clear the gadget names array in memc
137 - if ( $this->namesKey !== false ) {
138 - $wgMemc->delete( $this->namesKey );
139 - }
 111+ $this->updateCache( $id, array( 'json' => $json, 'timestamp' => $newTs ) );
140112
141113 return Status::newGood();
142114 }
——@@ -151,19 +123,7 @@
152124 $dbw->delete( 'gadgets', array( 'gd_id' => $id ), __METHOD__ );
153125 $affectedRows = $dbw->affectedRows();
154126
155 - // Remove gadget from in-object cache
156 - unset( $this->data[$id] );
157 - // Add it to the missing cache
158 - $this->missCache[$id] = true;
159 - // Store nonexistence in memc too
160 - $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
161 - if ( $key !== false ) {
162 - $wgMemc->set( $key, array() );
163 - }
164 - // Clear the gadget names array in memc so it'll be regenerated later
165 - if ( $this->namesKey !== false ) {
166 - $wgMemc->delete( $this->namesKey );
167 - }
 127+ $this->updateCache( $id, null );
168128
169129 if ( $affectedRows === 0 ) {
170130 return Status::newFatal( 'gadgets-manager-nosuchgadget', $id );
——@@ -229,110 +189,10 @@
230190 }
231191
232192 /**
233 - * Populate the keys in $this->data. Values are only populated when loading from the DB;
234 - * when loading from memc, all values are set to null and are lazy-loaded in loadDataFor().
235 - * @return array Array of gadget IDs
236 - */
237 - protected function loadIDs() {
238 - global $wgMemc;
239 - if ( $this->idsLoaded ) {
240 - // Already loaded
241 - return array_keys( $this->data );
242 - }
243 -
244 - // Try memc
245 - $cached = $this->namesKey !== false ? $wgMemc->get( $this->namesKey ) : false;
246 - if ( is_array( $cached ) ) {
247 - // Yay, data is in cache
248 - // Add to $this->data , but let things already in $this->data take precedence
249 - $this->data += $cached;
250 - $this->idsLoaded = true;
251 - return array_keys( $this->data );
252 - }
253 -
254 - // Get from DB
255 - $query = $this->getLoadIDsQuery();
256 - $dbr = $this->getDB();
257 - $res = $dbr->select( $query['tables'], $query['fields'], $query['conds'], __METHOD__,
258 - $query['options'], $query['join_conds'] );
259 -
260 - $toCache = array();
261 - foreach ( $res as $row ) {
262 - $this->data[$row->gd_id] = array( 'json' => $row->gd_blob, 'timestamp' => $row->gd_timestamp );
263 - $toCache[$row->gd_id] = null;
264 - }
265 - // Write to memc
266 - $wgMemc->set( $this->namesKey, $toCache );
267 - $this->idsLoaded = true;
268 - return array_keys( $this->data );
269 - }
270 -
271 - /**
272 - * Populate a given Gadget's data in $this->data . Tries memc first, then falls back to a DB query.
273 - * @param $id string Gadget ID
274 - * @return array( 'json' => JSON string, 'timestamp' => timestamp ) or empty array if the gadget doesn't exist.
275 - */
276 - protected function loadDataFor( $id ) {
277 - global $wgMemc;
278 - if ( isset( $this->data[$id] ) && is_array( $this->data[$id] ) ) {
279 - // Already loaded, nothing to do here.
280 - return $this->data[$id];
281 - }
282 - if ( isset( $this->missCache[$id] ) ) {
283 - // Gadget is already known to be missing
284 - return array();
285 - }
286 - // Need to use array_key_exists() here because isset() returns true for nulls. !@#$ you, PHP
287 - if ( $this->idsLoaded && !array_key_exists( $id, $this->data ) ) {
288 - // All IDs have been loaded into $this->data but $id isn't in there,
289 - // therefore it doesn't exist.
290 - $this->missCache[$id] = true;
291 - return array();
292 - }
293 -
294 - // Try cache
295 - $key = $this->getMemcKey( 'gadgets', 'localrepodata', $id );
296 - $cached = $key !== false ? $wgMemc->get( $key ) : false;
297 - if ( is_array( $cached ) ) {
298 - // Yay, data is in cache
299 - if ( count( $cached ) ) {
300 - // Cache entry contains data
301 - $this->data[$id] = $cached;
302 - } else {
303 - // Cache entry signals nonexistence
304 - $this->missCache[$id] = true;
305 - }
306 - return $cached;
307 - }
308 -
309 - // Get from database
310 - $query = $this->getLoadDataForQuery( $id );
311 - $dbr = $this->getDB();
312 - $row = $dbr->selectRow( $query['tables'], $query['fields'], $query['conds'], __METHOD__,
313 - $query['options'], $query['join_conds']
314 - );
315 - if ( !$row ) {
316 - // Gadget doesn't exist
317 - // Use empty array to prevent confusion with $wgMemc->get() return values for missing keys
318 - $data = array();
319 - // DO NOT store this in $this->data, because it's supposed to contain existing gadgets only
320 - $this->missCache[$id] = true;
321 - } else {
322 - $data = array( 'json' => $row->gd_blob, 'timestamp' => $row->gd_timestamp );
323 - // Save to object cache
324 - $this->data[$id] = $data;
325 - }
326 - // Save to memc
327 - $wgMemc->set( $key, $data );
328 -
329 - return $data;
330 - }
331 -
332 - /**
333 - * Get the DB query to use in getLoadIDs(). Subclasses can override this to tweak the query.
 193+ * Get the DB query to use in loadAllData(). Subclasses can override this to tweak the query.
334194 * @return Array
335195 */
336 - protected function getLoadIDsQuery() {
 196+ protected function getLoadAllDataQuery() {
337197 return array(
338198 'tables' => 'gadgets',
339199 'fields' => array( 'gd_id', 'gd_blob', 'gd_timestamp' ),
Index: branches/RL2/extensions/Gadgets/backend/ForeignDBGadgetRepo.php
——@@ -77,7 +77,7 @@
7878 }
7979 }
8080
81 - protected function getLoadIDsQuery() {
 81+ protected function getLoadAllDataQuery() {
8282 $query = parent::getLoadIDsQuery();
8383 $query['conds']['gd_shared'] = 1;
8484 return $query;
Index: branches/RL2/extensions/Gadgets/backend/GadgetRepo.php
——@@ -34,14 +34,6 @@
3535 abstract public function getGadget( $id );
3636
3737 /**
38 - * Clear any in-object caches this repository may have. In particular,
39 - * the return values of getGadgetIds() and getGadget() may be cached.
40 - * Callers may wish to clear this cache and reobtain a Gadget object
41 - * when they get a conflict error.
42 - */
43 - abstract public function clearInObjectCache();
44 -
45 - /**
4638 * Check whether this repository allows write actions. If this method returns false,
4739 * methods that modify the state of the repository or the gadgets in it (i.e. addGadget(),
4840 * modifyGadget() and deleteGadget()) will always fail.

Follow-up revisions

RevisionCommit summaryAuthorDate
r101479[RL2] Redesign memc key naming in CachedGadgetRepo, as promised in r101469. I...catrope14:55, 1 November 2011
r101484[RL2] Fix stupid typos in r101469. Also have maybeLoadIDs() cache the individ...catrope15:39, 1 November 2011

Status & tagging log