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

MaxMindBinaryProvider is not lazy and adds overhead to every request #301

Closed
fzaninotto opened this Issue Apr 1, 2014 · 7 comments

Comments

Projects
None yet
3 participants
@fzaninotto

fzaninotto commented Apr 1, 2014

The GeoIP composer library (maxmind/geoip-api-php), which is required when using the MaxMindBinaryProvider, includes several files for every request (NOT lazy):

  • /geoip/geoip/src/geoip.inc
  • /geoip/geoip/src/geoipcity.inc
    • /geoip/geoip/src/geoipregiovars.inc
  • /geoip/geoip/src/timezone.php

This is a know issue, that the maxmind team chose to not fix (cf maxmind/geoip-api-php#8).

This adds between 2 and 4ms in production to every request (with APC enabled). The only way to avoid that is to remove the dependency to the GeoIP composer package, which forces the loading in its composer.json:

    "autoload": {
        "files": [
        "src/geoip.inc",
        "src/geoipcity.inc",
        "src/timezone.php"
        ]
    },

In my opinion, this justifies a more complicated installation procedure for the GeoIP provider.

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 1, 2014

Member

I don't get your point. What should we do? Explaining in the doc that the GeoIP provider is not that good, because of the issue you mention here?

Member

willdurand commented Apr 1, 2014

I don't get your point. What should we do? Explaining in the doc that the GeoIP provider is not that good, because of the issue you mention here?

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Apr 1, 2014

I think Geocoder should not rely on geoip/geoip, but ask the user to download manually GeoIP data to a certain location. Then the user could configure that location in Geocoder, so that the GeoIP provider can (lazy-)load the data.

fzaninotto commented Apr 1, 2014

I think Geocoder should not rely on geoip/geoip, but ask the user to download manually GeoIP data to a certain location. Then the user could configure that location in Geocoder, so that the GeoIP provider can (lazy-)load the data.

@oschwald

This comment has been minimized.

Show comment
Hide comment
@oschwald

oschwald Apr 2, 2014

Contributor

Maybe it could be replaced with GeoIP2. With the legacy database design, a lot of data ended up in the API, which is a primary factor here.

Contributor

oschwald commented Apr 2, 2014

Maybe it could be replaced with GeoIP2. With the legacy database design, a lot of data ended up in the API, which is a primary factor here.

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 2, 2014

Member

Mmmmh, so you are talking about the MaxMindBinaryProvider, right? I do agree that such provider is far from good, especially because of the third-party dependency it uses under the hood...

What about using the GeoipProvider? It relies on the geoip PHP ext, and it seems to be what you are looking for.

Member

willdurand commented Apr 2, 2014

Mmmmh, so you are talking about the MaxMindBinaryProvider, right? I do agree that such provider is far from good, especially because of the third-party dependency it uses under the hood...

What about using the GeoipProvider? It relies on the geoip PHP ext, and it seems to be what you are looking for.

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Apr 2, 2014

Right, we're using the MaxMindBinaryProvider. I'll look at the GeoipProvider, but since it implies some work on the ops side, I'm not sure it's a very good alternative. Besides, does it perform better when profiling? I want to be sure that the time invested to switch providers will produce results.

Even if the GeoIPProvider solves our problem, I suggest that you add a warning about the MaxMindBinaryProvider, since it should not be used in production IMO.

fzaninotto commented Apr 2, 2014

Right, we're using the MaxMindBinaryProvider. I'll look at the GeoipProvider, but since it implies some work on the ops side, I'm not sure it's a very good alternative. Besides, does it perform better when profiling? I want to be sure that the time invested to switch providers will produce results.

Even if the GeoIPProvider solves our problem, I suggest that you add a warning about the MaxMindBinaryProvider, since it should not be used in production IMO.

@fzaninotto fzaninotto changed the title from GeoIP provider is not lazy and adds overhead to every request to MaxMindBinaryProvider is not lazy and adds overhead to every request Apr 2, 2014

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Apr 2, 2014

updated issue title and description to lift the misunderstanding

fzaninotto commented Apr 2, 2014

updated issue title and description to lift the misunderstanding

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 2, 2014

Member

I don't have any benchmarks, I never used these providers myself honestly...

I'm going to update the doc first, and open an issue to introduce a new provider, wrapping the GeoIP2 lib.

Member

willdurand commented Apr 2, 2014

I don't have any benchmarks, I never used these providers myself honestly...

I'm going to update the doc first, and open an issue to introduce a new provider, wrapping the GeoIP2 lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment