Thanks Benôit.
However I don't see any gain with that. Doing a case to detect the
'$end_of_table' is not necessary. It's expected to find
State#state.cache_size - NewSize elements in the cache. If not, then
there's a bug and it's better to fail fast, so that it's detected and
fixed sooner (a general OTP practice).
I also don't see the need for that new function, as it basically
duplicates what the existing function free_mru_cache_entry/0 does:
free_mru_cache_entry() ->
case ets:last(?BY_ATIME) of
'$end_of_table' ->
ok; % empty cache
LruTime ->
[{MruTime, UserName}] = ets:lookup(?BY_ATIME, MruTime),
true = ets:delete(?BY_ATIME, MruTime),
true = ets:delete(?BY_USER, UserName)
end.
On Thu, Jan 27, 2011 at 11:09 AM, Benoit Chesneau <[email protected]> wrote:
> I think we could remove the uses of lists call and just use pattern
> matching here. I attached a patch that is doing that. Also I added a
> guard in case there is no member items in the cache.
>
> - benoît
>
> ---
> src/couchdb/couch_auth_cache.erl | 25 +++++++++++++++----------
> 1 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/src/couchdb/couch_auth_cache.erl
> b/src/couchdb/couch_auth_cache.erl
> index 033e160..9bd3450 100644
> --- a/src/couchdb/couch_auth_cache.erl
> +++ b/src/couchdb/couch_auth_cache.erl
> @@ -169,15 +169,7 @@ handle_call({new_max_cache_size, NewSize},
> {reply, ok, State#state{max_cache_size = NewSize}};
>
> handle_call({new_max_cache_size, NewSize}, _From, State) ->
> - lists:foreach(
> - fun(_) ->
> - LruTime = ets:last(?BY_ATIME),
> - [{LruTime, UserName}] = ets:lookup(?BY_ATIME, LruTime),
> - true = ets:delete(?BY_ATIME, LruTime),
> - true = ets:delete(?BY_USER, UserName)
> - end,
> - lists:seq(1, State#state.cache_size - NewSize)
> - ),
> + free_mru_cache_entries(State#state.cache_size - NewSize),
> {reply, ok, State#state{max_cache_size = NewSize, cache_size = NewSize}};
>
> handle_call({fetch, UserName}, _From, State) ->
> @@ -223,7 +215,7 @@ terminate(_Reason, #state{db_notifier = Notifier}) ->
>
> code_change(_OldVsn, State, _Extra) ->
> {ok, State}.
> -
> +
>
> clear_cache(State) ->
> exec_if_auth_db(fun(AuthDb) -> catch couch_db:close(AuthDb) end),
> @@ -243,6 +235,19 @@ add_cache_entry(UserName, Credentials, ATime, State) ->
> true = ets:insert(?BY_USER, {UserName, {Credentials, ATime}}),
> State#state{cache_size = couch_util:get_value(size, ets:info(?BY_USER))}.
>
> +free_mru_cache_entries(1) ->
> + ok;
> +free_mru_cache_entries(Count) ->
> + case ets:last(?BY_ATIME) of
> + '$end_of_table' ->
> + ok;
> + LruTime ->
> + [{LruTime, UserName}] = ets:lookup(?BY_ATIME, LruTime),
> + true = ets:delete(?BY_ATIME, LruTime),
> + true = ets:delete(?BY_USER, UserName),
> + free_mru_cache_entries(Count-1)
> + end.
> +
>
> free_mru_cache_entry() ->
> case ets:last(?BY_ATIME) of
> --
> 1.7.3.4
>
>
> On Thu, Jan 27, 2011 at 11:46 AM, <[email protected]> wrote:
>> Author: fdmanana
>> Date: Thu Jan 27 10:46:42 2011
>> New Revision: 1064076
>>
>> URL: http://svn.apache.org/viewvc?rev=1064076&view=rev
>> Log:
>> Trivial small refactoring
>>
>> Shortening the code by using multiple function clauses.
>>
>> Modified:
>> couchdb/trunk/src/couchdb/couch_auth_cache.erl
>>
>> Modified: couchdb/trunk/src/couchdb/couch_auth_cache.erl
>> URL:
>> http://svn.apache.org/viewvc/couchdb/trunk/src/couchdb/couch_auth_cache.erl?rev=1064076&r1=1064075&r2=1064076&view=diff
>> ==============================================================================
>> --- couchdb/trunk/src/couchdb/couch_auth_cache.erl (original)
>> +++ couchdb/trunk/src/couchdb/couch_auth_cache.erl Thu Jan 27 10:46:42 2011
>> @@ -110,11 +110,8 @@ init(_) ->
>> ok = couch_config:register(
>> fun("couch_httpd_auth", "auth_cache_size", SizeList) ->
>> Size = list_to_integer(SizeList),
>> - ok = gen_server:call(?MODULE, {new_max_cache_size, Size},
>> infinity)
>> - end
>> - ),
>> - ok = couch_config:register(
>> - fun("couch_httpd_auth", "authentication_db", DbName) ->
>> + ok = gen_server:call(?MODULE, {new_max_cache_size, Size},
>> infinity);
>> + ("couch_httpd_auth", "authentication_db", DbName) ->
>> ok = gen_server:call(?MODULE, {new_auth_db, ?l2b(DbName)},
>> infinity)
>> end
>> ),
>> @@ -167,26 +164,21 @@ handle_call(auth_db_compacted, _From, St
>> ),
>> {reply, ok, State};
>>
>> +handle_call({new_max_cache_size, NewSize},
>> + _From, #state{cache_size = Size} = State) when NewSize >= Size ->
>> + {reply, ok, State#state{max_cache_size = NewSize}};
>> +
>> handle_call({new_max_cache_size, NewSize}, _From, State) ->
>> - case NewSize >= State#state.cache_size of
>> - true ->
>> - ok;
>> - false ->
>> - lists:foreach(
>> - fun(_) ->
>> - LruTime = ets:last(?BY_ATIME),
>> - [{LruTime, UserName}] = ets:lookup(?BY_ATIME, LruTime),
>> - true = ets:delete(?BY_ATIME, LruTime),
>> - true = ets:delete(?BY_USER, UserName)
>> - end,
>> - lists:seq(1, State#state.cache_size - NewSize)
>> - )
>> - end,
>> - NewState = State#state{
>> - max_cache_size = NewSize,
>> - cache_size = lists:min([NewSize, State#state.cache_size])
>> - },
>> - {reply, ok, NewState};
>> + lists:foreach(
>> + fun(_) ->
>> + LruTime = ets:last(?BY_ATIME),
>> + [{LruTime, UserName}] = ets:lookup(?BY_ATIME, LruTime),
>> + true = ets:delete(?BY_ATIME, LruTime),
>> + true = ets:delete(?BY_USER, UserName)
>> + end,
>> + lists:seq(1, State#state.cache_size - NewSize)
>> + ),
>> + {reply, ok, State#state{max_cache_size = NewSize, cache_size =
>> NewSize}};
>>
>> handle_call({fetch, UserName}, _From, State) ->
>> {Credentials, NewState} = case ets:lookup(?BY_USER, UserName) of
>>
>>
>>
>
--
Filipe David Manana,
[email protected], [email protected]
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
