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

Refactor bindRemoveButtons for improved performance #1144

Merged
merged 2 commits into from May 15, 2015
Merged

Refactor bindRemoveButtons for improved performance #1144

merged 2 commits into from May 15, 2015

Conversation

@mpchadwick
Copy link
Contributor

@mpchadwick mpchadwick commented Mar 29, 2015

There are known issues in Magento managing options with thousands of attributes.

It has been blogged about here, where the author proposed the following solution.

In my my profiling I found that bindRemoveButtons is the culprit in terms of the inefficiency. Each time an option is appended to the DOM bindRemoveButtons checks every single delete button on the page. You can imagine how that could add up when working with attributes with thousands of options.

This PR refactors the options template implements bindRemoveButton (rather than bindRemoveButtons) so that when an option is appended to the DOM only the delete button for that option gets bound.

@kandy
Copy link
Contributor

@kandy kandy commented Mar 29, 2015

Maybe better to use event delegation?

@mpchadwick
Copy link
Contributor Author

@mpchadwick mpchadwick commented Mar 29, 2015

Updated.

Based on my testing going from bindRemoveButtons() to bindRemoveButton() had a larger impact on performance, but this helped as well. That being said managing attributes with thousands of attributes has slowed down in Magento 2 when compared to Magento 1.

I have used the SQL scripts below to set up a dropdown attribute with 3,500 options in Magento 2, and Magento 1.13.1.0. Both installs are running on the same VM.

########################################
# Set Up A Massive Dropdown Attribute
# Magento 1.13.1.0
########################################

# User defined vars
SET @num_options = 3500;
SET @attribute_code = 'massive_attribute';
SET @attribute_label = 'Massive Attribute';
SET @store_id = 0;

# Create new attribute
INSERT INTO `eav_attribute` (`entity_type_id`, `attribute_code`, `backend_model`, `backend_type`, 
    `frontend_input`, `frontend_label`, `frontend_class`, `source_model`, `is_required`, `is_user_defined`, `is_unique`) 
VALUES ('4', @attribute_code, NULL, 'int', 'select', 
    @attribute_label, NULL, 'eav/entity_attribute_source_table', '0', '1', '0');

SELECT @attribute_id :=`attribute_id` FROM `eav_attribute` WHERE `attribute_code`=@attribute_code;

INSERT INTO `catalog_eav_attribute` (`attribute_id`, `is_global`, `is_searchable`, `is_filterable`, `is_comparable`, 
    `is_visible_on_front`, `is_html_allowed_on_front`, `is_filterable_in_search`, `used_in_product_listing`, 
    `used_for_sort_by`, `is_configurable`, `apply_to`, `is_visible_in_advanced_search`, `is_used_for_promo_rules`, `layered_navigation_canonical`) 
VALUES (@attribute_id, '1', '0', '0', '0', '0', '1', '0', '0', '0', '0', NULL, '0', '0', '0');

# Add options
DROP PROCEDURE IF EXISTS mpchadwick_add_options;
DELIMITER //
CREATE PROCEDURE mpchadwick_add_options(IN num_options INT, IN attribute_id INT, IN store_id INT)
BEGIN
    DECLARE i INT;
    SET i=0;
    WHILE i < num_options DO
        INSERT INTO `eav_attribute_option` (`attribute_id`, `sort_order`) VALUES (attribute_id, i);
        SELECT @option_id :=`option_id` FROM `eav_attribute_option` ORDER BY `option_id` DESC LIMIT 1;
        SET @value = CONCAT("value ", i);
        INSERT INTO `eav_attribute_option_value` (`option_id`, `store_id`, `value`) VALUES (@option_id, store_id, @value);
        SET i = i + 1;
    END WHILE;
END //
DELIMITER ;

CALL mpchadwick_add_options(@num_options, @attribute_id, @store_id);
########################################
# Set Up A Massive Dropdown Attribute
# Magento 2
########################################
# User defined vars
SET @num_options = 3500;
SET @attribute_code = 'massive_attribute';
SET @attribute_label = 'Massive Attribute';
SET @store_id = 0;

INSERT INTO `eav_attribute` (`entity_type_id`, `attribute_code`, `backend_model`, `backend_type`, 
    `frontend_input`, `frontend_label`, `source_model`, `is_required`, `is_user_defined`, `is_unique`) 
VALUES ('4', @attribute_code, NULL, 'int', 'select', 
    @attribute_label, 'Magento\\Eav\\Model\\Entity\\Attribute\\Source\\Table', '0', '1', '0');

SELECT @attribute_id :=`attribute_id` FROM `eav_attribute` WHERE `attribute_code`=@attribute_code;

INSERT INTO `catalog_eav_attribute` (`attribute_id`, `is_global`, `is_searchable`, `is_filterable`, `is_comparable`, 
    `is_visible_on_front`, `is_html_allowed_on_front`, `is_filterable_in_search`, `used_in_product_listing`, 
    `used_for_sort_by`, `apply_to`, `is_visible_in_advanced_search`, `is_used_for_promo_rules`) 
VALUES (@attribute_id, '0', '0', '0', '0', '0', '1', '0', '0', '0', NULL, '0', '0');

DROP PROCEDURE IF EXISTS mpchadwick_add_magento2_attribute_options;
DELIMITER //
CREATE PROCEDURE mpchadwick_add_magento2_attribute_options(IN num_options INT, IN attribute_id INT, IN store_id INT)
BEGIN
    DECLARE i INT;
    SET i = 0;
    WHILE i < num_options DO
        INSERT INTO `eav_attribute_option` (`attribute_id`, `sort_order`) VALUES (attribute_id, i);
        SELECT @option_id :=`option_id` FROM `eav_attribute_option` ORDER BY `option_id` DESC LIMIT 1;
        SET @value = CONCAT("value ", i);
        INSERT INTO `eav_attribute_option_value` (`option_id`, `store_id`, `value`) VALUES (@option_id, store_id, @value);
        SET i = i + 1;
    END WHILE;
END //
DELIMITER ;

CALL mpchadwick_add_magento2_attribute_options(@num_options, @attribute_id, @store_id);

Testing in Chome 41.

Magento 1.13.1 using a single call to bindRemoveButtons() function after all options have been added (solution implemented in my blog post I linked to above) 3.14 seconds spent on scripting.
magento-1 13 1 0__v2

Magento 2 using commit currently on the head of this PR, 8.94 seconds spent on scripting.
v3

I would say this PR is definitely improvement. One change that could be considered which would be of larger scope would be to add pagination.

@kokoc
Copy link
Member

@kokoc kokoc commented Mar 31, 2015

@mpchadwick Thank you for the contribution! We will get back to you once we complete the analysis.

@kokoc
Copy link
Member

@kokoc kokoc commented Apr 15, 2015

The code looks great!

CR: passed
Builds: green
Resolution: ready to merge
Internal reference: MAGETWO-36267

@magento-team magento-team merged commit 8d259f3 into magento:develop May 15, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kokoc
Copy link
Member

@kokoc kokoc commented May 27, 2015

@mpchadwick your pull request is merged in 0.74.0-beta9. Thank you very much for your contribution and continued support to Magento 2!

magento-team pushed a commit that referenced this pull request Jun 1, 2017
[okapis] PR 1144
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants