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.

Sign up for GitHub

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

add share/unshare for class objects #180

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add share/unshare for class objects #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
229ecc5
add share/unshare for class objects
chenyan-dfinity Sep 22, 2020
d1cd28e
add Buffer
chenyan-dfinity Sep 22, 2020
6d2569c
fix
chenyan-dfinity Sep 22, 2020
13f62c8
fix
chenyan-dfinity Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Buffer.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,23 @@ public class Buffer<X> (initCapacity : Nat) {
var count : Nat = 0;
var elems : [var X] = [var]; // initially empty; allocated upon first `add`

/// Get purely-functional representation
public func share() : [X] {
Prim.Array_tabulate<X>(count, func i = elems[i])
};

/// Put purely-functional representation into class.
public func unshare(xs : [X]) {
count := xs.size();
if (count == 0) {
elems := [var];
};
elems := Prim.Array_init<X>(count, xs[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an else missing here? Otherwise this will trap when empty.

for (i in elems.keys()) {
elems[i] := xs[i];
};
};

/// Adds a single element to the buffer.
public func add(elem : X) {
if (count == elems.size()) {
Expand Down
11 changes: 11 additions & 0 deletions src/HashMap.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ public class HashMap<K,V> (
var table : [var KVs<K,V>] = [var];
var _count : Nat = 0;

/// Get purely-functional representation
public func share() : ([KVs<K,V>], Nat) {
(A.freeze table, _count)
};

/// Put purely-functional representation into class. Need to make sure the tree is constructed with the same Eq and Hash functions
Copy link
Contributor

Choose a reason for hiding this comment

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

I vote that we either write those checking functions and use them, or remove this until we have them?

Is this a performance thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main use case is for upgrade. We cannot make HashMap stable, so the upgrade code would look like:

  stable var pureHashMap : HashMap.Tree = [];
  flexible let db: HashMap.HashMap = HashMap.unshareUnsafe(pureHashMap);
  system func preupgrade() {
   pureHashMap := HashMap.share();
  };

The db can be very large when upgrade, it's probably too expensive to check for the invariants.Also for the upgrade case, the invariants always hold.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

This is another use case for types that hide internal data but are not object types. (existential of some flavor? @crusso @rossberg ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious how BigMap handles upgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't handle it at the moment, but similar issues will arise (how to enforce invariants for the serialized representation without rechecking them).

Also, I think this question will arise for each developer, each time they implement such functions, for any canister with data invariants (so, almost all of them).

Perhaps we should add some notes in the design doc for share and unshare that just clarifies this design point (Do we expect these functions to be defensive and re-establish invariants, or to be trusting and assume that they hold, as with your unsafe variant?)

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/dfinity/motoko-base/blob/master/doc/design.md#classes

Currently reads as follows:

The share/unshare functions of a class need to convert to a type that is designed for stability (potentially, including extensibility) and space efficiency, not for enabling efficient direct operations.

If we follow this guide, it seems like we should be using a vector of key-value pairs for things like BigMap, and for other map-like structures, and shouldn't even include the hash values if they were computed for internal use only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, if at all possible, let's make such functions safe. Otherwise you break encapsulation. I doubt that the copying with freeze and thaw are significantly cheaper than a safe alternative.

Also, for polymorphic data structures, these functions need to be parameterised over the share/unshare functions for each type parameter, otherwise you cannot compose them properly.

public func unsafeUnshare(t : [KVs<K,V>], count : Nat) {
table := A.thaw(t);
_count := count;
};

/// Returns the number of entries in this HashMap.
public func size() : Nat = _count;

Expand Down
7 changes: 6 additions & 1 deletion src/RBTree.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,17 @@ public class RBTree<X, Y>(compareTo:(X, X) -> O.Order) {
/// Tree as share data.
///
/// Get non-OO, purely-functional representation:
/// for drawing, pretty-printing and non-OO contexts
/// for drawing, pretty-printing, upgrades and non-OO contexts
/// (e.g., async args and results):
public func share() : Tree<X, Y> {
tree
};

/// Put purely-functional representation into class. Need to make sure the tree is constructed with the same compare function
public func unsafeUnshare(t: Tree<X, Y>) {
tree := t;
};

/// Get the value associated with a given key.
public func get(x:X) : ?Y =
getRec(x, compareTo, tree);
Expand Down