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

Make faster experimental #1

Merged
merged 3 commits into from
Nov 22, 2016
Merged

Make faster experimental #1

merged 3 commits into from
Nov 22, 2016

Conversation

sargun
Copy link
Owner

@sargun sargun commented Nov 18, 2016

No description provided.

-spec value(orswot()) -> [member()].
-spec value(orswot() | legacy_orswot()) -> [member()].
value({_Clock, Entries, _Deferred}) when is_list(Entries) ->
[K || {K, _Dots} <- orddict:to_list(Entries)];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't Entries already a list?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entries is now a map().

Copy link

@GoelDeepak GoelDeepak Nov 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but in this case you are already checking is_list(Entries) then why orddict:to_list?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, orddict is an opaque type (even though we know it's implemented as a list).

Deffered = merge_deferred(LHSDeferred, RHSDeferred),
apply_deferred(Clock, Entries, Deffered);
merge(?EMPTY_ORSWOT, RHS) -> RHS;
merge(LHS, ?EMPTY_ORSWOT) -> LHS;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move EMPTY_ORSWOT pattern match above Deferred checks?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try to merge deferred if we can.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to do that only if it is non-empty. For Empty ones there is no merge, right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right. I'll move it.


-spec remove_elem({ok, riak_dt_vclock:vclock()} | error,
member(), {riak_dt_vclock:vclock(), orddict:orddict(), deferred()}) ->
member(), orswot()) ->
{ok, {riak_dt_vclock:vclock(), orddict:orddict(), deferred()}} |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orddict --> map in return typespec?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh? Confused?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return typespec {ok, {riak_dt_vclock:vclock(), orddict:orddict(), deferred()}} -> {ok, {riak_dt_vclock:vclock(), map(), deferred()}}.

equal({Clock1, Entries1, _}, {Clock2, Entries2, _}) ->
riak_dt_vclock:equal(Clock1, Clock2) andalso
orddict:fetch_keys(Entries1) == orddict:fetch_keys(Entries2) andalso
Entries1 == Entries2 andalso

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move Entries1 == Entries2 check as the first one?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing the vclocks is faster because that's implemented in C :).

@sargun
Copy link
Owner Author

sargun commented Nov 22, 2016

Pushed your changes.

@GoelDeepak
Copy link

lgtm

@sargun sargun merged commit 3394c86 into make-faster Nov 22, 2016
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

Successfully merging this pull request may close these issues.

2 participants