Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance for grids with permission checks #3640

Closed
wants to merge 10 commits into from
Closed

Improve performance for grids with permission checks #3640

wants to merge 10 commits into from

Conversation

sreichel
Copy link
Contributor

Description (*)

Some grids (eg orders) check whether user is allowed to click/view ... but its valididated for each row.

With this PR its only checked once.

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Oauth Relates to Mage_Oauth Component: Api2 Relates to Mage_Api2 labels Nov 10, 2023
@sreichel sreichel changed the title Improve performance for grids with permission checks draft: Improve performance for grids with permission checks Nov 10, 2023
@sreichel sreichel changed the title draft: Improve performance for grids with permission checks Improve performance for grids with permission checks Nov 10, 2023
@kiatng
Copy link
Contributor

kiatng commented Nov 11, 2023

How about implementing it at the session level:

public function isAllowed($resource, $privilege = null)
{
$user = $this->getUser();
$acl = $this->getAcl();
if ($user && $acl) {
if (!preg_match('/^admin/', $resource)) {
$resource = 'admin/' . $resource;
}
try {
return $acl->isAllowed($user->getAclRole(), $resource, $privilege);
} catch (Exception $e) {
try {
if (!$acl->has($resource)) {
return $acl->isAllowed($user->getAclRole(), null, $privilege);
}
} catch (Exception $e) {
}
}
}
return false;
}
/**
* Check if user is logged in
*

The improvement would not be limited to grid.

In this PR, line 229 if ($user && $acl) { is bypassed, but I suspect this check is unnecessary. If $user is empty, it won't get pass Mage_Adminhtml_Controller_Action::predispatch().

@sreichel
Copy link
Contributor Author

sreichel commented Feb 2, 2024

@kiatng i did some tests with xdebug and cant see any acl resource is checked multiple times. Need some advice.

@sreichel
Copy link
Contributor Author

sreichel commented Feb 3, 2024

Sample Data: Sales Order Grid with 46 orders

Before PR:

Function NameCallsCalls%Incl. Wall Time
(microsec)
IWall%Incl. CPU
(microsecs)
ICpu%Incl.
MemUse
(bytes)
IMemUse%Incl.
PeakMemUse
(bytes)
IPeakMemUse%
Current Function
Mage_Adminhtml_Block_Sales_Order_Grid::getRowUrl46 17.9% 135,638 11.7% 106,614 9.4% 10,536 0.2% 5,880 0.1%
Exclusive Metrics for Current Function807 0.6% 0 0.0% -4,296 -40.8% 64 1.1%
Parent function
Mage_Core_Block_Template::fetchView@246 100.0% 135,638 100.0% 106,614 100.0% 10,536 100.0% 5,880 100.0%
Child functions
Mage_Core_Block_Abstract::getUrl46 25.0% 87,147 64.2% 72,718 68.2% 7,944 75.4% 4,328 73.6%
Mage_Admin_Model_Session::isAllowed46 25.0% 46,690 34.4% 33,896 31.8% 3,160 30.0% 1,488 25.3%
Mage_Core_Model_Abstract::getId46 25.0% 872 0.6% 0 0.0% 584 5.5% 0 0.0%
Mage::getSingleton46 25.0% 122 0.1% 0 0.0% 3,144 29.8% 0 0.0%

After PR:

Function NameCallsCalls%Incl. Wall Time
(microsec)
IWall%Incl. CPU
(microsecs)
ICpu%Incl.
MemUse
(bytes)
IMemUse%Incl.
PeakMemUse
(bytes)
IPeakMemUse%
Current Function
Mage_Adminhtml_Block_Sales_Order_Grid::getRowUrl46 21.8% 94,850 8.6% 106,904 9.8% 12,240 0.3% 7,584 0.2%
Exclusive Metrics for Current Function649 0.7% 0 0.0% 792 6.5% 368 4.9%
Parent function
Mage_Core_Block_Template::fetchView@246 100.0% 94,850 100.0% 106,904 100.0% 12,240 100.0% 7,584 100.0%
Child functions
Mage_Core_Block_Abstract::getUrl46 33.3% 91,712 96.7% 102,916 96.3% 7,944 64.9% 7,104 93.7%
Mage_Adminhtml_Block_Widget_Grid::canView46 33.3% 1,473 1.6% 3,988 3.7% 2,920 23.9% 80 1.1%
Mage_Core_Model_Abstract::getId46 33.3% 1,016 1.1% 0 0.0% 584 4.8% 32 0.4%

sreichel and others added 3 commits February 3, 2024 18:31
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@sreichel
Copy link
Contributor Author

sreichel commented Feb 3, 2024

Okay ... working from smartphone was not ideal ... 😂 Fix it later.

@sreichel sreichel closed this by deleting the head repository Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Api2 Relates to Mage_Api2 Component: Oauth Relates to Mage_Oauth Don't forget this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants