We noticed some performance issues with the module: attached patch will add index on ip2table.ip_range_first and ip2table.ip_range_last to speed up performance.

Comments

tcmug’s picture

Title: Add index on ip2country ip ranges to speed up performance » File was dropped...
StatusFileSize
new801 bytes

Patch here:

Status: Needs review » Needs work

The last submitted patch, ip2country-add-index.patch, failed testing.

tcmug’s picture

StatusFileSize
new621 bytes

Apparently I screwed up the patch, heres another go:

tcmug’s picture

Status: Needs work » Needs review
StatusFileSize
new621 bytes

And again, this time marking for review.

greggles’s picture

Title: File was dropped... » Add index for improved performance

This seems reasonable to me. @tcmug - do you have an explain plan from before and after?

greggles’s picture

Here's an explain plan for the select query before/after:

MariaDB [cap]> explain SELECT country FROM ip2country WHERE (1262949420 >= ip_range_first AND 1262949420 <= ip_range_last) LIMIT 1;
+------+-------------+------------+------+---------------+------+---------+------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+------+-------------+------------+------+---------------+------+---------+------+------+-------------+
| 1 | SIMPLE | ip2country | ALL | NULL | NULL | NULL | NULL | 1 | Using where |
+------+-------------+------------+------+---------------+------+---------+------+------+-------------+
1 row in set (0.00 sec)

MariaDB [cap]> explain SELECT country FROM ip2country WHERE (1262949420 >= ip_range_first AND 1262949420 <= ip_range_last) LIMIT 1;
+------+-------------+------------+-------+---------------+------+---------+------+------+-----------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows |
+------+-------------+------------+-------+---------------+------+---------+------+------+-----------------------+
| 1 | SIMPLE | ip2country | range | ip | ip | 8 | NULL | 1 | Using index condition |
+------+-------------+------------+-------+---------------+------+---------+------+------+-----------------------+

The number of rows returned stays the same, but the method of finding them changes.

greggles’s picture

I did a bit of lightweight testing and didn't see any benefit from this change. An index will make the insert process slower at the db level, but in my experience that process is actually bottlenecked on php and it only runs infrequently.

So....without some benchmarking that shows this as a definite improvement I don't see a reason to make the change.

tr’s picture

Status: Needs review » Closed (works as designed)

I see a similar explain for MySQL.

I don't think the IP lookup causes any significant performance bottleneck. My tests show it to take less than a millisecond. That's added only to the user login time, not to every page load, so I don't think this poses any real problem.

My concern with performance is with the time it takes to update the database. Recent changes to this module have made it so the update can occur in the background without impacting the functionality of the module at all, so the update time is now less of a concern. (It used to be that the module wouldn't function properly while the database was being updated, because the updating process first dropped the existing data and THEN added the new data, so if it took a long time to update the data, the module would be unusable for a long time. That's no longer the case.)

The lookup time is a one-time-only penalty, and I don't think that it's worth changing the code unless you can demonstrate significant performance improvements for the change. That is not the case here.

greggles’s picture

That's added only to the user login time, not to every page load, so I don't think this poses any real problem.

That depends on your use case, actually. The way I use the module is that every visit to the site checks this lookup table.

Overall, though, I agree with your assessment.