Reduce atom leak #42
Reduce atom leak #42
Conversation
| @@ -56,8 +56,14 @@ next_context_var(CState, #ws{nr=Nr} = Ws) -> | |||
|
|
|||
| %% @doc Convert a list or binary to an atom. | |||
| -spec to_atom(string()|binary()|atom()) -> atom(). | |||
| to_atom(L) when is_list(L) -> erlang:list_to_atom(L); | |||
| to_atom(B) when is_binary(B) -> erlang:binary_to_atom(B, 'utf8'); | |||
| to_atom(L) when is_list(L) -> case catch erlang:list_to_existing_atom(L) of | |||
There was a problem hiding this comment.
It no need to add this logic here by reason that if atom already exists in the system, it will not create or re create new one, simple check:
1> erlang:memory(atom_used).
256891
2> list_to_atom("zotonic").
zotonic
3> erlang:memory(atom_used).
258010
4> list_to_atom("zotonic").
zotonic
5> erlang:memory(atom_used).
258010
6> list_to_existing_atom("zotonic").
zotonic
7> erlang:memory(atom_used).
258010As you can see, the size of atoms is not growing.
There was a problem hiding this comment.
Thanks, @vkatsuba! Always very helpful and informative.
I saw this pattern somewhere, and it stuck in my head. Your simple test proves that I'm thinking wrong about *_to_existing_atom.
There was a problem hiding this comment.
@williamthome always welcome! You can also read about some best practice at https://erlef.github.io/security-wg/secure_coding_and_deployment_hardening. You are thinking about atom leak correct here and this is can be issue. But implementation can take some time and deep discussion with @zotonic/developers I suppose.
There was a problem hiding this comment.
Thanks for the informative link.
Yes, atom leak can be a problem, but I was wrong in thinking that list_to_atom or binary_to_atom always increases memory usage.
|
So, the @vkatsuba review shows me that this PR does not make much (or no) sense. |
Not sure that this PR need to close, but I suppose it can be adapted a little bit. |
|
@williamthome actually it looks like here is already provided some issue #4 with short possible solution approach. Please, take a look. |
|
Maybe we can open a separate issue to discuss this. On a related note I saw that we have a confusion about template variables being a binary or an atom. This can result in two keys (one binary and one atom) of the same variable. I think it is best (due to atom leaks) to have variables as binaries. |
|
@mworrell, what do you think about this PR? Should I adapt it and change all keys to binary? |
|
Those atoms/binaries are more the keys for variables like in the I think the more correct was is here to map those to binaries. That will mean that we will also need to make some changes on the Zotonic side, as scomps (etc) are probably using the atom keys. |
|
Oh, this change can break things and causes a huge refactor. |
|
Yes, we can also postpone this change till the 1.1 or 2.0 |
I've noticed that convert
to_atomis used often.This PR can help to reduce the atom leak by first trying to convert to an existing atom.