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

RequestFactory: Optimize match proxy CIDR with remote IP #220

Closed
wants to merge 1 commit into from

Conversation

jakubboucek
Copy link
Contributor

  • BC break? no
  • doc PR: not relevant

We are using CloudFlare reverse proxy to run production. Cloudflare has proxy servers at 22 IP ranges. Nette RequestFactory is on every request must match user's IP with 22 CIDR IP masks.

$usingTrustedProxy = $remoteAddr && array_filter($this->proxies, fn(string $proxy): bool => Helpers::ipMatch($remoteAddr, $proxy));

⇧ this current construction is consuming unnecessarily much CPU/time, because:

  • PHP is uneffective with call callback fuctions on loop (read more about it from @kaja47)
  • Loop traversing is not lazy – it vainly continue in loop even have match.

Currently it's consuming about ~0.8 ms, that's ~1/10 of process whole request.

This PR suggest a little uglier but much faster constrictions which is up to:

  • 20 × faster when matched (~0.04 ms),
  • 4 × faster when unmatched (~0.2 ms).

No functionality changes.

@dg
Copy link
Member

dg commented Mar 1, 2023

It wouldn't be possible to use Arrays::some here, would it?

(Removing of callbacks is imho unnecessary premature optimization)

@jakubboucek
Copy link
Contributor Author

jakubboucek commented Mar 1, 2023

@dg Is here static callback any benefit except optically it's remove one level nested block?

As I said above, the callback only is slowing whole match up to 4× (independently to lazy loop). I see removing of callback here as necessary optimization.

@dg
Copy link
Member

dg commented Mar 2, 2023

The point is that callbacks are used in a huge number of different places. Nette has no doctrine of "not using callbacks (or Arrays)" for performance reasons. It's possible to establish such a doctrine if it leads to noticeably better performance, for example in the past Nette had a don't use a ternary operator doctrine because it was extremely slow.

So I don't see why in this particular case to do an optimization that saves 0.000001s per request.

@jakubboucek
Copy link
Contributor Author

I am not trying to introduce new general doctrines for Nette. I like callbacks. I like ternary operator. I don't like stupid „academic“ optimizations.

I believe that in this case it is a specific situation where usage of callback has no benefits but it's slowing down goodly optimized app about 1/10 of time for each request. It just for 22 IP ranges for Cloudflare. What about if somebody will use AWS CloudFront where is about 150 IP ranges to match.

Why I mean this is specific case:

  • Callback hasn't here any benefit except habit.
  • It's executed for every single request, including all redirects, ajax, api, etc.
  • Here is not effective way to cache results.
  • It's part of code which is very important for app Security. Is important to preventively make no any barrier which can user demotivate to use it – some company management they tend to prioritize performance over safety.

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.

None yet

2 participants