Download & Extend
Issues

Optimize the performance of PluginBase::setOptionDefaults

Posted by dawehner on November 24, 2012 at 11:31am
Project:Drupal core
Version:8.x-dev
Component:views.module
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:Performance, VDC

Issue Summary

Problem/Motivation

While performance testing on #1811816: Benchmark node_page_default vs. node view one issue was that PluginBase::setOptionDefaults was rather slow.

Proposed resolution

  • Remove the unneeded translation (discuss whether it makes sense to translate default values once we have metadata config translation in)
  • Simplify the code by requiring a default option (which is actually the case since d6.3)
Login or register to post comments

Comments

#1

Posted by dawehner on November 24, 2012 at 11:32am
Status:active» needs review
AttachmentSizeStatusTest resultOperations
drupal-1849280-1.patch7.36 KBIdleFAILED: [[SimpleTest]]: [MySQL] 48,774 pass(es), 0 fail(s), and 9 exception(s).View details

#2

Posted by System Message on November 24, 2012 at 12:31pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-1.patch, failed testing.

#3

Posted by dawehner on November 24, 2012 at 2:00pm
Status:needs work» needs review

Yeah this shows some bugs in defineOptions implementations.

AttachmentSizeStatusTest resultOperations
interdiff.txt679 bytesIgnored: Check issue status.NoneNone
drupal-1849280-3.patch8.02 KBIdleFAILED: [[SimpleTest]]: [MySQL] 48,756 pass(es), 2 fail(s), and 0 exception(s).View details

#4

Posted by System Message on November 24, 2012 at 3:02pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-3.patch, failed testing.

#5

Posted by dawehner on November 24, 2012 at 11:39pm
Status:needs work» needs review

This time with a proper default value for the boolean handler.

AttachmentSizeStatusTest resultOperations
drupal-1849280-5.patch8.02 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1849280-5.patch. Unable to apply patch. See the log in the details link for more information.View details
interdiff.txt683 bytesIgnored: Check issue status.NoneNone

#6

Posted by Berdir on December 15, 2012 at 3:16pm

#5: drupal-1849280-5.patch queued for re-testing.

#7

Posted by System Message on December 15, 2012 at 4:11pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-5.patch, failed testing.

#8

Posted by dawehner on December 15, 2012 at 5:30pm
Status:needs work» needs review

#5: drupal-1849280-5.patch queued for re-testing.

#9

Posted by System Message on December 15, 2012 at 6:14pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-5.patch, failed testing.

#10

Posted by dawehner on December 15, 2012 at 6:59pm
Status:needs work» needs review

#5: drupal-1849280-5.patch queued for re-testing.

#11

Posted by System Message on December 15, 2012 at 7:01pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-5.patch, failed testing.

#12

Posted by dawehner on December 15, 2012 at 7:11pm
Status:needs work» needs review

#5: drupal-1849280-5.patch queued for re-testing.

#13

Posted by dawehner on January 2, 2013 at 3:21pm

#5: drupal-1849280-5.patch queued for re-testing.

#14

Posted by System Message on January 2, 2013 at 6:17pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-5.patch, failed testing.

#15

Posted by dawehner on January 2, 2013 at 10:36pm
Status:needs work» needs review

This contained some other patch internally.

AttachmentSizeStatusTest resultOperations
drupal-1849280-15.patch2.41 KBIdleFAILED: [[SimpleTest]]: [MySQL] 50,824 pass(es), 0 fail(s), and 4 exception(s).View details

#16

Posted by System Message on January 3, 2013 at 5:12am
Status:needs review» needs work

The last submitted patch, drupal-1849280-15.patch, failed testing.

#17

Posted by dawehner on January 3, 2013 at 8:38am
Status:needs work» needs review

#15: drupal-1849280-15.patch queued for re-testing.

#18

Posted by dawehner on January 14, 2013 at 5:28pm

#15: drupal-1849280-15.patch queued for re-testing.

#19

Posted by System Message on January 14, 2013 at 6:50pm
Status:needs review» needs work

The last submitted patch, drupal-1849280-15.patch, failed testing.

#20

Posted by dawehner on January 14, 2013 at 9:01pm
Status:needs work» needs review

Let's fix it.

