Introduce _rdf_mapping_load_multiple() to reduce queries

Great idea!

+++ b/core/modules/rdf/rdf.module
@@ -190,17 +190,36 @@ function _rdf_get_default_mapping($type) {
-  if (!$mapping) {
-    return array();
+  if (!$mappings) {
+    return FALSE;
   }

why returning FALSE instead of an empty array?

Hm. Is there a reason there's a leading underscore on these functions? Elsewhere we do $thingy_load() and $thingy_load_multiple(), not _$thingy_load() and _$thingy_load_multuple(). Maybe fodder for a separate issue, but it'd be nice if these functions were consistent with elsewhere in core. We could introduce them as wrapping functions in D7 and just rename them in D8.

Tagging.

@webchick _rdf_mapping_load() is a helper function for rdf_entity_info_alter(), which hits the database. rdf_mapping_load() does exist, and retrieves its data from the entity info cache. I guess your point is that _rdf_mapping_load() is misleading wrt to the $thingy_load() convention?

@loganfsmyth we could have put the mapping loading from the db in rdf_entity_info_alter() but left it outside as a function in case another module needs to access the database mapping (bypassing the cache). But I guess a better name for _rdf_mapping_load() is in order (not for this issue though).

it's just that you get the RDF mappings for free along with the other properties of each entity type and bundle. if you need to call them separately, it's extra code that you have to repeat all the time, and you might forget to do it. This solution was suggested at the time by catch and fago. This approach is fairly aligned with the entity API btw.

Yikes; just noticed that my site is barfing after cache clears and module enabling/disabling, partly due to _rdf_mapping_load() running more than 500 times, using up a ton of memory while it's at it. Would love to see this go in. (This plus #1040790: _field_info_collate_fields() memory usage would go a long way towards helping sites with more than a few hundred fields + instances + bundles).

@geerlingguy could you try the patch in #4 on D7 and see whether it solves your issue?

Clear all caches — before patch:

265,225 calls to _rdf_mapping_load(), total of about 280ms of total query time, page execution in ~13600ms, ~190MB peak mem usage.

Clear all caches — after patch:

42 calls to _rdf_mapping_load_multiple(), total of about 7ms of total query time, page execution in ~13300ms, ~189MB peak mem usage.

I ran each clear all caches three times, and cleared APC's caches (restarted apache) in between each one as well, to make things fair. The only nit I have is with:

@@ -187,20 +187,36 @@ function _rdf_get_default_mapping($type) {
  *   The bundle the mapping refers to.
  *
  * @return
- *   An RDF mapping structure or an empty array if no record was found.
+ *   An RDF mapping structure or FALSE.

Why switch to a mixed return variable instead of an empty array if nothing found? Marking RTBC, though, since there's precedent elsewhere for mixed return values, and that definitely shouldn't block this patch (unless we have a policy to return only one type of value?).

(Attached is D7 patch I used to test on staging site benchmarked above—only difference is the file structure).

+++ b/core/modules/rdf/rdf.module
@@ -187,20 +187,36 @@ function _rdf_get_default_mapping($type) {
+ * Helper function to retreive a set of RDF mappings from the database.

s/retreive/retrieve

+++ b/core/modules/rdf/rdf.module
@@ -187,20 +187,36 @@ function _rdf_get_default_mapping($type) {
+ *   An RDF mapping structure or FALSE.

..., or FALSE if the mapping does not exist.

Fixed minor typos.

Back to RTBC.

Looks great. Committed/pushed to 8.x, moving to 7.x for backport.

Attached patch should work for D7. I found one typo in the patch that was committed to D8:

@@ -187,20 +187,36 @@ function _rdf_get_default_mapping($type) {
  *   The bundle the mapping refers to.
  *
  * @return
- *   An RDF mapping structure or an empty array if no record was found.
+ *   An RDF mapping structure or, FALSE if the mapping does not exist.
  */

Comma should go after word 'structure' instead of after 'or'. I fixed that typo in the attached D7 patch.

@geerlingguy could you roll a patch for D8 as well for the minor typo please?

Patch for D8 attached. (Should be ignored by testbot.)

#22: rdf-mapping-load-multiple-1338966-22.patch queued for re-testing.

Drupal 8 now uses CMI to load the mappings since #1869600: Refactor RDF mappings to be inline with the new Entity Field API went in, and the comment referenced in patch #24 no longer exists in D8, so this issue only focuses on D7 at this point.

#22 looks good to me as long as it passes the tests.

#22 applies and passes tests (locally) still.

Testbot has become flaky these days. patch to be committed in #22

#22 still green.

I think this looks good but not sure about this part:

  * @return
- *   An RDF mapping structure or an empty array if no record was found.
+ *   An RDF mapping structure, or FALSE if the mapping does not exist.
  */
 function _rdf_mapping_load($type, $bundle) {

An earlier comment in this issue said that part would not be backported to D7...

Given that it's a private function, I don't believe it's a huge deal to change it, but would be better not to if there's no good reason. And I actually don't think the change makes much sense? If the function returns an array normally, then it's better for it to always return an array to be consistent, right?

(node_load() and company might do it differently and return FALSE, but that's not the best practice and also, they return objects normally as opposed to arrays, which is a bit different...)

@David_Rothstein: right, a couple of people actually mentioned that in passing about the D7 patch, this mixed return type should not have been backported... let's limit the API changes for D7. All what needs to be done here is revert the docs change for _rdf_get_default_mapping(), and instead of FALSE, just return array():

+  return $mappings ? reset($mappings) : FALSE;

Done.

Thanks Jamie :) patch looks good, and verified interdiff locally, it's good to go. back to RTBC.

Committed to 7.x - thanks!