Optimize performance of checking is can access object #6944

peter-gribanov · 2021-03-15T08:56:34Z

Subject

Remove code duplication to improve performance.

Changelog

### Fixed

- Optimize performance of checking is can access object in `AbstractAdmin::configureActionButtons()`

VincentLanglet

The display is made this way:

    {% for item in admin.getActionButtons(action, (object is defined) ? object : null ) %}
        {% if item.template is defined %}
            {% include item.template %}
        {% endif %}
    {% endfor %}

So I assume it will change the order...

peter-gribanov · 2021-03-15T09:02:34Z

So I assume it will change the order...

Exactly. I forgot about it.

src/Admin/AbstractAdmin.php

VincentLanglet · 2021-03-15T19:42:47Z

src/Admin/AbstractAdmin.php

-            && null !== $object
+        $canAccessObject =
+            null !== $object
+            && \in_array($action, ['show', 'edit', 'delete', 'acl', 'history'], true)


I'm not sure this check improve performances

If $action is show or edit then $this->id($object) call thrice. As far as i can tell, calling method AbstractAdmin::id() is not free. This is a minor performance improvement, but it removes the duplication of code.

I can improve performance for internal actions by removing this line:

$canAccessObject = null !== $object - && \in_array($action, ['show', 'edit', 'delete', 'acl', 'history'], true) && null !== $this->id($object) ;

VincentLanglet

Tests are failing

peter-gribanov · 2021-03-16T10:52:13Z

tests/Admin/AdminTest.php

@@ -2172,7 +2172,7 @@ public function testGetActionButtonsListWithoutExtraChecks(): void
        $admin->method('isAclEnabled')->willReturn(true);
        $admin->method('getExtensions')->willReturn([]);

-        $admin->expects($this->exactly(9))->method('hasRoute')->willReturn(false);
+        $admin->expects($this->exactly(4))->method('hasRoute')->willReturn(false);


Another example of performance optimization.

peter-gribanov · 2021-03-16T15:50:26Z

src/Admin/AbstractAdmin.php

+        // nothing to do for non-internal actions
+        if (!\in_array($action, ['tree', 'show', 'edit', 'delete', 'acl', 'history', 'list', 'batch'], true)) {
+            return [];
+        }


All the following code for this method does not make sense for custom actions. Perhaps it would be better to immediately identify these custom actions?

Can someone explain to me why this is the case? I have custom actions from where I'd like to be able to move back to "edit" mode.

there is a configureActionButtons method for this.

Oh, you meant the early return ?

All the logic here doesn't make sens for custom action, the call to configureActionButtons still does. Can you open a pr @toooni ?

Yes, the early return. Yes, the configureActionButtons call does always make sense and I can make a PR. But why does an edit action button not make sense in a custom action? I'd think the default behaviour should be that the the edit (and other actions) should be visible like in internal actions (these could still be unset in configureActionButtons).

I recommend you to open the PR with what you think is the good behavior and we'll discuss about it.

But why does an edit action button would make sens in a custom action too ? For each actions, we have a check like

self::MASK_OF_ACTION_EDIT & $actionBit

Removing the early return won't add automatically the edit button in your custom action, and I think it's the right move. Custom action are custom, so buttons should be configured.

It has always been a challenge to add standard action buttons to custom action pages. But this must always be manually configured. Not every custom page needs to add an edit button.

Ok. I might be a bit biased because in my cases I could always need the edit action button. I've created a PR #7483 to allow configureActionButtons again.

Btw. maybe the default action buttons should be predefined somewhere so it would be easier to re-add them in custom actions.

phansys

Maybe we could use a bitmask in order to avoid the multiple calls to in_array().

peter-gribanov · 2021-03-18T09:33:47Z

Maybe we could use a bitmask in order to avoid the multiple calls to in_array().

Interesting idea. The performance will clearly be better. It may be easier to manage this.

VincentLanglet · 2021-03-19T21:16:57Z

src/Admin/AbstractAdmin.php

@@ -72,6 +72,34 @@ abstract class AbstractAdmin extends AbstractTaggedAdmin implements AdminInterfa
            Doctrine\\\Orm|Doctrine\\\Phpcr|Doctrine\\\MongoDB|Doctrine\\\CouchDB
        )\\\(.*)@x';

+    private const ACTION_TREE = 1;
+    // private const ACTION_CREATE = 2; // not used now


Why commenting this ?

I suggest we reserve a bit for the Create action. But the conditions for the Create button are not described in method AbstractAdmin::configureActionButtons() and this constant is not used.

Would you rather reserve a bit, remove unused code, or leave dead code?

If we need it, we could add it as the last bit so it's maybe not necessary to reserve a bit.
But if we reserve a bit, I prefer to create the const rather than having a comment.

Maybe @sonata-project/contributors think differently

I removed the commented code.

src/Admin/AbstractAdmin.php

phansys

As mentioned in a previous comment, I'd use MASK_ACTION_* instead of ACTION_*_IN constants, but since they are private, I think it isn't a blocker.

use bitmap in AbstractAdmin::configureActionButtons() not reserve a bit for the Create action rename mask constants

peter-gribanov · 2021-03-29T15:31:11Z

@phansys i didn't notice your comment. I change the name of the constants to MASK_OF_ACTION_* and MASK_OF_ACTIONS_USING_OBJECT.

VincentLanglet · 2021-03-29T15:41:36Z

Thanks @peter-gribanov

