search

strangerstudios / paid-memberships-pro

WordPress membership plugin to restrict access to content and charge recurring subscriptions using Stripe, PayPal, and more. Fully open source. 100% GPL.
https://www.paidmembershipspro.com
Other
460 stars 357 forks source link

Main Plugin File #46

Closed topdown closed 10 years ago

topdown commented 11 years ago

There are some issue with the way things are loaded and handled in the main plugin file. The issues create a performance hit that should not even be there. All of the includes from https://github.com/strangerstudios/paid-memberships-pro/blob/dev/paid-memberships-pro.php#L30 To https://github.com/strangerstudios/paid-memberships-pro/blob/dev/paid-memberships-pro.php#L58

Should be separated for when it is actually needed. There is no reason to include files on the front of the site that are only used in the admin panel. A simple is_admin() fixes this quickly.

Same with the update check https://github.com/strangerstudios/paid-memberships-pro/blob/dev/paid-memberships-pro.php#L64 Why is this running all the time, and every single page load? Queries that don't need to be there.

Options (db) were mentioned in another ticket. So I will leave that there.

strangerstudios commented 11 years ago

Topdown, first thanks for posting this issue.

I'm not sure there is an easy fix here.

Actually, after a quick check through, every one of those included files could potentially be needed on the frontend of the website. Some of them are typical admin things that might be run on the frontend if people are using certain plugins (e.g. saving profile fields). Some are bundled oddly so that the include file has admin and frontend stuff inside of them (e.g. the reports contain code for showing the reports in the admin, but also code for tracking activity on the frontend, etc).

I could try to architect things to better separate the admin code from the frontend, but I'm not sure it would be worth the performance boost. If you get me some numbers I would consider it.

The one exception here is the DB upgrade check. However, that code quickly checks an option in the WP DB to see if the DB is on the latest version. If so, nothing happens. There was a bug with this in one of the intermediate releases a couple weeks back that was running SQL queries on every page load, but that has been fixed in the latest versions.

I'll leave this open in case you want to discuss more specific pieces of code that could be wrapped in is_admin/etc to improve performance.

topdown commented 11 years ago

For performance work, the nature of it is a lot of work and testing for little results at a time.

Fact is, I advertise performance, its the nature of my work. So I very carefully look at performance and code before I recommend or use a plugin on a client site. Every little bit counts with performance, with the hope that the end results are a big difference.

With that said, the update check has to be wrapped, it is querying every page load https://github.com/strangerstudios/paid-memberships-pro/blob/dev/includes/upgradecheck.php#L13

As for numbers, I just took the obvious includes, and yes it could break things because I did not read through each one at this point. Tests, Blog with 1 protected post and listing only 5 posts. Initial, PMP disabled 11 queries, Load Time 0.226 s, 31.41MiB memory, 104 includes, 240 Hooks PMP enabled 50 queries, Load Time 0.289 s, 34.49MiB memory, 133 includes, 285 Hooks PMP enabled with is_admin() around obvious files 45 queries, Load Time 0.262 s, 33.82MiB memory, 127 includes, 280 Hooks

This is just a quick dirty test, if a person was to go through and do full separation, who knows, the results could be beyond impressive.

Membership sites can't be cached like normal sites, which I am sure you know, so every bit is a big deal here.

strangerstudios commented 11 years ago

Details are important here.

I'm not oblivious to these performance issues, so simply saying it's "a big deal" isn't going to change anything about how we're developing. We know it's important.

That said, the code base is over 3 years old and we iterate fast so there are going to be areas for improvement.

Let me know which files you thought we're "obvious" to wrap in is_admin(). Heck, do a pull request if you know how.

I understand the pmpro_upgradecheck one. I'll think about that. We probably don't have to worry about those cases where the plugin is activated but the DB tables are missing and someone hits the frontend of the site. Sweet, we can save 1 DB query there. Let me know the others.

Are you advertising a performance service or something?

strangerstudios commented 11 years ago

Hey, topdown, I think my tone is a little aggressive and defensive and I apologize for that. You're trying to help. And I'm trying to help you help me. :)

topdown commented 11 years ago

@strangerstudios said:

Are you advertising a performance service or something?

If this was a little slam, No I am not soliciting. But Yes, I do promote performance with my clients. Its one of the reasons they hire me.

Yes, I am just trying to help and have no problem forking, branching and patching. 8 years developing LAMP stack and 5 custom WordPress work, so I am not pointing fingers or anything like that. Just suggestions and thoughts.

I will see what I can do when I have some free time.