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

Improved handling of URLs to be purged for better performance. #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improved handling of URLs to be purged for better performance. #65

wants to merge 2 commits into from

Conversation

gnotaras
Copy link
Contributor

@gnotaras gnotaras commented Dec 9, 2013

This patch does not change anything else than the handling of the URLs to be purged.

Instead of placing a PURGE/BAN for each URL as it is generated, all URLs are collected in an array. After removing duplicate URLs from the array, a PURGE/BAN is then placed for each URL. Using this method, we avoid sending duplicate PURGE/BAN requests to the varnish servers and improve performance.

Also, I had to fix the indentation of the WPVarnishPurgePool() function to make it easier for me to set up the loop correctly avoiding unnecessary duplicate collection of the varnish server information or other URL calculations.

Finally, I added changelog entries under the dev release. Please change this accordingly.

Thanks in advance.

Instead of placing a PURGE/BAN for each URL as it is generated, all
URLs are collected in an array. After removing duplicate URLs from
the array, a PURGE/BAN is then placed for each URL. Using this
method, we avoid sending duplicate PURGE/BAN requests to the
varnish servers and improve performance.

This idea was taken from a derivative work (apparently) of
wp-varnish: https://github.com/admingeekz/varnish-wordpress

The code could be further enhanced to send PURGE/BAN requests
using HTTP pipelining to further improve performance.
@gnotaras
Copy link
Contributor Author

Hello Pål-Kristian,

I've performed several tests since yesterday and have come to the conclusion that there will always be cases in which (for example) both the publish_post and transition_post_status actions will be triggered resulting in sending duplicate BAN requests to the varnish proxies.

The only way to avoid this is by collecting all URLs to be purged in an array and remove the duplicate ones.

There is also another problem with the current implementation. It processes the varnish backend settings and other options every single time a URL is purged.

This patch resolves all these issues and also makes it easier to further enhance performance by implementing HTTP pipelining in the future.

BTW, thanks for accepting changes to this project.

@gnotaras
Copy link
Contributor Author

Hello Pål-Kristian,

I'd definitely appreciate some better communication.

Sending you patches that can be merged cleanly adds much overhead. I am currently working on the admin bar menu because it is incomplete and buggy as well. I really need to know what's going to happen to this, so I know on what base changeset I should work.

Your thoughts on this are very welcome and much appreciated.

@pkhamre
Copy link
Owner

pkhamre commented Dec 11, 2013

Hi George,

The last year I have not been contributing any to this project, and I've been thinking to create a GitHub organization that can maintain this project. Then all developers who contribute can join this project and we can have a better discussion forum there. What do you think about that?

@pothi
Copy link
Contributor

pothi commented Dec 11, 2013

+1 for a better discussion forum. While, this plugin is the best plugin for Varnish purge, it still lacks direction and best practices that are incorporated in certain WordPress plugins.

@gnotaras
Copy link
Contributor Author

Hi Pål-Kristian,

+1. Sounds like a good idea to me as well. Personally, I have very little spare time because of other obligations. But, I think a github organization could help to further improve this plugin in a more collaborative way. The plugin currently is good, but there is definitely room for improvement like @pothi said.

I started working on an experimental fork of this plugin, but I would be glad to contribute any changes to this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants