This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/couchdb.git
The following commit(s) were added to refs/heads/main by this push:
new ba54c635b Optimize mem3:dbname/1 function
ba54c635b is described below
commit ba54c635ba3b5a9e4650245f5b4b8df47f39d78c
Author: Nick Vatamaniuc <[email protected]>
AuthorDate: Tue May 16 18:42:30 2023 -0400
Optimize mem3:dbname/1 function
`mem3:dbname/1` with a `<<"shard/...">>` binary is called quite a few times
as
seen when profiling with fprof:
https://gist.github.com/nickva/38760462c1545bf55d98f4898ae1983d
In that case `mem3:dbname` is removing the timestamp suffix. However,
because
it uses `filename:rootname/1` which handles cases pertaining to file system
paths and such, it ends up being a bit more expensive than necessary.
To optimize it assume it has a timestamp suffix and try to parse it out
first,
and then verify can be parsed into an integer, if that fails fall back to
using
`filename:rootname/1`.
To lower chance of the timestamp suffix changing and us not noticing move
the
shard suffix generation function from fabric to mem3 so the generating and
the
stripping functions are right next to each other.
A quick speed comparison test shows a 6x speedup or so:
```
shard_speed_test() ->
Shard =
<<"shards/80000000-9fffffff/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.1234567890">>,
shard_speed_check(Shard, 10000).
shard_speed_check(Shard, N) ->
T0 = erlang:monotonic_time(),
do_dbname(Shard, N),
Dt = erlang:monotonic_time() - T0,
DtUsec = erlang:convert_time_unit(Dt, native, microsecond),
DtUsec / N.
do_dbname(_, 0) ->
ok;
do_dbname(Shard, N) ->
_ = dbname(Shard),
do_dbname(Shard, N - 1).
```
On main:
```
([email protected])1> mem3:shard_speed_test().
1.3099
```
With PR:
```
([email protected])1> mem3:shard_speed_test().
0.1959
```
---
src/fabric/src/fabric_db_create.erl | 3 +--
src/mem3/src/mem3.erl | 53 ++++++++++++++++++++++++++++++++++++-
2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/src/fabric/src/fabric_db_create.erl
b/src/fabric/src/fabric_db_create.erl
index 38770aea4..61bc13cc4 100644
--- a/src/fabric/src/fabric_db_create.erl
+++ b/src/fabric/src/fabric_db_create.erl
@@ -62,8 +62,7 @@ validate_dbname(DbName, Options) ->
end.
generate_shard_map(DbName, Options) ->
- {MegaSecs, Secs, _} = os:timestamp(),
- Suffix = "." ++ integer_to_list(MegaSecs * 1000000 + Secs),
+ Suffix = mem3:generate_shard_suffix(),
Shards = mem3:choose_shards(DbName, [{shard_suffix, Suffix} | Options]),
case mem3_util:open_db_doc(DbName) of
{ok, Doc} ->
diff --git a/src/mem3/src/mem3.erl b/src/mem3/src/mem3.erl
index 7b4a8eca2..a57cad2db 100644
--- a/src/mem3/src/mem3.erl
+++ b/src/mem3/src/mem3.erl
@@ -34,6 +34,7 @@
-export([ping/1, ping/2]).
-export([db_is_current/1]).
-export([shard_creation_time/1]).
+-export([generate_shard_suffix/0]).
%% For mem3 use only.
-export([name/1, node/1, range/1, engine/1]).
@@ -295,7 +296,7 @@ decode_placement_string(PlacementStr) ->
dbname(#shard{dbname = DbName}) ->
DbName;
dbname(<<"shards/", _:8/binary, "-", _:8/binary, "/", DbName/binary>>) ->
- list_to_binary(filename:rootname(binary_to_list(DbName)));
+ strip_shard_suffix(DbName);
dbname(DbName) when is_list(DbName) ->
dbname(list_to_binary(DbName));
dbname(DbName) when is_binary(DbName) ->
@@ -462,6 +463,26 @@ db_is_current(Name) when is_binary(Name) ->
% for unit tests that either test or use mem3_rep logic
couch_server:exists(Name).
+generate_shard_suffix() ->
+ UnixSeconds = os:system_time(second),
+ "." ++ integer_to_list(UnixSeconds).
+
+strip_shard_suffix(DbName) when is_binary(DbName) ->
+ % length(".1684269710") = 11. On 2286-11-20 the timestamp would flip to 11
+ % digits so we'd have to increase length to 12 then.
+ case DbName of
+ <<Prefix:(byte_size(DbName) - 11)/binary, $., Ts/binary>> ->
+ try
+ _ = binary_to_integer(Ts),
+ Prefix
+ catch
+ error:badarg ->
+ filename:rootname(DbName)
+ end;
+ _ ->
+ filename:rootname(DbName)
+ end.
+
-ifdef(TEST).
-include_lib("eunit/include/eunit.hrl").
@@ -505,4 +526,34 @@ rotate_rand_distribution_test() ->
Cases = [rotate_rand([1, 2, 3]) || _ <- lists:seq(1, 100)],
?assertEqual(3, length(lists:usort(Cases))).
+strip_shard_suffix_test_() ->
+ Prefix = <<"shards/c0000000-ffffffff/">>,
+ [
+ {DbName, ?_assertEqual(Res, dbname(<<Prefix/binary, DbName/binary>>))}
+ || {Res, DbName} <- [
+ {<<"foo">>, <<"foo.1684269710">>},
+ {<<"foo">>, <<"foo.168426971z">>},
+ {<<"foo/bar">>, <<"foo/bar.1684269710">>},
+ {<<"foo">>, <<"foo.1">>},
+ {<<"foo">>, <<"foo.abc">>},
+ {<<"foo">>, <<"foo.1111111111111111">>},
+ {<<"">>, <<"">>},
+ {<<"/">>, <<"/">>},
+ {<<"//">>, <<"//">>},
+ {<<"/.foo">>, <<"/.foo">>},
+ {<<".">>, <<"..">>},
+ {<<".">>, <<"..foo">>}
+ ]
+ ].
+
+shard_suffix_test() ->
+ % Assert a few basic things about the db suffixes we generate. If we change
+ % this scheme make sure to update strip_shard_suffix/1 and other places
+ % which assume the suffix is a 10 digit unix timestamp.
+ Suffix = generate_shard_suffix(),
+ ?assertEqual($., hd(Suffix)),
+ ?assertEqual(11, length(Suffix)),
+ [$. | Timestamp] = Suffix,
+ ?assert(is_integer(list_to_integer(Timestamp))).
+
-endif.
