Buffer Class Extension #416

kentosugama · 2022-09-22T21:45:27Z

Base Library Extension Doc: https://docs.google.com/document/d/1uKouujLL3KueLpqjjFV2gd4dqu_6GYD107WBb6_GSVE/edit

This last point is related to the RRB Trees, Rope, and Finger Trees data structures that I was discussing before. For various reasons, I am leaning towards an RRB Tree implementation of a scalable sequence structure, if it is necessary. See a talk on this data structure here: https://www.youtube.com/watch?v=sPhpelUfu8Q

kentosugama · 2022-10-10T19:44:02Z

Apologies that this is such a large PR. I'll definitely keep in mind for the future to break PRs into smaller, more frequent chunks.

luc-blaeser · 2022-10-17T16:21:06Z

src/Buffer.mo

+    };
+
+    return true;
+  };


Similarly, here:

public func isStrictSuffixOf<X>(suffix : Buffer<X>, buffer : Buffer<X>, equal : (X, X) -> Bool) : Bool { if (buffer.size() <= suffix.size()) { return false; }; isSuffixOf(suffix, buffer, equal); };

luc-blaeser

Great work, lots of functionality. Just had a few small suggestions.

luc-blaeser

Thanks a lot for the changes.

crusso · 2022-10-18T16:41:49Z

src/Array.mo

@@ -42,7 +41,7 @@ module {
    };
  };

-  /// Sorts the given array according to the `cmp` function.
+  /// Sorts the given array according to the `compare` function.


Suggested change

/// Sorts the given array according to the `compare` function.

/// Sorts the given array, in ascending order, according to the `compare` function.

crusso · 2022-10-18T16:43:06Z

src/Array.mo

    freeze(tmp)
  };

-  /// Sorts the given array in place according to the `cmp` function.
+  /// Sorts the given array in place according to the `compare` function.


Suggested change

/// Sorts the given array in place according to the `compare` function.

/// Sorts the given array in place, in ascending order, according to the `compare` function.

crusso · 2022-10-18T16:45:51Z

src/Buffer.mo

+/// `capacity` is the length of the underyling array that backs this list.
+/// `capacity` >= `size` is an invariant for this class.
+/// 
+/// Like arrays, elements in the buffer are ordered by indices from 0 to size-1.


Suggested change

/// Like arrays, elements in the buffer are ordered by indices from 0 to size-1.

/// Like arrays, elements in the buffer are ordered by indices from 0 to `size`-1.

crusso · 2022-10-18T17:03:11Z

src/Buffer.mo

+      _size += size2;
+    };
+
+    /// Sorts the elements in the buffer according to `compare`.


Suggested change

/// Sorts the elements in the buffer according to `compare`.

/// Sorts the elements in the buffer, in ascending order, according to `compare`.

crusso · 2022-10-18T17:10:57Z

src/Buffer.mo

+  /// If any invocation of `f` produces an #err, returns an #err. Otherwise
+  /// Returns an #ok containing the new buffer.


Suggested change

/// If any invocation of `f` produces an #err, returns an #err. Otherwise

/// Returns an #ok containing the new buffer.

/// If any invocation of `f` produces an `#err`, returns an `#err`. Otherwise

/// Returns an `#ok` containing the new buffer.

crusso · 2022-10-18T17:26:06Z

src/Buffer.mo

+    i := 0;
+    j := 0;
+    while (i < subSize and j < size) {
+      if (equal(subBuffer.get(i), buffer.get(j)) and i == (subSize - 1 : Nat)) {


It might be worth hoisting (subSize - 1 : Nat) out of the loop, computing it just once.

Okay will do!

It would be cool to have a loop invariant pass for hosting these, since subSize is bound to a let.

crusso · 2022-10-18T17:34:52Z

src/Buffer.mo

+    let size = buffer.size();
+    if (size == 0) { [var] } else {
+      let newArray = Prim.Array_init<X>(size, buffer.get(0));
+      var i = 0;


Suggested change

var i = 0;

var i = 1;

Can't you start from 1 since you've already copied position 0?

crusso · 2022-10-18T17:50:25Z

src/Buffer.mo

+      Prim.trap "Chunk size must be non-zero in chunk";
+    };
+
+    let newBuffer = Buffer<Buffer<X>>(Prim.abs(Prim.floatToInt(Prim.floatCeil(Prim.intToFloat(buffer.size()) / Prim.intToFloat(size)))));


Do you really need to use floats here. Isn't integer division and remainder enough?

crusso · 2022-10-18T17:56:32Z

src/Buffer.mo

+  // The length of `elements` is increased by `INCREASE_FACTOR` when capacity is reached.
+  // The length of `elements` is decreased by `DECREASE_FACTOR` when capacity is strictly less than
+  // `DECREASE_THRESHOLD`.
+  private let INCREASE_FACTOR = 1.5; // Keep this factor low to minimize cycle limit problem


The floats make me slightly nervous - could you use a ratio instead? I.e. Integer multiply by 3 and divide by 2 or some other numerator denominator pair?

Yup, will change

crusso

Left some more suggestions. Lovely work! Next time, submit in smaller morsels - it's hard for a reviewer to take in so much code in one bite.

crusso · 2022-10-19T08:43:26Z

src/Buffer.mo

-      Prim.abs(Prim.floatToInt(Prim.floatCeil(Prim.intToFloat(oldCapacity) * INCREASE_FACTOR)));
+      // calculates ceil(oldCapacity * INCREASE_FACTOR) without floats
+      let product = oldCapacity * INCREASE_FACTOR_NUME;
+      (product / INCREASE_FACTOR_DENOM) + (if (product % INCREASE_FACTOR_DENOM != 0) { 1 } else { 0 })


Suggested change

(product / INCREASE_FACTOR_DENOM) + (if (product % INCREASE_FACTOR_DENOM != 0) { 1 } else { 0 })

(product + INCREASE_FACTOR_DENOM - 1 ) / INCREASE_FACTOR_DENOM

Might do the trick as well. @ggreif amirite? I very well may not be

@crusso yep this should work. Example: ((57 * 7) + 3 - 1) / 3 gives 133.7, 57 * 7/3 = 133.

Okay will swap out

crusso · 2022-10-19T08:44:50Z

src/Buffer.mo

+        buffer.size() / size + 
+        (if (buffer.size() % size != 0) { 1 } else { 0 })


Suggested change

buffer.size() / size +

(if (buffer.size() % size != 0) { 1 } else { 0 })

(buffer.size() + size - 1) / size

analogously. But check my arithmetic. (cc @ggreif)

crusso

(but see small refactorings, if correct)

ggreif · 2022-10-21T11:34:00Z

I'd like to re-open this as the commits have been removed from master.

kentosugama added 24 commits August 31, 2022 13:01

update ignore file

2782444

first attempt at buffer extension

94d9744

More first attempt Buffer methods

265e008

More buffer extensions

95e7fbd

more extensions

99c6b19

More buffer extensions. Still untested

59f046e

some buffer class cleanup

e3e7c2e

Merge branch 'master' into extensions

93afc95

refactor buffer class methods

ebe5aac

Add buffer unit testings with matchers library

4f266c9

Add more tests and refactoring

a258550

Checkpointing here. Might use optional type in underlying array

0943f43

Use option types in underlying array for buffer

6c8a65a

More tests

b4c4001

more buffer debug and tests

d1871d4

last tests

907b312

convert buffer to use iterators

b50fb6e

optimize sort in buffer

f658347

small opts

2f2c8cf

Fix CI

260614d

convert some class methods to static functions to reduce object size

8e1004b

Cleanup some error messages in Buffer

c91e245

Cleanup tests

783d669

Add documentation and fix style

ef1dfa5

kentosugama marked this pull request as ready for review October 10, 2022 19:39

kentosugama requested review from crusso, matthewhammer and ggreif October 10, 2022 19:41

kentosugama requested a review from chenyan-dfinity October 11, 2022 16:04

luc-blaeser reviewed Oct 17, 2022

View reviewed changes

luc-blaeser previously approved these changes Oct 17, 2022

View reviewed changes

Incorporate Lucs feedback

aa0d95d

kentosugama dismissed luc-blaeser’s stale review via aa0d95d October 18, 2022 04:55

luc-blaeser self-requested a review October 18, 2022 15:45

luc-blaeser previously approved these changes Oct 18, 2022

View reviewed changes

crusso reviewed Oct 18, 2022

View reviewed changes

crusso previously approved these changes Oct 18, 2022

View reviewed changes

Incorporate Claudios feedback

77b29a7

kentosugama dismissed stale reviews from crusso and luc-blaeser via 77b29a7 October 19, 2022 02:52

crusso reviewed Oct 19, 2022

View reviewed changes

crusso previously approved these changes Oct 19, 2022

View reviewed changes

Arithmetic opts

b4e3cd6

kentosugama dismissed crusso’s stale review via b4e3cd6 October 20, 2022 04:42

crusso approved these changes Oct 20, 2022

View reviewed changes

kentosugama merged commit a224e57 into master Oct 21, 2022

kentosugama deleted the extensions branch October 21, 2022 01:42

ggreif restored the extensions branch October 21, 2022 11:31

ggreif mentioned this pull request Oct 21, 2022

Buffer Class Extension #417

Merged

16 tasks

	/// Sorts the given array according to the `compare` function.
	/// Sorts the given array, in ascending order, according to the `compare` function.

	/// Sorts the given array in place according to the `compare` function.
	/// Sorts the given array in place, in ascending order, according to the `compare` function.

	/// Like arrays, elements in the buffer are ordered by indices from 0 to size-1.
	/// Like arrays, elements in the buffer are ordered by indices from 0 to `size`-1.

	/// Sorts the elements in the buffer according to `compare`.
	/// Sorts the elements in the buffer, in ascending order, according to `compare`.

		/// If any invocation of `f` produces an #err, returns an #err. Otherwise
		/// Returns an #ok containing the new buffer.

	(product / INCREASE_FACTOR_DENOM) + (if (product % INCREASE_FACTOR_DENOM != 0) { 1 } else { 0 })
	(product + INCREASE_FACTOR_DENOM - 1 ) / INCREASE_FACTOR_DENOM

		buffer.size() / size +
		(if (buffer.size() % size != 0) { 1 } else { 0 })

	buffer.size() / size +
	(if (buffer.size() % size != 0) { 1 } else { 0 })
	(buffer.size() + size - 1) / size

Buffer Class Extension #416

Buffer Class Extension #416

Uh oh!

Conversation

kentosugama commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentosugama commented Oct 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luc-blaeser left a comment

Choose a reason for hiding this comment

Uh oh!

luc-blaeser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Oct 19, 2022 • edited by ggreif Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

ggreif commented Oct 21, 2022

Uh oh!

Uh oh!

kentosugama commented Sep 22, 2022 •

edited

Loading

crusso left a comment •

edited

Loading

crusso Oct 19, 2022 •

edited by ggreif

Loading