Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.1, 1.2
    • Fix Version/s: 1.1, 1.2
    • Component/s: None
    • Labels:
      None
    • Skill Level:
      Regular Contributors Level (Easy to Medium)

      Description

      New vhost manager. allows dynamic add of vhosts without restart, wildcard in vhost and specific functions in erlang by kind of domain. It also fix issue in etap test (160) .

      Find attached to this ticket the patch. It is also available in my github repo :
      http://github.com/benoitc/couchdb/commit/435c756cc6e687886cc7055302963f422cf0e161

      more details :

      This gen_server keep state of vhosts added to the ini and try to
      match the Host header (or forwarded) against rules built against
      vhost list.

      Declaration of vhosts take place in the configuration file :

      [vhosts]
      example.com = /example
      *.example.com = /example

      The first line will rewrite the rquest to display the content of the
      example database. This rule works only if the Host header is
      'example.com' and won't work for CNAMEs. Second rule on the other hand
      match all CNAMES to example db. So www.example.com or db.example.com
      will work.

      The wildcard ('*') should always be the last in the cnames:

      "*.db.example.com = /" will match all cname on top of db
      examples to the root of the machine. (for now no rewriting is
      possible).

      By default vhosts redirection is handle by the function
      couch_httpd_vhost:redirect_to_vhost, but you could pass per vhost a
      specific function :

      "*.domain.com" =

      {Module, Func}

      "

      The function take the Mochiweb Request object and should return a new
      Mochiweb Request object.

      You could also change the default function to handle request by
      changing the setting `redirect_vhost_handler` in `httpd` section of
      the Ini:

      [httpd]
      redirect_vhost_handler =

      {Module, Fun}

      The function take 2 args : the mochiweb request object and the target
      path.

      1. COUCHDB-855-2.patch
        7 kB
        Benoit Chesneau
      2. COUCHDB-855-1.patch
        6 kB
        Benoit Chesneau
      3. COUCHDB-855_20110224-2.patch
        9 kB
        Benoit Chesneau
      4. COUCHDB-855_20110224-1.patch
        9 kB
        Benoit Chesneau
      5. COUCHDB-855_20110224-1.patch
        9 kB
        Benoit Chesneau
      6. 0001-Squashed-commit-of-the-following.patch
        22 kB
        Benoit Chesneau
      7. 0001-fix-COUCHDB-855-.-now-the-gen_server-is-only-used-to.patch
        9 kB
        Benoit Chesneau
      8. 0001-fix-COUCHDB-855-.-now-the-gen_server-is-only-used-to.patch
        9 kB
        Benoit Chesneau
      9. 0001-fix-COUCHDB-855-.-now-the-gen_server-is-only-used-to.patch
        9 kB
        Benoit Chesneau

        Activity

        Hide
        Chris Anderson added a comment -

        patch looks great! thanks so much. only change I'd suggest is removing this change:

        basicView : {
        map : stringFun(function(doc)

        { - emit(doc.integer, doc.string); + if (doc.integer && doc.string) + emit(doc.integer, doc.string); })
        },
        withReduce : {
        map : stringFun(function(doc) { - emit(doc.integer, doc.string); + if (doc.integer && doc.string) + emit(doc.integer, doc.string); }

        ),

        I know these guard conditions are good practice, but in real life there are lots of "bad" views out there, so we should ensure the tests pass even without the guards.

        Show
        Chris Anderson added a comment - patch looks great! thanks so much. only change I'd suggest is removing this change: basicView : { map : stringFun(function(doc) { - emit(doc.integer, doc.string); + if (doc.integer && doc.string) + emit(doc.integer, doc.string); }) }, withReduce : { map : stringFun(function(doc) { - emit(doc.integer, doc.string); + if (doc.integer && doc.string) + emit(doc.integer, doc.string); } ), I know these guard conditions are good practice, but in real life there are lots of "bad" views out there, so we should ensure the tests pass even without the guards.
        Hide
        Benoit Chesneau added a comment -

        mmm?

        Show
        Benoit Chesneau added a comment - mmm?
        Hide
        Benoit Chesneau added a comment -

        new version of the patch allowing Host rewriting :

        Like in the _rewrite handler you could match some variable and use them to create the target path. Some examples:

        [vhosts]
        .example.com = /
        $dbname.example.com = /$dbname
        $ddocname.$dbname.example.com = /$dbname/_design/$ddocname/_rewrite

        First rule pass wildcard as dbname, second do the same but use a variable name and the third one allows you to use any app with $ddocname in any db with $dbname .

        Show
        Benoit Chesneau added a comment - new version of the patch allowing Host rewriting : Like in the _rewrite handler you could match some variable and use them to create the target path. Some examples: [vhosts] .example.com = / $dbname.example.com = /$dbname $ddocname.$dbname.example.com = /$dbname/_design/$ddocname/_rewrite First rule pass wildcard as dbname, second do the same but use a variable name and the third one allows you to use any app with $ddocname in any db with $dbname .
        Hide
        Benoit Chesneau added a comment -

        commited.

        Show
        Benoit Chesneau added a comment - commited.
        Hide
        Filipe Manana added a comment -

        Sorry for this insanely late reply. I missed the introduction of this patch completely.

        I totally disagree making this a gen_server. This can have a very serious impact on performance when thousands of parallel requests come in.

        The reason is that for each received request, a synchronous call is made against the vhost gen_server:

        https://github.com/apache/couchdb/blob/trunk/src/couchdb/couch_httpd.erl#L172

        The call itself basically does string comparsions, splitting, proplists lookups etc. All is CPU bound:

        https://github.com/apache/couchdb/blob/trunk/src/couchdb/couch_httpd_vhost.erl#L145

        This means, that like couch_file, couch_db_updater and couch_server, this is a serialization point, seriously limiting the amount of parallelism we can offer.

        Is there any reason to make this a gen_server? To me it seems like a simple function call with closure variables (to avoid doing the couch_config:get calls, re:split, couch_httpd:make_arity_2_fun calls, etc everytime the mochiweb loop is called)

        Therefore, sorry to say, my vote is -1 solely by the fact it adds a bottleneck

        Show
        Filipe Manana added a comment - Sorry for this insanely late reply. I missed the introduction of this patch completely. I totally disagree making this a gen_server. This can have a very serious impact on performance when thousands of parallel requests come in. The reason is that for each received request, a synchronous call is made against the vhost gen_server: https://github.com/apache/couchdb/blob/trunk/src/couchdb/couch_httpd.erl#L172 The call itself basically does string comparsions, splitting, proplists lookups etc. All is CPU bound: https://github.com/apache/couchdb/blob/trunk/src/couchdb/couch_httpd_vhost.erl#L145 This means, that like couch_file, couch_db_updater and couch_server, this is a serialization point, seriously limiting the amount of parallelism we can offer. Is there any reason to make this a gen_server? To me it seems like a simple function call with closure variables (to avoid doing the couch_config:get calls, re:split, couch_httpd:make_arity_2_fun calls, etc everytime the mochiweb loop is called) Therefore, sorry to say, my vote is -1 solely by the fact it adds a bottleneck
        Hide
        Paul Joseph Davis added a comment -

        I'd say its a bit late after six months to -1 a patch.

        I'd be +1 on looking at results on bringing the binding functions into the calling process instead of running in the gen_server. I would also be interested in seeing whether using the gen_server to store vhost rule state vs refetching from couch_config and re-parsing per request.

        Show
        Paul Joseph Davis added a comment - I'd say its a bit late after six months to -1 a patch. I'd be +1 on looking at results on bringing the binding functions into the calling process instead of running in the gen_server. I would also be interested in seeing whether using the gen_server to store vhost rule state vs refetching from couch_config and re-parsing per request.
        Hide
        Benoit Chesneau added a comment -

        Main reason is that actually the hard restart in couch_httpd is really insane and was the cause of a lot of problem. You can check it by using previous patch that wasn't using this gen_server for the same feature. There are mail about this I think.

        Anyway I want to rework this feature to improve it::

        • 1. Caching Host patterns so we don't have to rework them each time
        • 2. Improve pattern matching and make it faster by preprocessing rules.

        Let me some days to extract it from my codebase.

        Show
        Benoit Chesneau added a comment - Main reason is that actually the hard restart in couch_httpd is really insane and was the cause of a lot of problem. You can check it by using previous patch that wasn't using this gen_server for the same feature. There are mail about this I think. Anyway I want to rework this feature to improve it:: 1. Caching Host patterns so we don't have to rework them each time 2. Improve pattern matching and make it faster by preprocessing rules. Let me some days to extract it from my codebase.
        Hide
        Filipe Manana added a comment -

        Yep, I agree it's really late to -1 it. I never noticed before about this gen_server synchronous call.
        Nevertheless the -1 is only because of this specific implementation. The features are unquestionably useful and +1 for that.

        Like Paul said, it would be interesting to know how much time is spent fetching the configs from couch_config, doing the necessary parsing and matchings, etc. I think to have a good idea about it something like this is ok:

        T0 = erlang:now(),
        % do the vhost stuff
        T1 = erlang:now(),
        io:format("time spent doing vhost stuff: ~pus", [timer:now_diff(T1, T0)])

        Measuring the whole time of the request, in a similar fashion, grabbing T0 at the very beginning of the request handler loop and T1 at the very end would allows us to have a better idea of the relative time spent doing the vhosts stuff. Basically similar to what I did for COUCHDB-913

        I would say a cache is only worth if those values are significant, maybe if they're > 5 - 20ms or > 5 - 10% of the total time spent for the request. Cache lookup times might not be much lower than the time spent doing the vhosts stuff (independently of having the cache implemented with a protected/public ets or a gen_server).

        Show
        Filipe Manana added a comment - Yep, I agree it's really late to -1 it. I never noticed before about this gen_server synchronous call. Nevertheless the -1 is only because of this specific implementation. The features are unquestionably useful and +1 for that. Like Paul said, it would be interesting to know how much time is spent fetching the configs from couch_config, doing the necessary parsing and matchings, etc. I think to have a good idea about it something like this is ok: T0 = erlang:now(), % do the vhost stuff T1 = erlang:now(), io:format("time spent doing vhost stuff: ~pus", [timer:now_diff(T1, T0)] ) Measuring the whole time of the request, in a similar fashion, grabbing T0 at the very beginning of the request handler loop and T1 at the very end would allows us to have a better idea of the relative time spent doing the vhosts stuff. Basically similar to what I did for COUCHDB-913 I would say a cache is only worth if those values are significant, maybe if they're > 5 - 20ms or > 5 - 10% of the total time spent for the request. Cache lookup times might not be much lower than the time spent doing the vhosts stuff (independently of having the cache implemented with a protected/public ets or a gen_server).
        Hide
        Paul Joseph Davis added a comment -

        Yeah. My bet for superfastest implementation would probably be a publically readable ets table that used lookups to find matching vhost definitions that the gen_server updated in response to config changes.

        Also, if memory serves, this code hasn't yet been switched over to use exported functions in config callbacks.

        Show
        Paul Joseph Davis added a comment - Yeah. My bet for superfastest implementation would probably be a publically readable ets table that used lookups to find matching vhost definitions that the gen_server updated in response to config changes. Also, if memory serves, this code hasn't yet been switched over to use exported functions in config callbacks.
        Hide
        Filipe Manana added a comment -

        Yep, like couch_auth_cache does. The insertions would be done by doing a cast to the gen_server, which in turn would add the entry to the ets. Let's see first how expensive these vhost related checks/mappings are.

        Show
        Filipe Manana added a comment - Yep, like couch_auth_cache does. The insertions would be done by doing a cast to the gen_server, which in turn would add the entry to the ets. Let's see first how expensive these vhost related checks/mappings are.
        Hide
        Benoit Chesneau added a comment -

        I'm currently writing different implementation:

        1. Rather than using pattern matching each time and bind list of
        substitutions on the fly, I compile rule first and place it in a
        public ets. (having a generic caching service in couch would help)

        Rules are first compiled at startup, then if config change they are
        reinstalled.

        2. On request, the new vhost is matching first the port then if host
        match and finally replace substitution in target.

        I removed completly the gen_server, and the service should be faster.
        Patch is coming.

        Show
        Benoit Chesneau added a comment - I'm currently writing different implementation: 1. Rather than using pattern matching each time and bind list of substitutions on the fly, I compile rule first and place it in a public ets. (having a generic caching service in couch would help) Rules are first compiled at startup, then if config change they are reinstalled. 2. On request, the new vhost is matching first the port then if host match and finally replace substitution in target. I removed completly the gen_server, and the service should be faster. Patch is coming.
        Hide
        Benoit Chesneau added a comment -

        Here is a patch removing the need of a gen_server. Works the same except rules are pre installed in application environement when couch_httpd is started.

        Pas tests with previous patch in couch_httpd changing the way config changes are handled.

        Show
        Benoit Chesneau added a comment - Here is a patch removing the need of a gen_server. Works the same except rules are pre installed in application environement when couch_httpd is started. Pas tests with previous patch in couch_httpd changing the way config changes are handled.
        Hide
        Benoit Chesneau added a comment -

        If you're ok with this one I will commit it.

        Show
        Benoit Chesneau added a comment - If you're ok with this one I will commit it.
        Hide
        Benoit Chesneau added a comment -

        Without using the gen_server, we have to rebuild the redirect vhost handler function each time. Also previous patch was loading the vhosts global list on each request like it was before which make host dispatching slower.

        Thhis new patch load vhosts and vhosts gllobal list only when couch_httpd is started. Values are saved in appliucation env. This version is still a little slower than previous (0.134MS AGAINS 0.110 MS) but a lot better than the 0.234ms we had before.

        Show
        Benoit Chesneau added a comment - Without using the gen_server, we have to rebuild the redirect vhost handler function each time. Also previous patch was loading the vhosts global list on each request like it was before which make host dispatching slower. Thhis new patch load vhosts and vhosts gllobal list only when couch_httpd is started. Values are saved in appliucation env. This version is still a little slower than previous (0.134MS AGAINS 0.110 MS) but a lot better than the 0.234ms we had before.
        Hide
        Benoit Chesneau added a comment -

        New patch have been provided did you have time to check it in term of
        performances ? Now that 1.0.2 is (mostly) out, I think it would be
        good to have it in 1.1 before they go.

        I've etsted before/after this patch. Didn't noticed lot of
        performances changes, but I guess it can be noticed when you have a
        lot of concurrent connections, since the gen_server will serialize
        them. Maybe a test with relaximation ?

        • benoit
        Show
        Benoit Chesneau added a comment - New patch have been provided did you have time to check it in term of performances ? Now that 1.0.2 is (mostly) out, I think it would be good to have it in 1.1 before they go. I've etsted before/after this patch. Didn't noticed lot of performances changes, but I guess it can be noticed when you have a lot of concurrent connections, since the gen_server will serialize them. Maybe a test with relaximation ? benoit
        Hide
        Filipe Manana added a comment -

        Hi Benoit, thanks

        As soon as I get enough free time, I'll try the patch on my machine.

        Show
        Filipe Manana added a comment - Hi Benoit, thanks As soon as I get enough free time, I'll try the patch on my machine.
        Hide
        Benoit Chesneau added a comment -

        put this issue as blocker, since some thinks it's a problem. Need feedback.

        Show
        Benoit Chesneau added a comment - put this issue as blocker, since some thinks it's a problem. Need feedback.
        Hide
        Filipe Manana added a comment -

        Benoît,

        how did you measure those times? Are they averages? Were they measured in a concurrent or a non-concurrent scenario?

        I tested with a single machine, running both CouchDB and the relaximation test, with the existing gen_server approach and your latest patch. The test was done like this:

        $ node tests/test_writes.js --clients 500 --url http://localhost:5984/ --duration 90

        For each one of the alternatives (gen_server vs no gen_server), I added an io:format, in couch_httpd.erl, call like this:

        + T0 = erlang:now(),
        + MochiReq1 = couch_httpd_vhost:dispatch_host(MochiReq),
        + io:format("vhost lookup took ~p~n", [timer:now_diff(erlang:now(), T0)]),

        Now, the basic analysis I did:

        >>> without the gen_server (latest patch)

        http://graphs.mikeal.couchone.com/#/graph/0379dbdaef29b1c0fbf0342154019ed3

        $ egrep 'vhost lookup' log_vhosts_no_server | wc -l
        64251

        20 more frequent times

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | head -20
        201 33
        140 34
        121 36
        115 35
        108 32
        103 37
        97 43
        92 41
        86 42
        82 44
        79 40
        77 45
        76 39
        73 38
        68 47
        67 48
        58 46
        57 50
        54 52
        54 49

        20 less frequent times

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | tail -20
        1 10046
        1 10043
        1 10042
        1 10037
        1 10036
        1 10035
        1 10032
        1 10029
        1 10028
        1 10022
        1 10018
        1 10014
        1 10013
        1 10011
        1 10009
        1 10008
        1 10005
        1 10003
        1 10002
        1 10000

        number of lookups that took less than 50us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 50' | wc -l
        1704

        (about 2,7%)

        number of lookups that took less than 100us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 100' | wc -l
        3374

        (about 5,3%)

        number of lookups that took less than 200us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 200' | wc -l
        5921

        (about 8,2%)

        number of lookups that took less than 500us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 500' | wc -l
        14341

        (about 22,3%)

        number of lookups that took less than 1000us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 1000' | wc -l
        24259

        (about 37,8%)

        number of lookups that took less than 2000us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 2000' | wc -l
        35153

        (about 54,7%)

        number of lookups that took less than 3000us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 3000' | wc -l
        41630

        (about 64,8%)

        number of lookups that took less than 4000us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 4000' | wc -l
        46452

        (about 72,3%)

        number of lookups that took less than 5000us

        $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 5000' | wc -l
        49603

        (about 77,2%)

        >>> with the gen_server (current trunk)

        http://graphs.mikeal.couchone.com/#/graph/0379dbdaef29b1c0fbf034215401b7d4

        $ egrep 'vhost lookup' log_vhosts_gen_server | wc -l
        62736

        20 more frequent response times

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | head -20
        198 33
        156 34
        118 32
        116 35
        115 36
        110 37
        109 42
        104 44
        94 41
        92 39
        91 43
        87 45
        86 40
        78 46
        66 51
        65 38
        62 48
        61 49
        61 47
        59 62

        20 less frequent response times

        egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | tail -20
        1 10047
        1 10045
        1 10042
        1 10041
        1 10033
        1 10032
        1 10030
        1 10028
        1 10026
        1 10024
        1 10020
        1 10019
        1 10016
        1 10013
        1 10010
        1 10009
        1 10008
        1 10007
        1 10003
        1 10000

        number of lookups that took less than 50us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 50' | wc -l
        1818

        (about 2,8%)

        number of lookups that took less than 100us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 100' | wc -l
        3443

        (about 5,4%)

        number of lookups that took less than 200us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 200' | wc -l
        6150

        (about 9,8%)

        number of lookups that took less than 500us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 500' | wc -l
        14307

        (about 22,9%)

        number of lookups that took less than 1000us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 1000' | wc -l
        23908

        (about 38,1%)

        number of lookups that took less than 2000us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 2000' | wc -l
        34275

        (about 54,6%)

        number of lookups that took less than 3000us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 3000' | wc -l
        40457

        (about 64,5%)

        number of lookups that took less than 4000us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 4000' | wc -l
        44858

        (about 71,5%)

        number of lookups that took less than 5000us

        $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 5000' | wc -l
        47775

        (about 76,1%)

        I made a few runs for each approach, and the total number of requests served by the gen_server approach is always smaller (about 1000 to 2000 less) compared to the non-gen_server approach (perhaps it's a coincidence)

        I tend to favour the non-gen_server approach. A more complete and realistic test would consist of having several machines doing thousands of requests in parallel against the Couch server. Unfortunately I only have one right now and will be out for the next 2 weeks.

        Let's see what others think of this. Paul?

        Show
        Filipe Manana added a comment - Benoît, how did you measure those times? Are they averages? Were they measured in a concurrent or a non-concurrent scenario? I tested with a single machine, running both CouchDB and the relaximation test, with the existing gen_server approach and your latest patch. The test was done like this: $ node tests/test_writes.js --clients 500 --url http://localhost:5984/ --duration 90 For each one of the alternatives (gen_server vs no gen_server), I added an io:format, in couch_httpd.erl, call like this: + T0 = erlang:now(), + MochiReq1 = couch_httpd_vhost:dispatch_host(MochiReq), + io:format("vhost lookup took ~p~n", [timer:now_diff(erlang:now(), T0)] ), Now, the basic analysis I did: >>> without the gen_server (latest patch) http://graphs.mikeal.couchone.com/#/graph/0379dbdaef29b1c0fbf0342154019ed3 $ egrep 'vhost lookup' log_vhosts_no_server | wc -l 64251 20 more frequent times $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | head -20 201 33 140 34 121 36 115 35 108 32 103 37 97 43 92 41 86 42 82 44 79 40 77 45 76 39 73 38 68 47 67 48 58 46 57 50 54 52 54 49 20 less frequent times $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | tail -20 1 10046 1 10043 1 10042 1 10037 1 10036 1 10035 1 10032 1 10029 1 10028 1 10022 1 10018 1 10014 1 10013 1 10011 1 10009 1 10008 1 10005 1 10003 1 10002 1 10000 number of lookups that took less than 50us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 50' | wc -l 1704 (about 2,7%) number of lookups that took less than 100us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 100' | wc -l 3374 (about 5,3%) number of lookups that took less than 200us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 200' | wc -l 5921 (about 8,2%) number of lookups that took less than 500us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 500' | wc -l 14341 (about 22,3%) number of lookups that took less than 1000us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 1000' | wc -l 24259 (about 37,8%) number of lookups that took less than 2000us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 2000' | wc -l 35153 (about 54,7%) number of lookups that took less than 3000us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 3000' | wc -l 41630 (about 64,8%) number of lookups that took less than 4000us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 4000' | wc -l 46452 (about 72,3%) number of lookups that took less than 5000us $ egrep 'vhost lookup' log_vhosts_no_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 5000' | wc -l 49603 (about 77,2%) >>> with the gen_server (current trunk) http://graphs.mikeal.couchone.com/#/graph/0379dbdaef29b1c0fbf034215401b7d4 $ egrep 'vhost lookup' log_vhosts_gen_server | wc -l 62736 20 more frequent response times $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | head -20 198 33 156 34 118 32 116 35 115 36 110 37 109 42 104 44 94 41 92 39 91 43 87 45 86 40 78 46 66 51 65 38 62 48 61 49 61 47 59 62 20 less frequent response times egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | sort -n | uniq -c | sort -nr | tail -20 1 10047 1 10045 1 10042 1 10041 1 10033 1 10032 1 10030 1 10028 1 10026 1 10024 1 10020 1 10019 1 10016 1 10013 1 10010 1 10009 1 10008 1 10007 1 10003 1 10000 number of lookups that took less than 50us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 50' | wc -l 1818 (about 2,8%) number of lookups that took less than 100us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 100' | wc -l 3443 (about 5,4%) number of lookups that took less than 200us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 200' | wc -l 6150 (about 9,8%) number of lookups that took less than 500us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 500' | wc -l 14307 (about 22,9%) number of lookups that took less than 1000us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 1000' | wc -l 23908 (about 38,1%) number of lookups that took less than 2000us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 2000' | wc -l 34275 (about 54,6%) number of lookups that took less than 3000us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 3000' | wc -l 40457 (about 64,5%) number of lookups that took less than 4000us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 4000' | wc -l 44858 (about 71,5%) number of lookups that took less than 5000us $ egrep 'vhost lookup' log_vhosts_gen_server | cut -d ' ' -f 4 | perl -ne 'print $_ if $_ < 5000' | wc -l 47775 (about 76,1%) I made a few runs for each approach, and the total number of requests served by the gen_server approach is always smaller (about 1000 to 2000 less) compared to the non-gen_server approach (perhaps it's a coincidence) I tend to favour the non-gen_server approach. A more complete and realistic test would consist of having several machines doing thousands of requests in parallel against the Couch server. Unfortunately I only have one right now and will be out for the next 2 weeks. Let's see what others think of this. Paul?
        Hide
        Benoit Chesneau added a comment - - edited

        Just used curl + xargs with file edited to display times. Figures are differents, but they have been done on a laptop (think x201i i3) . Thanks for the figures anyway .

        But yes so :

        • gen_server give a constant time until the number of concurrent requests become high. That sound logical since it's serialized. Speed is relatively constant because it doesn't have to reparse config each time.

        There are not so many service that handle 1000 simultaneous requests but well, I'm too for the version without gen_server. It works now due to previous change in couch_httpd on config changes handling.

        Now the last bit to solve is the call to the application:get_env/set_env. Paul is in favor of using a gen_server for that. Other methode I can see, which is used in some other projects is to use ets:

        ets:new(?MODULE, [named_table, public]) ....

        If we use a generic gen_server it may be used for other purposes I guess. I've no preference for on or the other. Except that we shouldn't use too much ets which was the reason I used application module functions.

        Show
        Benoit Chesneau added a comment - - edited Just used curl + xargs with file edited to display times. Figures are differents, but they have been done on a laptop (think x201i i3) . Thanks for the figures anyway . But yes so : gen_server give a constant time until the number of concurrent requests become high. That sound logical since it's serialized. Speed is relatively constant because it doesn't have to reparse config each time. There are not so many service that handle 1000 simultaneous requests but well, I'm too for the version without gen_server. It works now due to previous change in couch_httpd on config changes handling. Now the last bit to solve is the call to the application:get_env/set_env. Paul is in favor of using a gen_server for that. Other methode I can see, which is used in some other projects is to use ets: ets:new(?MODULE, [named_table, public] ) .... If we use a generic gen_server it may be used for other purposes I guess. I've no preference for on or the other. Except that we shouldn't use too much ets which was the reason I used application module functions.
        Hide
        Paul Joseph Davis added a comment -

        Those numbers make me thing you should start playing with basho_bench cause I have no idea what they mean.

        My reasoning for using the gen_server instead of an ets table or get/set env functions is that at the moment it doesn't seem like ets is going to win much over the gen_server model and using a complete ets table for a single value seems a tidge silly.

        Show
        Paul Joseph Davis added a comment - Those numbers make me thing you should start playing with basho_bench cause I have no idea what they mean. My reasoning for using the gen_server instead of an ets table or get/set env functions is that at the moment it doesn't seem like ets is going to win much over the gen_server model and using a complete ets table for a single value seems a tidge silly.
        Hide
        Benoit Chesneau added a comment -

        Well that why i was using application environment . Application environement is managed by an application_controller module and all applications settings are added to an ets table:

        https://github.com/erlang/otp/blob/dev/lib/kernel/src/application_controller.erl#L870

        Maybe an alternative would be to maintain our own couch_env gen_server. I see some other uses for that like maintaining plugins and such. What do you think ?

        Show
        Benoit Chesneau added a comment - Well that why i was using application environment . Application environement is managed by an application_controller module and all applications settings are added to an ets table: https://github.com/erlang/otp/blob/dev/lib/kernel/src/application_controller.erl#L870 Maybe an alternative would be to maintain our own couch_env gen_server. I see some other uses for that like maintaining plugins and such. What do you think ?
        Hide
        Filipe Manana added a comment -

        Yes, there's no need to use the application plus a custom public ets, since the former is basically already a layer on top of a public ets.

        Since there's no general agreement about whether using the gen_server solution versus the non-gen_server solution (Paul seems to prefer the former, and I the last), it's up to you to decide which to use.
        Feel free to close the ticket with the solution you prefer.

        cheers

        Show
        Filipe Manana added a comment - Yes, there's no need to use the application plus a custom public ets, since the former is basically already a layer on top of a public ets. Since there's no general agreement about whether using the gen_server solution versus the non-gen_server solution (Paul seems to prefer the former, and I the last), it's up to you to decide which to use. Feel free to close the ticket with the solution you prefer. cheers
        Hide
        Paul Joseph Davis added a comment -

        Just a note that I prefer the gen server just store the parsed stuff and just gives that to other processes to do the actual matching. It'd also react to config changes to re-populate the parsed state on configuration change. Using the application environment seemed like it could lead to weirdness that we aren't anticipating. Although that could be because I'm just not familiar enough with the app env, but that's exactly why I'm hesitant to rely on it.

        Show
        Paul Joseph Davis added a comment - Just a note that I prefer the gen server just store the parsed stuff and just gives that to other processes to do the actual matching. It'd also react to config changes to re-populate the parsed state on configuration change. Using the application environment seemed like it could lead to weirdness that we aren't anticipating. Although that could be because I'm just not familiar enough with the app env, but that's exactly why I'm hesitant to rely on it.
        Hide
        Filipe Manana added a comment -

        Good call Paul. I'm not familiar at all either with the application environment, so yes there's always a risk of not using it well or ending up not being reliable enough for this use case. I also think preferable to let the other processes do all the matching and url processing, just keeping the most expensive things in the gen_server, so that the gen_server call does no processing at all (or very very little).

        Show
        Filipe Manana added a comment - Good call Paul. I'm not familiar at all either with the application environment, so yes there's always a risk of not using it well or ending up not being reliable enough for this use case. I also think preferable to let the other processes do all the matching and url processing, just keeping the most expensive things in the gen_server, so that the gen_server call does no processing at all (or very very little).
        Hide
        Paul Joseph Davis added a comment -

        Just to be sure when you said:

        > just keeping the most expensive things in the gen_server,

        you meant keeping them out of the gen server, right? Pretty sure it's just a typo but wanted to double check we're talking about the same gen_server.

        Show
        Paul Joseph Davis added a comment - Just to be sure when you said: > just keeping the most expensive things in the gen_server, you meant keeping them out of the gen server, right? Pretty sure it's just a typo but wanted to double check we're talking about the same gen_server.
        Hide
        Benoit Chesneau added a comment -

        Some app like riak or webmmachine are using app env. I'm pretty sure it's safe. Anyway just to be sure, by using a gen_server, you mean maintaining parsed values in a dict or a state ? (I suppose you don't want an ets here too).

        -benoit

        Show
        Benoit Chesneau added a comment - Some app like riak or webmmachine are using app env. I'm pretty sure it's safe. Anyway just to be sure, by using a gen_server, you mean maintaining parsed values in a dict or a state ? (I suppose you don't want an ets here too). -benoit
        Hide
        Paul Joseph Davis added a comment -

        Using it, and using it correctly are two different things. I'm sure it can be used just not if we should use it here.

        I'm saying that the gen_server stores exactly what you were storing in the application environment, and that config change trigger the gen_server to do the reprocessing of the configuration settings. All the matching logic should take place outside the gen_server process. Each process handling an HTTP request would then make a call to the gen_server to grab a copy of the current pre-processed configuration values.

        Show
        Paul Joseph Davis added a comment - Using it, and using it correctly are two different things. I'm sure it can be used just not if we should use it here. I'm saying that the gen_server stores exactly what you were storing in the application environment, and that config change trigger the gen_server to do the reprocessing of the configuration settings. All the matching logic should take place outside the gen_server process. Each process handling an HTTP request would then make a call to the gen_server to grab a copy of the current pre-processed configuration values.
        Hide
        Filipe Manana added a comment -

        Paul: right, a typo I meant keeping pre-processed stuff in the gen_server's state and giving that to the client (via a simple get call, without any processing) for him to do the processing

        Show
        Filipe Manana added a comment - Paul: right, a typo I meant keeping pre-processed stuff in the gen_server's state and giving that to the client (via a simple get call, without any processing) for him to do the processing
        Hide
        Benoit Chesneau added a comment -

        Patch fixing performance issue. Now the gen_server is only used to keep vhosts settings and reload them when it's needed.

        If it's ok I will commit it.

        Show
        Benoit Chesneau added a comment - Patch fixing performance issue. Now the gen_server is only used to keep vhosts settings and reload them when it's needed. If it's ok I will commit it.
        Hide
        Benoit Chesneau added a comment -

        behaviour line was missing.

        Show
        Benoit Chesneau added a comment - behaviour line was missing.
        Hide
        Benoit Chesneau added a comment -

        behaviour line was missing.

        Show
        Benoit Chesneau added a comment - behaviour line was missing.
        Hide
        Filipe Manana added a comment -

        Thanks Benoît,

        + VHostGlobals = re:split(couch_config:get("httpd",
        + "vhost_global_handlers",""), ", ?",[

        {return, list}

        ]),

        There, I would split by "\\s*,
        s*" instead. We recently had an issue in a comma separated list (compressible types) because there were 2 spaces after a comma ( COUCHDB-1034 ).

        +-define(SERVER, ?MODULE).

        I also don't see that SERVER macro getting used anywhere in the diff.

        cheers

        Show
        Filipe Manana added a comment - Thanks Benoît, + VHostGlobals = re:split(couch_config:get("httpd", + "vhost_global_handlers",""), ", ?",[ {return, list} ]), There, I would split by "\\s*, s*" instead. We recently had an issue in a comma separated list (compressible types) because there were 2 spaces after a comma ( COUCHDB-1034 ). +-define(SERVER, ?MODULE). I also don't see that SERVER macro getting used anywhere in the diff. cheers
        Hide
        Benoit Chesneau added a comment -

        last version of the patch. it ill be commited later today except if someone find anything since.

        Show
        Benoit Chesneau added a comment - last version of the patch. it ill be commited later today except if someone find anything since.
        Hide
        Benoit Chesneau added a comment -

        using only one hand doesn't help. The right patch.

        Show
        Benoit Chesneau added a comment - using only one hand doesn't help. The right patch.
        Hide
        Adam Kocoloski added a comment -

        Hi Benoit, I have a couple of comments:

        1) Use fun ?MODULE:config_change/2 instead of fun config_change/2. I think the latter is just syntactic sugar for fun(A,B) -> config_change(A,B) end, so it doesn't get the benefit of using the export table.

        2) Is the reordering of entries in default.ini.tpl.in significant? I'd be very careful with that. I don't think the order in the .ini file generally matches the output of couch_config:get("daemons"), and the output of that call is what ultimately determines the daemon startup order. If you need to guarantee that the vhost server starts before httpd does you may need to do something different.

        3) I don't see any reason for this server to be trapping exit signals. Suggest removing that process_flag.

        Cheers, Adam

        Show
        Adam Kocoloski added a comment - Hi Benoit, I have a couple of comments: 1) Use fun ?MODULE:config_change/2 instead of fun config_change/2. I think the latter is just syntactic sugar for fun(A,B) -> config_change(A,B) end, so it doesn't get the benefit of using the export table. 2) Is the reordering of entries in default.ini.tpl.in significant? I'd be very careful with that. I don't think the order in the .ini file generally matches the output of couch_config:get("daemons"), and the output of that call is what ultimately determines the daemon startup order. If you need to guarantee that the vhost server starts before httpd does you may need to do something different. 3) I don't see any reason for this server to be trapping exit signals. Suggest removing that process_flag. Cheers, Adam
        Hide
        Benoit Chesneau added a comment -

        right, changing that.

        It was more logical to load vhost layer before httpd. But in fact it's
        not really important since we make sure rules are loaded when httptd
        start via the install function.

        right. since we don't catch it.

        new patch is coming.

        Show
        Benoit Chesneau added a comment - right, changing that. It was more logical to load vhost layer before httpd. But in fact it's not really important since we make sure rules are loaded when httptd start via the install function. right. since we don't catch it. new patch is coming.
        Hide
        Benoit Chesneau added a comment -

        patch with changes suggested by Adam. I kept the new position of vhosts in daemon for pure logic purpose, but it can be reverted if you think there is any reason for that.

        Show
        Benoit Chesneau added a comment - patch with changes suggested by Adam. I kept the new position of vhosts in daemon for pure logic purpose, but it can be reverted if you think there is any reason for that.
        Hide
        Benoit Chesneau added a comment -

        patch with rights granted to ASF. (would be better if it's the default imo)

        Show
        Benoit Chesneau added a comment - patch with rights granted to ASF. (would be better if it's the default imo)
        Hide
        Benoit Chesneau added a comment -

        commited.

        Show
        Benoit Chesneau added a comment - commited.

          People

          • Assignee:
            Benoit Chesneau
            Reporter:
            Benoit Chesneau
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development