Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QUESTION] prepare_done(State, Action) function #389

Open
Laymer opened this issue Oct 9, 2019 · 6 comments
Open

[QUESTION] prepare_done(State, Action) function #389

Laymer opened this issue Oct 9, 2019 · 6 comments

Comments

@Laymer
Copy link

Laymer commented Oct 9, 2019

Hello 🙂

I have a quick question regarding the prepare_done/2 function. More specifically, looking at this line, I was wondering if it would not be interesting to refactor the function such that :

-define(NORMAL_COMMIT, <<1>>).

...

prepare_done(State, ?NORMAL_COMMIT, MaxPrepareTime) ->
            prepare_done(?NORMAL_COMMIT, 
                        length(State#state.updated_partitions),
                        State#state.transaction,
                        MaxPrepareTime);

prepare_done(?NORMAL_COMMIT, UpdatedPartitions, Transaction, MaxPrepareTime) ->
            ok = ?CLOCKSI_VNODE:commit(UpdatedPartitions, 
                        Transaction, 
                        MaxPrepareTime),
            {next_state, receive_committed,
                            State#state{
                                num_to_ack = UpdatedPartitions,
                                commit_time = MaxPrepareTime,
                                state = committing}}.

If I understand correctly, this could perhaps be a way to avoid passing the full state of the FSM across all helper functions and matching on multiple clauses in a single function with a case statement. I have not tested it but I was wondering if this might be a possible improvement ?

Igor

@peterzeller
Copy link
Member

Hi Igor,

You need to pass the current State to construct the new state in State#state{...}, so your code would not compile.
I also don't see the benefit of splitting this code into multiple functions, but maybe that would be clearer if you would show the whole code.

Is there a reason why would you changed the atom normal_commit to a macro/binary?

@Laymer
Copy link
Author

Laymer commented Oct 9, 2019

Hi Peter,

I believe that a macro definition might do the job using a single bit ( for example <<1>> ) in the transition and relying on the preprocessor could be a possibility to have the advantage of a clear macro without any overhead at runtime if I am not mistaken.

Regarding the state itself, my idea was that splitting the functions into their separate actions would allow the process running the FSM module to perform direct calls to matching sections, without several case clauses.

And I was wondering if passing the full state only to the top level function, would not be more memory-efficient since it would allow passing only the necessary variables to matching functions called subsequently.

My idea behind this would be to actually try to run an Antidote node on a GRiSP board, but at this time I have not yet deployed such a release so I cannot tell for sure and I am a bit unfamiliar with the gen_statem behavior, but if I have any updates regarding this topic I'll update 🙂

Thanks for your reply !

Igor

@peterzeller
Copy link
Member

I believe that a macro definition might do the job using a single bit ( for example <<1>> ) in the transition and relying on the preprocessor could be a possibility to have the advantage of a clear macro without any overhead at runtime if I am not mistaken.

I think you cannot get smaller than an atom in Erlang. According to http://erlang.org/doc/efficiency_guide/advanced.html an Atom takes up 1 word, whereas a binary takes at least 3 words.

And I was wondering if passing the full state only to the top level function, would not be more memory-efficient since it would allow passing only the necessary variables to matching functions called subsequently.

The state is not copied when it is passed as a parameter. Terms only have to be copied when they are sent to other processes.

@Laymer
Copy link
Author

Laymer commented Oct 10, 2019

In fact you are completely right. After doing a bit of RTFM about binary constructions, I might go for a different approach but I am not yet 100%. I will probably do some tests using binary generators to construct the full state as in :

<<<<Type:1, State:2, Time:8, ....>> || Type:1, State:2, Time:8, ... <<- Binary>>

And I will see if anything interesting is noticeable 🙂 Thanks !

@peterzeller
Copy link
Member

If you are looking for opportunities to save memory, there are some other things I would look at before going into micro optimizations like using binaries. For example:

  • The pool of processes controlled by READ_CONCURRENCY could be changed to only spawn processes on demand (same for INTER_DC_QUERY_CONCURRENCY).
  • The metadata used in CRDTs could be reduced.
  • The log/cache could be improved to store items on disk (Saalik and Kevin are currently working on this)

It's probably a good idea to profile memory usage before doing any optimizations.

@Laymer
Copy link
Author

Laymer commented Oct 11, 2019

Thank you very much for your insights ! 🙂 I will se if I can eventually contribute with something useful !

Best,

Igor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants