[erlang-questions] Slow when using chained gen_server:call's, redesign or optimize?

[God Dang]

I'm creating a system where I've ended up with alot of gen_servers that provides a clean interface. When I run this under load I see that the gen_server:call's is becoming a bottleneck.

For instance, In a handle_request I might ask an other gen_server to get me a cached object, then ask the database something, then etc...

and in some cases I have my-gen_server->cache-gen_server->memcache-client-gen_server as you see it stacks up to alot of steps. I've tried to optimize with deferring gen_server responses and that has given a slight performance improvement but not as drastical as if I for instance bypass one gen_server instance.

Is there a better way to go about this or some smart optimization to do? And FYI, I use gen_server when I need to keep a state of a connection or something so if the answer is to scrap  or reduce the number of gen_servers I will need to keep those connections somewhere else.

Thanks, Dang

[Gianfranco Alongi]

Could you put common memory-related operations into one single gen_server?
Does this need to be gen_server at all?

You could use the synchronized serialization to generate push-back
behaviour from your system,
so that you do not handle a new request before it's possible - maybe
you are already doing this,
or not.

If you really want to find the bottlenecks, you could try with fprof
http://www.erlang.org/doc/man/fprof.html

/G

> _______________________________________________
> erlang-questions mailing list
> erlang-q...@erlang.org
> http://erlang.org/mailman/listinfo/erlang-questions
>
_______________________________________________
erlang-questions mailing list
erlang-q...@erlang.org
http://erlang.org/mailman/listinfo/erlang-questions

[Max Bourinov]

Instead of memcache you can use ETS which is "built in" and way faster.

Sent from my iPhone

[Matthew Evans]

Of course you need to  run a profiler such as fprof to see what's going on.

Sounds like a classic head of line blocking problem. Many requests, possibly from processes on different schedulers/cores, all getting serialized on a single gen_server.

The obvious, and maybe non-OTP, answer is to hold some of this state information in a public or protected named ETS table that your clients read from directly. A single gen_server can still own and write to that ETS table. 

Another obvious answer is to provide back-pressure of some kind to prevent clients from requesting data when it is under load.

You might find that a particular infrequent  gen_server:call operation is taking a long time to complete causing a message queue to suddenly grow. You might want to change such an operation from:

handle_call({long_operation,Data},From,State) ->

    Rsp = do_lengthy_operation(Data),

    {reply, Rsp, State};

to:

handle_call({long_operation,Data},From,State) ->

    spawn(fun() ->

            Rsp = do_lengthy_operation(Data),

            gen_server:reply(Rsp,From)

     end),   

    {noreply, State};

---

From: god...@hotmail.com
To: erlang-q...@erlang.org
Date: Sat, 28 Jan 2012 00:06:04 +0000
Subject: [erlang-questions] Slow when using chained gen_server:call's, redesign or optimize?

[Jesper Louis Andersen]

On 1/28/12 4:18 PM, Matthew Evans wrote:

The obvious, and maybe non-OTP, answer is to hold some of this state information in a public or protected named ETS table that your clients read from directly. A single gen_server can still own and write to that ETS table. 

This would be my first idea. Create an ETS table being protected. Writes to the table goes through the gen_server,

-export([write/1, read/1]).

write(Obj) ->
  call({write, Obj}).

call(M) ->
  gen_server:call(?SERVER, M, infinity).

but reads happen in the calling process of the API and does not go through the gen_server at all,

read(Key) ->
  case ets:lookup(?TAB, Key) of
    [] -> not_found;
    [_|_] = Objects -> {ok, Objects}
  end.

Creating the table with {read_concurrency, true} as the option will probably speed up reads by quite a bit as well. It is probably going to be a lot faster than having all caching reads going through that single point of contention. Chances are that just breaking parts of the chain is enough to improve the performance of the system.

-- 
Jesper Louis Andersen
  Erlang Solutions Ltd., Copenhagen, DK

[Jachym Holecek]

Hi,

