trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: git commit: TS-2570: Reduce number of malloc/free in remap_stats fast path
Date Fri, 14 Feb 2014 17:19:11 GMT
On Feb 13, 2014, at 3:14 PM, sorber@apache.org wrote:

> Updated Branches:
>  refs/heads/master 6636f4dcf -> 178c7b92a
> 
> 
> TS-2570: Reduce number of malloc/free in remap_stats fast path
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/178c7b92
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/178c7b92
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/178c7b92
> 
> Branch: refs/heads/master
> Commit: 178c7b92a42d3eead77143f78bcf60afbb6d3951
> Parents: 6636f4d
> Author: Phil Sorber <sorber@apache.org>
> Authored: Thu Feb 13 16:09:13 2014 -0700
> Committer: Phil Sorber <sorber@apache.org>
> Committed: Thu Feb 13 16:09:36 2014 -0700
> 
> ----------------------------------------------------------------------
> plugins/experimental/remap_stats/remap_stats.c | 108 ++++++++------------
> 1 file changed, 42 insertions(+), 66 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/178c7b92/plugins/experimental/remap_stats/remap_stats.c
> ----------------------------------------------------------------------
> diff --git a/plugins/experimental/remap_stats/remap_stats.c b/plugins/experimental/remap_stats/remap_stats.c
> index 246796f..04b1723 100644
> --- a/plugins/experimental/remap_stats/remap_stats.c
> +++ b/plugins/experimental/remap_stats/remap_stats.c
> @@ -23,6 +23,7 @@
> #include "ink_defs.h"
> 
> #include "ts/ts.h"
> +#include <stdint.h>
> #include <stdbool.h>
> #include <string.h>
> #include <stdio.h>
> @@ -32,6 +33,8 @@
> #define PLUGIN_NAME "remap_stats"
> #define DEBUG_TAG PLUGIN_NAME
> 
> +#define MAX_STAT_LENGTH (1<<8)
> +
> typedef struct
> {
>     bool post_remap_host;
> @@ -40,16 +43,10 @@ typedef struct
>     TSMutex stat_creation_mutex;
> } config_t;
> 
> -typedef struct
> -{
> -    char *hostname;
> -    bool remap_success;
> -} txn_data_t;
> -
> static void
> stat_add(char *name, TSMgmtInt amount, TSStatPersistence persist_type, TSMutex create_mutex)
> {
> -    int stat_id = -1, *statp;
> +    intptr_t stat_id = -1;

If you make this a union, you don't need so many fiddly casts later ...

>     ENTRY search, *result = NULL;
>     static __thread struct hsearch_data stat_cache;
>     static __thread bool hash_init = false;
> @@ -70,30 +67,28 @@ stat_add(char *name, TSMgmtInt amount, TSStatPersistence persist_type,
TSMutex c
>         // so this mutex won't be much overhead and it fixes a race condition
>         // in the RecCore. Hopefully this can be removed in the future.
>         TSMutexLock(create_mutex);
> -        if (TS_ERROR == TSStatFindName((const char *) name, &stat_id))
> +        if (TS_ERROR == TSStatFindName((const char *) name, (int *)&stat_id))
>         {
>             stat_id = TSStatCreate((const char *) name, TS_RECORDDATATYPE_INT, persist_type,
TS_STAT_SYNC_SUM);
>             if (stat_id == TS_ERROR)
>                 TSDebug(DEBUG_TAG, "Error creating stat_name: %s", name);
>             else
> -                TSDebug(DEBUG_TAG, "Created stat_name: %s stat_id: %d", name, stat_id);
> +                TSDebug(DEBUG_TAG, "Created stat_name: %s stat_id: %d", name, (int)stat_id);
>         }
>         TSMutexUnlock(create_mutex);
> 
>         search.key = TSstrdup(name);
> -        statp = TSmalloc(sizeof(int));
> -        *statp = stat_id;
> -        search.data = (void *) statp;
> +        search.data = (void *) stat_id;
>         hsearch_r(search, ENTER, &result, &stat_cache);
> -        TSDebug(DEBUG_TAG, "Cached stat_name: %s stat_id: %d", name, stat_id);
> +        TSDebug(DEBUG_TAG, "Cached stat_name: %s stat_id: %d", name, (int)stat_id);
>     }
>     else
> -        stat_id = *((int *) result->data);
> +        stat_id = (intptr_t) result->data;
> 
>     if (likely(stat_id >= 0))
> -        TSStatIntIncrement(stat_id, amount);
> +        TSStatIntIncrement((int)stat_id, amount);
>     else
> -        TSDebug(DEBUG_TAG, "stat error! stat_name: %s stat_id: %d", name, stat_id);
> +        TSDebug(DEBUG_TAG, "stat error! stat_name: %s stat_id: %d", name, (int)stat_id);
> }
> 
> static char *
> @@ -118,30 +113,16 @@ get_effective_host(TSHttpTxn txn)
>     return tmp;
> }
> 
> -static char *
> -create_stat_name(char *hostname, char *basename)
> -{
> -    char *stat_name;
> -    size_t stat_len;
> -
> -    stat_len = strlen(hostname) + strlen(basename) + strlen(PLUGIN_NAME) + 3 + strlen("plugin.");
> -    stat_name = TSmalloc(stat_len * sizeof(char));
> -    snprintf(stat_name, stat_len, "plugin.%s.%s.%s", PLUGIN_NAME, hostname, basename);
> -    return stat_name;
> -}
> -
> static int
> handle_read_req_hdr(TSCont cont, TSEvent event ATS_UNUSED, void *edata)
> {
>     TSHttpTxn txn = (TSHttpTxn) edata;
>     config_t *config;
> -    txn_data_t *txnd;
> +    void *txnd;
> 
>     config = (config_t *) TSContDataGet(cont);
> -    txnd = TSmalloc(sizeof(txn_data_t));
> -    txnd->remap_success = false;
> -    txnd->hostname = get_effective_host(txn);
> -    TSHttpTxnArgSet(txn, config->txn_slot, (void *) txnd);
> +    txnd = (void *) get_effective_host(txn); // low bit left 0 because we do not know
that remap succeeded yet

You should wrap this bit twiddling up in a small set of inline functions.

> +    TSHttpTxnArgSet(txn, config->txn_slot, txnd);
> 
>     TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE);
>     TSDebug(DEBUG_TAG, "Read Req Handler Finished");
> @@ -153,21 +134,16 @@ handle_post_remap(TSCont cont, TSEvent event ATS_UNUSED, void *edata)
> {
>     TSHttpTxn txn = (TSHttpTxn) edata;
>     config_t *config;
> -    txn_data_t *txnd;
> +    void *txnd = (void *) 0x01; // low bit 1 because we are post remap and thus success
> 
>     config = (config_t *) TSContDataGet(cont);
> 
>     if (config->post_remap_host)
> -    {
> -        txnd = TSmalloc(sizeof(txn_data_t));
> -        txnd->remap_success = true;
> -        txnd->hostname = NULL;
> -        TSHttpTxnArgSet(txn, config->txn_slot, (void *) txnd);
> -    }
> +        TSHttpTxnArgSet(txn, config->txn_slot, txnd);
>     else
>     {
> -        txnd = (txn_data_t *) TSHttpTxnArgGet(txn, config->txn_slot);
> -        txnd->remap_success = true;
> +        txnd = (void *) ((intptr_t)txnd | (intptr_t)TSHttpTxnArgGet(txn, config->txn_slot));
// We need the hostname pre-remap
> +        TSHttpTxnArgSet(txn, config->txn_slot, txnd);
>     }
> 
>     TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE);
> @@ -175,46 +151,50 @@ handle_post_remap(TSCont cont, TSEvent event ATS_UNUSED, void *edata)
>     return 0;
> }
> 
> +#define CREATE_STAT_NAME(s,h,b) snprintf(s, MAX_STAT_LENGTH, "plugin.%s.%s.%s", PLUGIN_NAME,
h, b);
> +
> static int
> handle_txn_close(TSCont cont, TSEvent event ATS_UNUSED, void *edata)
> {
>     TSHttpTxn txn = (TSHttpTxn) edata;
>     config_t *config;
> -    txn_data_t *txnd;
> +    void *txnd;
>     TSHttpStatus status_code = 0;
>     TSMBuffer buf;
>     TSMLoc hdr_loc;
>     uint64_t out_bytes, in_bytes;
> -    char *remap, *stat_name;
> +    char *remap, *hostname;
> +    char *unknown = "unknown";
> +    char stat_name[MAX_STAT_LENGTH];
> 
>     config = (config_t *) TSContDataGet(cont);
> -    txnd = (txn_data_t *) TSHttpTxnArgGet(txn, config->txn_slot);
> +    txnd = TSHttpTxnArgGet(txn, config->txn_slot);
> +
> +    hostname = (char *) ((intptr_t)txnd & (~((intptr_t) 0x01))); // Get hostname
> 
>     if (txnd)
>     {
> -        if (txnd->remap_success)
> +        if ((intptr_t) txnd & 0x01) // remap succeeded?
>         {
>             if (!config->post_remap_host)
> -                remap = txnd->hostname;
> +                remap = hostname;
>             else
>                 remap = get_effective_host(txn);
> 
>             if (!remap)
> -                remap = TSstrdup("unknown");
> +                remap = unknown;
> 
>             in_bytes = TSHttpTxnClientReqHdrBytesGet(txn);
>             in_bytes += TSHttpTxnClientReqBodyBytesGet(txn);
> 
> -            stat_name = create_stat_name(remap, "in_bytes");
> +            CREATE_STAT_NAME(stat_name, remap, "in_bytes")
>             stat_add(stat_name, (TSMgmtInt) in_bytes, config->persist_type, config->stat_creation_mutex);
> -            TSfree(stat_name);
> 
>             out_bytes = TSHttpTxnClientRespHdrBytesGet(txn);
>             out_bytes += TSHttpTxnClientRespBodyBytesGet(txn);
> 
> -            stat_name = create_stat_name(remap, "out_bytes");
> +            CREATE_STAT_NAME(stat_name, remap, "out_bytes")
>             stat_add(stat_name, (TSMgmtInt) out_bytes, config->persist_type, config->stat_creation_mutex);
> -            TSfree(stat_name);
> 
>             if (TSHttpTxnClientRespGet(txn, &buf, &hdr_loc) == TS_SUCCESS)
>             {
> @@ -222,33 +202,29 @@ handle_txn_close(TSCont cont, TSEvent event ATS_UNUSED, void *edata)
>                 TSHandleMLocRelease(buf, TS_NULL_MLOC, hdr_loc);
> 
>                 if ((status_code >= 200) && (status_code <= 299))
> -                    stat_name = create_stat_name(remap, "status_2xx");
> +                    CREATE_STAT_NAME(stat_name, remap, "status_2xx")
>                 else if ((status_code >= 300) && (status_code <= 399))
> -                    stat_name = create_stat_name(remap, "status_3xx");
> +                    CREATE_STAT_NAME(stat_name, remap, "status_3xx")
>                 else if ((status_code >= 400) && (status_code <= 499))
> -                    stat_name = create_stat_name(remap, "status_4xx");
> +                    CREATE_STAT_NAME(stat_name, remap, "status_4xx")
>                 else if ((status_code >= 500) && ((int)status_code <= 599))
> -                    stat_name = create_stat_name(remap, "status_5xx");
> +                    CREATE_STAT_NAME(stat_name, remap, "status_5xx")
>                 else
> -                    stat_name = create_stat_name(remap, "status_other");
> +                    CREATE_STAT_NAME(stat_name, remap, "status_other")
> 
>                 stat_add(stat_name, 1, config->persist_type, config->stat_creation_mutex);
> -                TSfree(stat_name);
>             }
>             else
>             {
> -                stat_name = create_stat_name(remap, "status_unknown");
> +                CREATE_STAT_NAME(stat_name, remap, "status_unknown")
>                 stat_add(stat_name, 1, config->persist_type, config->stat_creation_mutex);
> -                TSfree(stat_name);
>             }
> 
> -            TSfree(remap);
> -
> +            if (remap != unknown)
> +                TSfree(remap);
>         }
> -        else if (txnd->hostname)
> -            TSfree(txnd->hostname);
> -
> -        TSfree(txnd);
> +        else if (hostname)
> +            TSfree(hostname);

It is safe to pass NULL to TSfree.

>     }
> 
>     TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE);
> 


Mime
View raw message