search

varnishcache / varnish-devicedetect

VCL based device detection for Varnish Cache.
Other
299 stars 88 forks source link

Process overrides before processing all regular expressions #3

Closed sergesyrota closed 11 years ago

sergesyrota commented 11 years ago

This way we don't parse/execute any regular expressions if override is set.

lkarsten commented 11 years ago

Hi, and thanks for the contribution.

The patch is reformatting all tabs and spaces in the entire file, and as such I'm not going to merge it in its current form.

The more general question raised here is if it makes sense to don't do the matching if the override is set. Usage of the override in our original case was just for testing/development. No ordinary users would have it set. In that case having it last (and making the file as readable as possible) made sense. Optimize for the 80%, not the 20%.

Do you have a setup where ordinary users have the cookie set?

sergesyrota commented 11 years ago

Oops, did not realize that. I can reformat it back...

We adapted your VCL in our configuration actually, and we use the cookie to force certain mode on the web site. If we detect mobile browser, and user clicks "full site" - we set force cookie to persist desktop version (and vice versa). Still, it's not going to be set for majority of cases, but for those that it's set - there'd be savings on all those regular expressions that don't need to be processed. Although I haven't timed how long it takes to process them, I think it would affect our current average processing time of less than 1ms.

On the other hand, someone might want to have both forced device, and auto-detected, to see how many are forced, making this change useless too.

Feel free to reject it, I just thought I'd offer an idea that might be useful ;) No hard feeling for that :)

Best Regards, Sergey Syrota

On Fri, Jan 11, 2013 at 4:10 AM, Lasse Karstensen notifications@github.comwrote:

Hi, and thanks for the contribution.

The patch is reformatting all tabs and spaces in the entire file, and as such I'm not going to merge it in its current form.

The more general question raised here is if it makes sense to don't do the matching if the override is set. Usage of the override in our original case was just for testing/development. No ordinary users would have it set. In that case having it last (and making the file as readable as possible) made sense. Optimize for the 80%, not the 20%.

Do you have a setup where ordinary users have the cookie set?

— Reply to this email directly or view it on GitHubhttps://github.com/varnish/varnish-devicedetect/pull/3#issuecomment-12138331.

lkarsten commented 11 years ago

Hi Sergey.

I agree with your reasoning. Please re-send the patch without the indentation problems.

Since you have this in production, you perhaps also have some fixes related to user-agents? I'd be very grateful for any such updates.

Thanks.

With regards, Lasse Karstensen Varnish Software AS

sergesyrota commented 11 years ago

Ok, will do tonight.

For user agents, we actually replaced your regexps with a different set to match what we have on the backend (for if Varnish is down, or in maintenance). And we just implemented it yesterday, so Haven't had a chance to see any anomalies. But if I come across anything, I'll let you know.

Best regards, Serge Syrota

On Jan 11, 2013, at 9:38 AM, Lasse Karstensen notifications@github.com wrote:

Hi Sergey.

I agree with your reasoning. Please re-send the patch without the indentation problems.

Since you have this in production, you perhaps also have some fixes related to user-agents? I'd be very grateful for any such updates.

Thanks.

With regards, Lasse Karstensen Varnish Software AS — Reply to this email directly or view it on GitHub.

sergesyrota commented 11 years ago

Hmm, I see a mix of tabs and spaces in the old file, and my changeset has an extra tab to everything that went into "else" clause. What is your standard? 4 spaces, or tabs? Alternatively, the most easy to read changeset would be if I can do an early return after processing cookie, instead of having an else clause and indenting everything. But I was not able to figure out how that's done with VCL. Standard return() does not work.

Let me know.

Best Regards, Sergey Syrota

On Fri, Jan 11, 2013 at 9:54 AM, Serge Syrota serg.syrota@gmail.com wrote:

Ok, will do tonight.

For user agents, we actually replaced your regexps with a different set to match what we have on the backend (for if Varnish is down, or in maintenance). And we just implemented it yesterday, so Haven't had a chance to see any anomalies. But if I come across anything, I'll let you know.

Best regards, Serge Syrota

On Jan 11, 2013, at 9:38 AM, Lasse Karstensen notifications@github.com wrote:

Hi Sergey.

I agree with your reasoning. Please re-send the patch without the indentation problems.

Since you have this in production, you perhaps also have some fixes related to user-agents? I'd be very grateful for any such updates.

Thanks.

With regards, Lasse Karstensen Varnish Software AS

— Reply to this email directly or view it on GitHubhttps://github.com/varnish/varnish-devicedetect/pull/3#issuecomment-12149263.

lkarsten commented 11 years ago

On Fri, Jan 11, 2013 at 06:37:40PM -0800, sergesyrota wrote:

Hmm, I see a mix of tabs and spaces in the old file, and my changeset has an extra tab to everything that went into "else" clause. What is your standard? 4 spaces, or tabs? Alternatively, the most easy to read changeset would be if I can do an early return after processing cookie, instead of having an else clause and indenting everything. But I was not able to figure out how that's done with VCL. Standard return() does not work.

Hi.

Thanks for pointing this out. There have been multiple sources for these regular expressions, and some were just copy&pasted in.

I've now reformatted the files according to Varnish code standards, and in addition moved the override handling to the top as you suggested. There is no early return, if-else and indent is the only construct VCL has for this.

Thank you for the contribution.

With regards, Lasse Karstensen Varnish Software AS

sergesyrota commented 11 years ago

Ok, great :)

Best regards, Serge Syrota

On Jan 15, 2013, at 5:14 AM, Lasse Karstensen notifications@github.com wrote:

On Fri, Jan 11, 2013 at 06:37:40PM -0800, sergesyrota wrote:

Hmm, I see a mix of tabs and spaces in the old file, and my changeset has an extra tab to everything that went into "else" clause. What is your standard? 4 spaces, or tabs? Alternatively, the most easy to read changeset would be if I can do an early return after processing cookie, instead of having an else clause and indenting everything. But I was not able to figure out how that's done with VCL. Standard return() does not work.

Hi.

Thanks for pointing this out. There have been multiple sources for these regular expressions, and some were just copy&pasted in.

I've now reformatted the files according to Varnish code standards, and in addition moved the override handling to the top as you suggested. There is no early return, if-else and indent is the only construct VCL has for this.

Thank you for the contribution.

With regards, Lasse Karstensen Varnish Software AS — Reply to this email directly or view it on GitHub.