Principal.mo uses deprecated function

[timohanke]

motoko-base/src/Principal.mo

Line 80 in 8028da2

Blob.fromArray(Array.append(crc32Bytes, Blob.toArray(hashSum)))

It is a little weird if the base library itself uses deprecated functions because it produces compiler warnings that the user cannot do anything about. Shall we inline the Array.append function here?

[ottodevs]

100% agree with your observation about the use of deprecated functions in the base library. It's indeed confusing and can lead to unnecessary compiler warnings.

Regarding the documentation's advice on Array.append, it's important to note that its linear complexity applies with each use, not just within loops. This inefficiency is more significant in loops due to repetitive copying and increased resource usage. However, for single operations like in the code snippet you've shared, the impact might not be as critical, but it's still not ideal.

(from documentation:)

@deprecated Array.append copies its arguments and has linear complexity; when used in a loop, consider using a Buffer, and Buffer.append, instead.

I've tested an alternative approach locally that avoids using Array.append by manually handling the concatenation. This method involves creating a new array of the combined size of crc32Bytes and hashSum, then populating it with elements from both arrays. This approach avoids the additional copies created by Array.append and offers more direct control over memory management. Additionally, I used Array.freeze to convert the mutable array combinedArray into an immutable array before passing it to Blob.fromArray. This is necessary because Blob.fromArray expects an immutable array. Here's the modified code:

Replace:

 Blob.fromArray(Array.append(crc32Bytes, Blob.toArray(hashSum))) 

With:

 let hashSumArray = Blob.toArray(hashSum);
 
 let combinedSize = Array.size(crc32Bytes) + Array.size(hashSumArray);
 let combinedArray = Array.init<Nat8>(combinedSize, 0);

 for (i in Iter.range(0, Array.size(crc32Bytes) - 1)) {
   combinedArray[i] := crc32Bytes[i];
 };
 
for (i in Iter.range(0, Array.size(hashSumArray) - 1)) {
   combinedArray[Array.size(crc32Bytes) + i] := hashSumArray[i];
 };

 Blob.fromArray(Array.freeze(combinedArray));

This revised approach should provide a more efficient and warning-free solution, but since I am fairly new to motoko I would like to get feedback on this before submitting a PR.

Edit: Just to add that we also would need to import Iter at top of the file for it to work properly:

import Iter "Iter";

[timohanke]

Thank for the contribution. I think Array.freeze will copy the whole array which means your code would not gain anything over the existing code.

I would just copy the definition of Array.append into Principal.mo and not replace the line. Because having only one line makes it easy to read. Or, alternatively, inline Array.append here which is essentially Array.tabulate.

[ottodevs]

Hi Timo,

Thanks for the heads-up about Array.freeze. Totally see your point about the potential extra copy. I've tweaked the code to inline the Array.append logic, aiming for efficiency without losing clarity. Here's what it looks like now:

  return Blob.fromArray(
    Prim.Array_tabulate<Nat8>(
      Array.size(crc32Bytes) + Array.size(Blob.toArray(hashSum)),
      func i {
        if (i < Array.size(crc32Bytes)) {
          crc32Bytes[i]
        } else {
          Blob.toArray(hashSum)[i - Array.size(crc32Bytes)]
        }
      }
    )
  );

Would love to know if this sits right with you. If it does, I can get the PR ready.

Cheers.

(Updated to refactor into a more concise version)

[ottodevs]

related: #522

[ottodevs]

Thank for the contribution. I think Array.freeze will copy the whole array which means your code would not gain anything over the existing code.

I would just copy the definition of Array.append into Principal.mo and not replace the line. Because having only one line makes it easy to read. Or, alternatively, inline Array.append here which is essentially Array.tabulate.

Just a quick follow-up on our previous discussion. I've gone ahead and inlined Array.tabulate directly for now, as it seemed to make the code neater and this specific logic isn’t used elsewhere in Principal.mo. But I’ve also prepared a version that keeps the one-liner style by copying the Array.append logic into a new function. I'm open to your opinion on which approach fits best with the overall code structure.

Here is the other version (external auxiliary function outside the toLedgerAccount func:

  /// Append two arrays into a new array.
  ///
  /// @param array1 The first array to append.
  /// @param array2 The second array to append.
  /// @return A new array containing the elements of array1 followed by the elements of array2.
  public func appendArrays<X>(array1 : [X], array2 : [X]) : [X] {
    let size1 = Array.size(array1);
    let size2 = Array.size(array2);
    Prim.Array_tabulate<X>(
      size1 + size2,
      func i {
        if (i < size1) {
          array1[i]
        } else {
          array2[i - size1]
        }
      }
    )
  };

By the way, I was proactively setting up the PR, but I hit a snag with some failing tests in the master branch, specifically the RBTree test, which has held up my PR submission. I'll keep you posted once that's sorted out.

Thanks