AttachmentSizeStatusTest resultOperations
drupal-1849280-20.patch3.36 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1849280-20.patch. Unable to apply patch. See the log in the details link for more information.View details
interdiff.txt971 bytesIgnored: Check issue status.NoneNone

#21

Posted by dawehner on February 9, 2013 at 12:48am

#20: drupal-1849280-20.patch queued for re-testing.

#22

Posted by System Message on February 9, 2013 at 12:59am
Status:needs review» needs work

The last submitted patch, drupal-1849280-20.patch, failed testing.

#23

Posted by dawehner on February 9, 2013 at 1:30am
Status:needs work» needs review

Just another rerole

AttachmentSizeStatusTest resultOperations
drupal-1849280-23.patch3.44 KBIdleFAILED: [[SimpleTest]]: [MySQL] 49,131 pass(es), 505 fail(s), and 254 exception(s).View details

#24

Posted by System Message on February 9, 2013 at 2:02am
Status:needs review» needs work

The last submitted patch, drupal-1849280-23.patch, failed testing.

#25

Posted by tim.plunkett on February 9, 2013 at 4:21am
Status:needs work» needs review

#23: drupal-1849280-23.patch queued for re-testing.

#26

Posted by tim.plunkett on February 9, 2013 at 4:51am
Status:needs review» needs work

The changes look great! Two small nitpicks

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -99,17 +99,27 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+   * Fill up the options of the plugin with defaults.

Fills

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -99,17 +99,27 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+   *   An array which stores the actual option values of the plugin.
+   *
+   * @param array $options

No blank line needed

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -99,17 +99,27 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+   *     - default: The default value of one option
+   *     - contains: (optional) An array which describes the available options
+   *       under the key.

These are indented 2 spaces too much, and the (optional) goes before the key name I think

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/Boolean.phpundefined
@@ -38,7 +38,7 @@ protected function defineOptions() {
-    $options['not'] = array('definition bool' => 'reverse');
+    $options['not'] = array('default' => FALSE, 'bool' => TRUE);

Hahaha, what was that? :)

#27

Posted by dawehner on February 9, 2013 at 7:49am
Status:needs work» needs review

#23: drupal-1849280-23.patch queued for re-testing.

#28

Posted by dawehner on February 9, 2013 at 8:54am

Yeah indeed this patch showed some odd bugs in the code :)
Thanks for the review!

AttachmentSizeStatusTest resultOperations
drupal-1849280-28.patch3.43 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,832 pass(es).View details

#29

Posted by tim.plunkett on February 20, 2013 at 8:51pm
Status:needs review» reviewed & tested by the community
Issue tags:+needs backport to D7

The removed $level param hasn't been used since Views 6.x-2.x, so in theory this clean-up could get backported.

Nice work @dawehner!

#30

Posted by dawehner on February 20, 2013 at 10:39pm
Issue tags:-needs backport to D7

We probably could only fix the $level parameter, but not the rest, as people might have broken option_defintions(),
so doesn't seems worth to do.

#31

Posted by damiankloip on February 21, 2013 at 12:04am
Issue tags:+needs backport to D7

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -99,17 +99,26 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
       if (isset($definition['contains']) && is_array($definition['contains'])) {

If we are trimming the fat out of this method, should/could we also assume that if 'contains' is set, it will be an array.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined
@@ -99,17 +99,26 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+   *   - default: The default value of one option
+   *   - (optional) contains: An array which describes the available options
+   *       under the key.

Small thing: Should we add the translatable key to the docs too?

I have also just opened #1922966: remove 'bool' key from option definitions as a related follow up.

#32

Posted by damiankloip on February 21, 2013 at 12:05am
Issue tags:-needs backport to D7

Oops, must have x-posted

#33

Posted by dawehner on February 21, 2013 at 11:42pm

Good points.

AttachmentSizeStatusTest resultOperations
interdiff.txt1.21 KBIgnored: Check issue status.NoneNone
drupal-1849280-33.patch3.72 KBIdlePASSED: [[SimpleTest]]: [MySQL] 52,247 pass(es).View details

#34

Posted by webchick on February 28, 2013 at 1:42am
Status:reviewed & tested by the community» fixed

Yay performance!

Committed and pushed to 8.x. Thanks!

#35

Posted by System Message on March 14, 2013 at 1:50am
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nobody click here