From 0cca90c21deb2517de95c83a837bf90efc6b9fa0 Mon Sep 17 00:00:00 2001 From: Vincent de Phily Date: Fri, 25 Jun 2010 17:32:11 +0200 Subject: [PATCH 1/3] Fix encoding of fixmap type. The tag value was wrong, and a missing /binary flag caused an error. --- erlang/msgpack.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erlang/msgpack.erl b/erlang/msgpack.erl index 789ed53..bbf9e64 100644 --- a/erlang/msgpack.erl +++ b/erlang/msgpack.erl @@ -168,7 +168,7 @@ unpack_array_(Bin, RestLen, RetList) when is_binary(Bin)-> pack_map(M)-> case dict:size(M) of Len when Len < 16 -> - << 2#1001:4, Len:4/integer-unit:1, (pack_map_(dict:to_list(M))) >>; + << 2#1000:4, Len:4/integer-unit:1, (pack_map_(dict:to_list(M)))/binary >>; Len when Len < 16#10000 -> % 65536 << 16#DE:8, Len:16/big-unsigned-integer-unit:1, (pack_map_(dict:to_list(M)))/binary >>; Len -> From 279121f87f322f3c1a2430751eb3a1032030853d Mon Sep 17 00:00:00 2001 From: Vincent de Phily Date: Mon, 28 Jun 2010 11:56:12 +0200 Subject: [PATCH 2/3] erlang: Use a simple proplist instead of a dict. A dict is overkill (code, cpu, memory) in most cases, and proplist<->dict conversion can easily be done by the libray user if desired. This is in line with other erlang libraries I've seen for various encoding schemes. The map encoder had a bug until I looked at it (see previous commit), so I guess it wasn't used much yet and a change is ok at this stage. The chosen representation for maps is a tuple containing the proplist as the only element. --- erlang/msgpack.erl | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/erlang/msgpack.erl b/erlang/msgpack.erl index bbf9e64..bc63677 100644 --- a/erlang/msgpack.erl +++ b/erlang/msgpack.erl @@ -30,8 +30,7 @@ % erl> S = . % erl> {S, <<>>} = msgpack:unpack( msgpack:pack(S) ). -type reason() :: enomem | badarg | no_code_matches. --type map() :: any(). % there's no 'dict' type... --type msgpack_term() :: [msgpack_term()] | integer() | float() | {dict, map()}. +-type msgpack_term() :: [msgpack_term()] | {[{msgpack_term(),msgpack_term()}]} | integer() | float(). % ===== external APIs ===== % -spec pack(Term::msgpack_term()) -> binary(). @@ -49,8 +48,10 @@ pack(Bin) when is_binary(Bin) -> pack_raw(Bin); pack(List) when is_list(List) -> pack_array(List); -pack(Map) when is_tuple(Map), element(1,Map)=:=dict -> +pack({Map}) when is_list(Map) -> pack_map(Map); +pack(Map) when is_tuple(Map), element(1,Map)=:=dict -> + pack_map(dict:from_list(Map)); pack(_O) -> {error, undefined}. @@ -166,13 +167,13 @@ unpack_array_(Bin, RestLen, RetList) when is_binary(Bin)-> % FIXME: write test for pack_map/1 pack_map(M)-> - case dict:size(M) of + case length(M) of Len when Len < 16 -> - << 2#1000:4, Len:4/integer-unit:1, (pack_map_(dict:to_list(M)))/binary >>; + << 2#1000:4, Len:4/integer-unit:1, (pack_map_(M))/binary >>; Len when Len < 16#10000 -> % 65536 - << 16#DE:8, Len:16/big-unsigned-integer-unit:1, (pack_map_(dict:to_list(M)))/binary >>; + << 16#DE:8, Len:16/big-unsigned-integer-unit:1, (pack_map_(M))/binary >>; Len -> - << 16#DF:8, Len:32/big-unsigned-integer-unit:1, (pack_map_(dict:to_list(M)))/binary >> + << 16#DF:8, Len:32/big-unsigned-integer-unit:1, (pack_map_(M))/binary >> end. pack_map_([])-> <<>>; @@ -182,15 +183,15 @@ pack_map_([{Key,Value}|Tail]) -> % FIXME: write test for unpack_map/1 -spec unpack_map_(binary(), non_neg_integer(), [{term(), msgpack_term()}])-> {more, non_neg_integer()} | { any(), binary()}. -unpack_map_(Bin, 0, Dict) when is_binary(Bin) -> {Dict, Bin}; -unpack_map_(Bin, Len, Dict) when is_binary(Bin) and is_integer(Len) -> +unpack_map_(Bin, 0, Acc) when is_binary(Bin) -> {{lists:reverse(Acc)}, Bin}; +unpack_map_(Bin, Len, Acc) when is_binary(Bin) and is_integer(Len) -> case unpack(Bin) of { more, MoreLen } -> { more, MoreLen+Len-1 }; { Key, Rest } -> case unpack(Rest) of {more, MoreLen} -> { more, MoreLen+Len-1 }; - { Value, Rest2 }-> - unpack_map_(Rest2,Len-1,dict:store(Key,Value,Dict)) + { Value, Rest2 } -> + unpack_map_(Rest2,Len-1,[{Key,Value}|Acc]) end end. @@ -295,13 +296,13 @@ unpack_(Flag, Payload)-> 16#DE when PayloadLen >= 2 -> % map 16 << Len:16/big-unsigned-integer-unit:1, Rest/binary >> = Payload, - unpack_map_(Rest, Len, dict:new()); + unpack_map_(Rest, Len, []); 16#DE -> {more, 2-PayloadLen}; 16#DF when PayloadLen >= 4 -> % map 32 << Len:32/big-unsigned-integer-unit:1, Rest/binary >> = Payload, - unpack_map_(Rest, Len, dict:new()); + unpack_map_(Rest, Len, []); % positive fixnum Code when Code >= 2#00000000, Code < 2#10000000-> @@ -325,7 +326,7 @@ unpack_(Flag, Payload)-> Code when Code >= 2#10000000 , Code < 2#10010000 -> % 1000XXXX for FixMap Len = Code rem 2#10000000, - unpack_map_(Payload, Len, dict:new()); + unpack_map_(Payload, Len, []); _Other -> {error, no_code_matches} @@ -392,12 +393,9 @@ long_test()-> map_test()-> Ints = lists:seq(0, 65), - Map = dict:from_list([ {X, X*2} || X <- Ints ] ++ [{<<"hage">>, 324}, {43542, [nil, true, false]}]), + Map = {[ {X, X*2} || X <- Ints ] ++ [{<<"hage">>, 324}, {43542, [nil, true, false]}]}, {Map2, <<>>} = msgpack:unpack(msgpack:pack(Map)), - ?assertEqual(dict:size(Map), dict:size(Map2)), - OrdMap = orddict:from_list( dict:to_list(Map) ), - OrdMap2 = orddict:from_list( dict:to_list(Map2) ), - ?assertEqual(OrdMap, OrdMap2), + ?assertEqual(Map, Map2), ok. unknown_test()-> From 537322e3b59978d26fcf096433fc718c16cc8b84 Mon Sep 17 00:00:00 2001 From: Vincent de Phily Date: Mon, 28 Jun 2010 14:17:44 +0200 Subject: [PATCH 3/3] Big speedup (around 40%) of maps and arrays encoding by using proper tail recursion. --- erlang/msgpack.erl | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/erlang/msgpack.erl b/erlang/msgpack.erl index bc63677..255542b 100644 --- a/erlang/msgpack.erl +++ b/erlang/msgpack.erl @@ -59,7 +59,7 @@ pack(_O) -> % if failed in decoding and not end, get more data % and feed more Bin into this function. % TODO: error case for imcomplete format when short for any type formats. --spec unpack( binary() )-> +-spec unpack( binary() )-> {msgpack_term(), binary()} | {more, non_neg_integer()} | {error, reason()}. unpack(Bin) when not is_binary(Bin)-> {error, badarg}; @@ -82,7 +82,7 @@ unpack_all(Data)-> % ===== internal APIs ===== % % positive fixnum -pack_uint_(N) when is_integer( N ) , N < 128 -> +pack_uint_(N) when is_integer( N ) , N < 128 -> << 2#0:1, N:7 >>; % uint 8 pack_uint_( N ) when is_integer( N ) andalso N < 256 -> @@ -145,15 +145,15 @@ pack_raw(Bin) when is_binary(Bin)-> pack_array(L) when is_list(L)-> case length(L) of Len when Len < 16 -> - << 2#1001:4, Len:4/integer-unit:1, (pack_array_(L))/binary >>; + << 2#1001:4, Len:4/integer-unit:1, (pack_array_(L, <<>>))/binary >>; Len when Len < 16#10000 -> % 65536 - << 16#DC:8, Len:16/big-unsigned-integer-unit:1,(pack_array_(L))/binary >>; + << 16#DC:8, Len:16/big-unsigned-integer-unit:1,(pack_array_(L, <<>>))/binary >>; Len -> - << 16#DD:8, Len:32/big-unsigned-integer-unit:1,(pack_array_(L))/binary >> + << 16#DD:8, Len:32/big-unsigned-integer-unit:1,(pack_array_(L, <<>>))/binary >> end. -pack_array_([])-> <<>>; -pack_array_([Head|Tail])-> - << (pack(Head))/binary, (pack_array_(Tail))/binary >>. +pack_array_([], Acc) -> Acc; +pack_array_([Head|Tail], Acc) -> + pack_array_(Tail, <>). % FIXME! this should be tail-recursive and without lists:reverse/1 unpack_array_(<<>>, 0, RetList) -> {lists:reverse(RetList), <<>>}; @@ -169,16 +169,16 @@ unpack_array_(Bin, RestLen, RetList) when is_binary(Bin)-> pack_map(M)-> case length(M) of Len when Len < 16 -> - << 2#1000:4, Len:4/integer-unit:1, (pack_map_(M))/binary >>; + << 2#1000:4, Len:4/integer-unit:1, (pack_map_(M, <<>>))/binary >>; Len when Len < 16#10000 -> % 65536 - << 16#DE:8, Len:16/big-unsigned-integer-unit:1, (pack_map_(M))/binary >>; + << 16#DE:8, Len:16/big-unsigned-integer-unit:1, (pack_map_(M, <<>>))/binary >>; Len -> - << 16#DF:8, Len:32/big-unsigned-integer-unit:1, (pack_map_(M))/binary >> + << 16#DF:8, Len:32/big-unsigned-integer-unit:1, (pack_map_(M, <<>>))/binary >> end. -pack_map_([])-> <<>>; -pack_map_([{Key,Value}|Tail]) -> - << (pack(Key))/binary,(pack(Value))/binary,(pack_map_(Tail))/binary >>. +pack_map_([], Acc) -> Acc; +pack_map_([{Key,Value}|Tail], Acc) -> + pack_map_(Tail, << Acc/binary, (pack(Key))/binary, (pack(Value))/binary>>). % FIXME: write test for unpack_map/1 -spec unpack_map_(binary(), non_neg_integer(), [{term(), msgpack_term()}])-> @@ -200,7 +200,7 @@ unpack_map_(Bin, Len, Acc) when is_binary(Bin) and is_integer(Len) -> {more, pos_integer()} | {msgpack_term(), binary()} | {error, reason()}. unpack_(Flag, Payload)-> PayloadLen = byte_size(Payload), - case Flag of + case Flag of 16#C0 -> {nil, Payload}; 16#C2 -> @@ -313,7 +313,7 @@ unpack_(Flag, Payload)-> {(Code - 16#100), Payload}; Code when Code >= 2#10100000 , Code < 2#11000000 -> -% 101XXXXX for FixRaw +% 101XXXXX for FixRaw Len = Code rem 2#10100000, << Return:Len/binary, Remain/binary >> = Payload, {Return, Remain}; @@ -344,7 +344,7 @@ compare_all([LH|LTL], [RH|RTL]) -> compare_all(LTL, RTL). test_data()-> - [true, false, nil, + [true, false, nil, 0, 1, 2, 123, 512, 1230, 678908, 16#FFFFFFFFFF, -1, -23, -512, -1230, -567898, -16#FFFFFFFFFF, 123.123, -234.4355, 1.0e-34, 1.0e64,