Add multiset support for IBLT #66

arorashu · 2020-07-15T16:41:14Z

Added multiset implementation for IBLT, that uses modular(residual) sums.
Created a new Sync IBLTSync_Multiset that derives from IBLTSync, similar to the existing IBLTSync_HalfRound.

Tests done

All existing test cases pass.
Added new test case for multiset IBLT

This addresses Issue#63 and Issue#64.

1. load client & server data from file 2. run client process, server process 3. print stats about sync

- Use `IBLTSync_Multiset` in place of `IBLTSync` in tests where multiset is used - Default IBLT uses isMultiset=false

because defaulted default constructor defined outside the class is treated as user defined, and is initialized with garbage values

- create constructor to initialize the function pointers

arorashu · 2020-07-20T20:26:21Z

Function pointers were the easiest way to get the same performance as inheritance, but with minimal code changes, i.e. we don't have to change IBLT usage in the code anywhere. However, I am working on a different implementation that does this using inheritance/templates now.

trachten · 2020-07-20T20:34:43Z

thanks! Function pointers are messy because they can easily get corrupted ...

…

On Jul 20, 2020, at 4:26 PM, 7/20/20, Shubham Arora ***@***.***> wrote: Function pointers were the easiest way to get the same performance as inheritance, but with minimal code changes, i.e. we don't have to change IBLT usage in the code anywhere. However, I am working on a different implementation that does this using inheritance/templates now. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACM5LUZ7EJWEV5INQA6W7RTR4SR7ZANCNFSM4O2WHHIA>.

--- Ari Trachtenberg, Boston University http://people.bu.edu/trachten mailto:trachten@bu.edu ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Random quote of the day: "For me, it is important to remember, but it is far more important to remember to act." - Simon Wiesenthal ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

- create communication functions for IBLTMultiset

arorashu · 2020-07-21T02:19:53Z

Updated with class based implementation

novakboskov · 2020-07-21T16:07:10Z

Updated with class based implementation

Great! I'm going to review this today.

novakboskov

In the interest of time, I am going to address the minor issues myself, merge it, and then make an issue to require the refactoring of IBLTMultiset.

novakboskov · 2020-07-21T16:31:17Z

include/CPISync/Syncs/IBLTMultiset.h

+
+private:
+    /**
+     *


Looks like you miss the comment here.

novakboskov · 2020-07-21T16:40:26Z

src/Communicants/Communicant.cpp

@@ -278,6 +278,18 @@ void Communicant::commSend(const IBLT& iblt, bool sync) {
    }
 }

+void Communicant::commSend(const IBLTMultiset &iblt, bool sync) {


This is a kind of problem that comes with this kind of inheritance. You have void Communicant::commSend(const IBLT& iblt, bool sync) and void Communicant::commSend(const IBLTMultiset &iblt, bool sync) that are practically identical. And that is because sending an IBLT over the network apparently has nothing to do with the kind of adder you use in its implementation. And that tells us that it might be that Adder is what needs its own class and not the IBLTWithSomeOtherAdder.

Alternatively, you can stick with this inheritance but then keep void Communicant::commSend(const IBLT &iblt, bool sync) only. As IBLTMultiset inherits from IBLT that should work fine.

novakboskov · 2020-07-21T16:46:22Z

src/Syncs/IBLTMultiset.cpp

+    // must return positive modulus
+    // C++ by default does not give positive modulus
+    if (res < 0 ){
+        res += LARGE_PRIME;


Don't use unnecessary braces.

Well I actually like being explicit here, just to avoid any future errors. But this is fine too :)

novakboskov · 2020-07-21T16:52:17Z

src/Syncs/IBLTMultiset.cpp

+}
+
+void IBLTMultiset::erase(ZZ key, ZZ value) {
+    _insertModular(-1, key, value);


Very bad idea to have _insertModular actually removing stuff when a negative value is passed. At least, this should be two function API or the name of the method should be changed to something along the line of modify.

novakboskov · 2020-07-21T16:54:30Z

src/Syncs/IBLTMultiset.cpp

+    if (count != 0 && keySum!=0) {
+        long absCount = abs(count);
+        long plusOrMinus;
+        NTL::conv(plusOrMinus, keySum / abs(keySum));


We normally use the functional versions of the NTL conversions on this project.

Thanks for pointing out! Linking the doc here for reference, if it's of use to someone in the future.

arorashu added 9 commits June 23, 2020 16:46

Run Sync with IBLTSync and CPISync as:

5e5b091

1. load client & server data from file 2. run client process, server process 3. print stats about sync

modular arithmetic based IBLT implementation for multisets

26a7af8

-more IBLT residual sum changes

501e109

- add test for IBLT multiset insert

96e0cf7

- temporarily add IBLT unit tests target

d47bc45

- Create IBLTSync_Multiset.h for multiset IBLTSync

949dae2

- Use `IBLTSync_Multiset` in place of `IBLTSync` in tests where multiset is used - Default IBLT uses isMultiset=false

- Remove unnecessary comments, add docstrings

c2fd9f6

- initialize default value for isMultiset flag,

7051890

because defaulted default constructor defined outside the class is treated as user defined, and is initialized with garbage values

Remove divide by zero scenario

8171d89

arorashu requested a review from novakboskov July 15, 2020 16:47

- Create function pointers for get, insert and listEntries

fce6b5c

- create constructor to initialize the function pointers

arorashu added 5 commits July 20, 2020 20:15

git cherry pick fixes

86381ad

- Create separate classes for IBLTMultiset, IBLTSync_Multiset

8e98c6a

- create communication functions for IBLTMultiset

- commit IBLTSync_Multiset.cpp file

8126927

- remove comments

773cff2

- cleanup files for PR review

f37742b

novakboskov reviewed Jul 21, 2020

View reviewed changes

Minor refactoring reflecting my comments on the PR

5391c46

novakboskov merged commit ad06b44 into master Jul 22, 2020

This was referenced Jul 22, 2020

Multiset reconciliation with IBLTSync #63

Closed

Support addition as a parameter in IBLT #64

Closed

arorashu deleted the ibltsync-multiset branch August 3, 2020 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiset support for IBLT #66

Add multiset support for IBLT #66

arorashu commented Jul 15, 2020

arorashu commented Jul 20, 2020

trachten commented Jul 20, 2020 via email

arorashu commented Jul 21, 2020

novakboskov commented Jul 21, 2020

novakboskov left a comment •

edited

Loading

novakboskov Jul 21, 2020

novakboskov Jul 21, 2020

novakboskov Jul 21, 2020

novakboskov Jul 21, 2020

arorashu Jul 21, 2020

novakboskov Jul 21, 2020

novakboskov Jul 21, 2020

arorashu Jul 21, 2020

Add multiset support for IBLT #66

Add multiset support for IBLT #66

Conversation

arorashu commented Jul 15, 2020

arorashu commented Jul 20, 2020

trachten commented Jul 20, 2020 via email

arorashu commented Jul 21, 2020

novakboskov commented Jul 21, 2020

novakboskov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novakboskov left a comment •

edited

Loading