[Replying to multiple replies at once, all quoted text reformatted for
readability (seems people these days can't be bothered!?).]

[Warning: excessively nitpicky content ahead, here and there.]

# God Dang 2012-01-28:

> I'm creating a system where I've ended up with alot of gen_servers that
> provides a clean interface. When I run this under load I see that the
> gen_server:call's is becoming a bottleneck.

You're probably treating asynchronous things as synchronous somewhere along
the path, inflicting collateral damage to concurrent users? Synchronous
high-level API is fine, but if you know some operation is expensive or
depends on response from the outside world you should record request
context in ETS (row looking something like {Req_id, Timeout, From[, ...]}
and process requests out-of-order OR offload the actual processing to
short lived worker processes like Matthew Evans says OR a combination
of both OR somesuch.

My point being that gen_server:call/N, by itself, is *very* fast in
practice, so chances are you're doing something wrong elsewhere.

Other (unlikely) thing: you're not sending very large data structures in
messages, are you? That could hurt, but there are ways to address that
too if needed.

# Matthew Evans 2012-01-28:

> Another obvious answer is to provide back-pressure of some kind to prevent
> clients from requesting data when it is under load.

On external interfaces (or for global resource usage of some sort): yes, a
fine idea (a clear "must have", actually!); but doing this internally would
seem excessively defensive to me, unless further justification was given.

> You might want to change such an operation from:
>
> handle_call({long_operation,Data},From,State) ->
> Rsp = do_lengthy_operation(Data),
> {reply, Rsp, State};
>
> to:
>
> handle_call({long_operation,Data},From,State) ->
> spawn(fun() -> Rsp = do_lengthy_operation(Data), gen_server:reply(Rsp,From) end),
> {noreply, State};

1. Why do people bother introducing "one-shot" variables for trivial
expressions they could have inlined? Means less context to maintain
when reading the code...

2. Surely you meant proc_lib:spawn_link/X there, didn't you? SASL logs
and fault propagation are the reason. While there are exceptions to
this, they're extremely rare.

3. The order of arguments to gen_server:reply/2 is wrong.

Regarding the general approach: yes, a fine idea too. Depending on what
do_lengthy_operation/1 does putting these workers under supervisor might
be called for.

# Jesper Louis Andersen 2012-01-28:

> This would be my first idea. Create an ETS table being protected.
> Writes to the table goes through the gen_server,

Yes, a fine idea too -- ETS is one of the less obvious cornerstones
of Erlang programming (but don't tell "purity" fascists )... One
detail: almost all of my ETS tables are public even when many of
them are really treated as private or protected, reason is to keep
high degree of runtime tweakability just in case (this might be a
bit superstitious I admit).

> -export([write/1, read/1]).
>
> write(Obj) ->
> call({write, Obj}).
>
> call(M) ->
> gen_server:call(?SERVER, M, infinity).

1. Abstracting trivial functionality such as call/1 above only
obfuscates code for precisely zero gain.

2. Same goes for typing "?SERVER" instead of the actual server
name. Using "?MODULE" is however alright, as long as it's
only referred to from current module (as it should).

3. No infinite timeouts without very good justification! You're
sacrificing a good default protective measure for no good
reason...

> but reads happen in the calling process of the API and does not go
> through the gen_server at all,
>
> read(Key) ->
> case ets:lookup(?TAB, Key) of
> [] -> not_found;
> [_|_] = Objects -> {ok, Objects}
> end.

(2) from above also applies to "?TAB" here. More to the point, it's
sometimes perfectly OK to do table writes directly from caller's
context too, like:

write(Item) ->
true = ets:insert_new(actual_table_name, Item).

It can of course be very tricky business and needs good thinking first.
I bet you're aware of this, mentioning it just because it's a handy
trick that doesn't seem to be widely known.

> Creating the table with {read_concurrency, true} as the option will
> probably speed up reads by quite a bit as well. It is probably going
> to be a lot faster than having all caching reads going through that
> single point of contention. Chances are that just breaking parts of
> the chain is enough to improve the performance of the system.

Well, yes, avoiding central points of contention (such as blessed
processes or, you guessed it, ETS tables) is certainly good engineering
practice, but see first part of this email for other considerations.

BR,
-- Jachym

[God Dang]

Thank you all for taking time to help me out. Some clarification, I've come to the conclusion that gen_server(or more precisely my design) is to blame after running multiple sessions with fprof, eprof and alot of timer:now_diff and timer:tc. For example, if I put load on the system and time my handle_call({long_operation,Data},From,State) I see that the execution time begins rise from <10ms to around ~2000ms and stabilizing there under my load. If I remove the last gen_server and instead implement it as a simple function it stabilizes around ~1000ms and removing yet another one gives me yet lower execution times.

A word on my design, a lot of the time the only reason we use a gen_server is for keeping a state and more precisely a connection. For instance a generic db backend which keeps the pid of the db connection, a cache backend which keeps the memcache connection pid, my own server that keep state. and within my handle_call({long_operation,Data},From,State) I could be doing a couple of gen_server:call's and within the database gen_server I could be doing additional gen_server call's to the cache backend. Why we've constructed it as separate gen_server's is also to be able to call it from different parts of the system like db:do_something() or cache:get().

On paper this looked like a clean and nice design but under load it starts to behave poorly.

To answer all in order:

# Gianfranco Alongi 2012-01-28:

> You could use the synchronized serialization to generate push-back
> behaviour from your system,
> so that you do not handle a new request before it's possible - maybe
> you are already doing this, or not.

I don' understand what this mean. Can you please clarify.

# Matthew Evans 2012-01-28

> The obvious, and maybe non-OTP, answer is to hold some of this state information in a public or protected named ETS table

> that your clients read from directly. A single gen_server can still own and write to that ETS table.

Sounds like a smart approach.

> Another obvious answer is to provide back-pressure of some kind to prevent clients from requesting data when it is under load.

I don't understand this fully.

> You might find that a particular infrequent  gen_server:call operation is taking a long time to complete causing a message queue

> to suddenly grow. You might want to change such an operation from:

I've done that on the first most handle_call. That's what I meant with deferring gen_server responses. I saw some speedups on the first handle_call but doing this on short lived handle_call's did see slowdowns.

# Jesper Louis Andersen 2012-01-28

> This would be my first idea. Create an ETS table being protected. Writes to the table goes through the gen_server,

I like this approach, not as "beautiful" as a pure OTP-approach, but if does the trick. Hey.

# Jachym Holecek 2012-01-29

> You're probably treating asynchronous things as synchronous somewhere along the path, inflicting collateral damage to concurrent users?

it's basically a get_data() function where we need to do roundtrips to db, other stuff to be able to return the value. Can I do this in a asynchronous fashion? I tend to think of get's as handle_calls and sets as hanlde_cast.

Big thank you all.

/dang

> Date: Sat, 28 Jan 2012 18:13:25 -0500
> From: fr...@circlewave.net
> To: jesper.lou...@erlang-solutions.com
> CC: erlang-q...@erlang.org
> Subject: Re: [erlang-questions] Slow when using chained gen_server:call's, redesign or optimize?

[Gianfranco Alongi]

Hi,

What i meant was that you could maybe change your current design (if
it is not like this already) so that you
do not start to process another request from an external interface
until your system has the actual resources
to do so.

[Jesper Louis Andersen]

On 1/29/12 12:13 AM, Jachym Holecek wrote:
>> -export([write/1, read/1]).
>>
>> write(Obj) ->
>> call({write, Obj}).
>>
>> call(M) ->
>> gen_server:call(?SERVER, M, infinity).
>

> 3. No infinite timeouts without very good justification! You're
> sacrificing a good default protective measure for no good
> reason...
>

The question here is really one of what you want to do if a write takes
more than the default timeout of 5 seconds. You can either decide to
tell the writer that the cache is clogged up at the moment and the
abstain from caching the result, you can crash, or you can wait around
like the above code does. What the right thing to do in a situation
depends a bit on what semantics you want.

Perhaps my reaction for a cache was to knee-jerky. You might want a call
to fail if it takes too long to process because it will uncover another
problem in the code: namely overload of the cache.

--
Jesper Louis Andersen
Erlang Solutions Ltd., Copenhagen, DK

_______________________________________________

[Jachym Holecek]

# Jesper Louis Andersen 2012-01-29:

> On 1/29/12 12:13 AM, Jachym Holecek wrote:
> >>-export([write/1, read/1]).
> >>
> >>write(Obj) ->
> >> call({write, Obj}).
> >>
> >>call(M) ->
> >> gen_server:call(?SERVER, M, infinity).
> >
> > 3. No infinite timeouts without very good justification! You're
> > sacrificing a good default protective measure for no good
> > reason...
> >
> The question here is really one of what you want to do if a write
> takes more than the default timeout of 5 seconds. You can either
> decide to tell the writer that the cache is clogged up at the moment
> and the abstain from caching the result, you can crash, or you can
> wait around like the above code does. What the right thing to do in
> a situation depends a bit on what semantics you want.

True.

> Perhaps my reaction for a cache was to knee-jerky. You might want a
> call to fail if it takes too long to process because it will uncover
> another problem in the code: namely overload of the cache.

And it will also release the resources held by waiting process, that's
my main concern. What came to mind immediately when I saw the infinite
timeout was a typical (IMO) scenario when one might be tempted to use
those:

%% This could be a load-balancer or failover manager process that
%% acts as entry point to a protocol stack, or somesuch thing.

send_req(Pid, Req, Timeout) ->
gen_server:call(Pid, {send_req, Req, Timeout}, infinity).

handle_call({send_req, Req, Timeout}, From, #state{parties = Ps} = State) ->
party:send_req(choose_party(Ps), {send_req, Req, From, Timeout}),
{noreply, State};

Where party module would perhaps do some more delegation of its own and
eventually request ends up sent out to an external system, timeout gets
planned and its reference recorded along with From in and ETS table.
When either timeout triggers or response arrives it gets correlated
against ETS and gen_server:reply/2 is called.

Now if something goes wrong and that ETS table evaporates or a low-level
process explodes and the error, by mistake, isn't propagated correctly
(which would involve faulting all pending requests immediately), one is
left with the client process sitting there forever. Sure, gen_server:call/X
isn't stupid and monitors the server process -- but given the amount of
delegation we have going on, that one may very well still be alive and
doing well. Now over time these zombie processes could add u, and whole
node crashes.

This is a somewhat elaborate scenario and depends on a suitable bug being
already present somewhere in the system, or perhaps just some unfortunate
timing in otherwise reasonably designed system. But let's consider trivial
change, everything else being the same:

send_req(Pid, Req, Timeout) ->
gen_server:call(Pid, {send_req, Req, scale_down(Timeout)}, Timeout).

scale_down(N) ->
%% This could involve both low and high internal processing overhead
%% allowance cutoff if one wanted to be super-correct about this.
round(N * 0.90).

Bugs or not and timing or not we now have a hard deadline on resource
release and can sleep a bit more peacefully at night without nightmares
of zombie apocalypse. :-) Furthermore, this valuable behavioral contract
is immediately apparent during code inspection, making things easier to
reason about.

Hopefully this sort of context clarifies my (possibly overly terse)
response.

BR,
-- Jachym

[tom kelly]

Hi List,

I'm a bit late replying to this thread, only reading it now. Probably too late to help the original poster but here's my contribution anyway.

Matthew, hope you don't mind me suggesting a minor improvement to your code snippet, just to demonstrate a larger point. Instead of:

handle_call({long_operation,Data},From,State) ->
    spawn(fun() ->
            Rsp = do_lengthy_operation(Data),
            gen_server:reply(Rsp,From)
     end),
    {noreply, State};

I prefer getting the calling thread to do as much work as possible.
Here a process is spawned to do the work and then die, all while the calling process waits. I think it's preferable to get the calling process to do the work itself, it avoids the spawn and any crashes in the do_lengthy_operation are argubly easier to debug. Eg:

long_operation(Data) ->
    State = gen_server:call(?MODULE, get_state),
    do_lengthy_operation(Data,State).

handle_call(get_state,_From,State) ->
    {reply,State,State};

Normally when I'm writing a new gen_server I get the calling process to do as much work as possible and try to get the handle_calls/casts to only do the work that requires mutual exclusion. I normally store what I can in a protected, named_table ets table so that interface funtions that need this can read it without going through the gen_server but I do all updates in a handle_call/cast.

Once one of my gen_servers became a bottle-neck in a system and I ended up taking these steps to fix the problem. Since then I've adopted this as a rule of thumb when writing new gen_servers and avoid such pointless bottle-necks that I'd have to fix later on anyway.

I inherited some code with the same problem as the original poster with gen_server calls going five-deep in one instance. Everything worked fine at low load levels but once the load crossed a threshold there was no recovery as every management process spent most of its time waiting for other management processes to reply while their own message queues built up. Meanwhile the worker threads would timeout, reconnect and add to the backlog. The message queues just built up until the entire node fell over.

After making sure that the chained calls weren't required to keep the data consistent I moved as work as possible to the calling process. This way I kept the interface unchanged for each module but avoided the case where each process in the chain was tied up, waiting for the last one to complete.

I think for newbies it's important to be aware of which process is doing the work. I know when I started using Erlang it took a while to get my head around the concurrency.

//TTom.