peter-gribanov force-pushed the canAccessObject_performance branch from 94e0fbf to d29a3bb Compare March 15, 2021 08:59

VincentLanglet reviewed Mar 15, 2021

View reviewed changes

peter-gribanov force-pushed the canAccessObject_performance branch 2 times, most recently from ac5cd50 to e35c4ab Compare March 15, 2021 09:07

peter-gribanov requested a review from VincentLanglet March 15, 2021 09:10

phansys reviewed Mar 15, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

phansys previously approved these changes Mar 15, 2021

View reviewed changes

VincentLanglet reviewed Mar 15, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

VincentLanglet reviewed Mar 15, 2021

View reviewed changes

peter-gribanov dismissed phansys’s stale review via 99f8db4 March 16, 2021 10:23

peter-gribanov force-pushed the canAccessObject_performance branch from e35c4ab to 99f8db4 Compare March 16, 2021 10:23

VincentLanglet previously approved these changes Mar 16, 2021

View reviewed changes

VincentLanglet reviewed Mar 16, 2021

View reviewed changes

peter-gribanov dismissed VincentLanglet’s stale review via 9c9d1c9 March 16, 2021 10:51

peter-gribanov force-pushed the canAccessObject_performance branch from 99f8db4 to 9c9d1c9 Compare March 16, 2021 10:51

peter-gribanov commented Mar 16, 2021

View reviewed changes

VincentLanglet previously approved these changes Mar 16, 2021

View reviewed changes

peter-gribanov dismissed VincentLanglet’s stale review via c21b11b March 16, 2021 15:44

peter-gribanov force-pushed the canAccessObject_performance branch from 9c9d1c9 to c21b11b Compare March 16, 2021 15:44

peter-gribanov closed this Mar 16, 2021

peter-gribanov reopened this Mar 16, 2021

peter-gribanov commented Mar 16, 2021

View reviewed changes

peter-gribanov force-pushed the canAccessObject_performance branch from c21b11b to 86c2f2c Compare March 16, 2021 15:53

VincentLanglet previously approved these changes Mar 17, 2021

View reviewed changes

phansys previously approved these changes Mar 17, 2021

View reviewed changes

peter-gribanov dismissed stale reviews from phansys and VincentLanglet via 4db6edf March 18, 2021 09:33

peter-gribanov force-pushed the canAccessObject_performance branch from 86c2f2c to 4db6edf Compare March 18, 2021 09:33

peter-gribanov force-pushed the canAccessObject_performance branch from 4db6edf to 9b2873a Compare March 18, 2021 09:36

VincentLanglet reviewed Mar 19, 2021

View reviewed changes

phansys reviewed Mar 20, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

VincentLanglet requested review from a team and phansys March 24, 2021 19:13

VincentLanglet previously approved these changes Mar 29, 2021

View reviewed changes

VincentLanglet requested a review from a team March 29, 2021 13:56

phansys requested changes Mar 29, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

peter-gribanov dismissed VincentLanglet’s stale review via 5406a64 March 29, 2021 14:55

peter-gribanov force-pushed the canAccessObject_performance branch from 296dd6b to 5406a64 Compare March 29, 2021 14:55

phansys reviewed Mar 29, 2021

View reviewed changes

optimize performance of checking is can access object

01dad35

use bitmap in AbstractAdmin::configureActionButtons() not reserve a bit for the Create action rename mask constants

peter-gribanov force-pushed the canAccessObject_performance branch from 5406a64 to 01dad35 Compare March 29, 2021 15:28

phansys approved these changes Mar 29, 2021

View reviewed changes

phansys requested review from VincentLanglet and a team March 29, 2021 15:35

VincentLanglet approved these changes Mar 29, 2021

View reviewed changes

VincentLanglet merged commit 3997b4f into sonata-project:3.x Mar 29, 2021

VincentLanglet added the patch label Mar 29, 2021

peter-gribanov deleted the canAccessObject_performance branch March 30, 2021 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize performance of checking is can access object #6944

Optimize performance of checking is can access object #6944

peter-gribanov commented Mar 15, 2021

VincentLanglet left a comment

peter-gribanov commented Mar 15, 2021

VincentLanglet Mar 15, 2021

peter-gribanov Mar 16, 2021

VincentLanglet left a comment

peter-gribanov Mar 16, 2021

peter-gribanov Mar 16, 2021

VincentLanglet Mar 16, 2021

toooni Sep 13, 2021

VincentLanglet Sep 13, 2021

VincentLanglet Sep 13, 2021

toooni Sep 13, 2021

VincentLanglet Sep 13, 2021

peter-gribanov Sep 13, 2021

toooni Sep 14, 2021

phansys left a comment

peter-gribanov commented Mar 18, 2021

VincentLanglet Mar 19, 2021

peter-gribanov Mar 22, 2021

VincentLanglet Mar 22, 2021

peter-gribanov Mar 23, 2021

phansys left a comment

peter-gribanov commented Mar 29, 2021

VincentLanglet commented Mar 29, 2021

Optimize performance of checking is can access object #6944

Optimize performance of checking is can access object #6944

Conversation

peter-gribanov commented Mar 15, 2021

Subject

Changelog

VincentLanglet left a comment

Choose a reason for hiding this comment

peter-gribanov commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansys left a comment

Choose a reason for hiding this comment

peter-gribanov commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansys left a comment

Choose a reason for hiding this comment

peter-gribanov commented Mar 29, 2021

VincentLanglet commented Mar 29, 2021