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

AssocList.replace: Even less allocations #541

Closed
wants to merge 1 commit into from

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented Mar 8, 2023

@crusso tempted me to look at #535, and I found some more improvements, by avoiding to reconstruct the (hd_k, hd_v) tuple.

Before:

{heap_diff = 16_080_000; instructions = 92_841_876}

After:

{heap_diff = 8_088_000; instructions = 69_362_716}

@crusso tempted me to look at dfinity#535, and I found some more improvements,
by avoiding to reconstruct the `(hd_k, hd_v)` tuple.

Before:
```
{heap_diff = 16_080_000; instructions = 92_841_876}
```
After:
```
{heap_diff = 8_088_000; instructions = 69_362_716}
```
@ghost ghost added the cla:agreed label Mar 8, 2023
@nomeata
Copy link
Contributor Author

nomeata commented Mar 8, 2023

I also found the problem why the old code was allocating. The same effect as #535 could be achieved with

diff --git a/test.mo b/test.mo
index 2c52719..65bb9ac 100644
--- a/test.mo
+++ b/test.mo
@@ -14,8 +14,8 @@ actor {
       switch (al) {
         case (null) {
           switch value {
-            case (null) { (null, null) };
-            case (?value) { (?((key, value), null), null) }
+            case (null) { return (null, null) };
+            case (?value) { return (?((key, value), null), null) }
           }
         };
         case (?((hd_k, hd_v), tl)) {
@@ -23,12 +23,12 @@ actor {
             // if value is null, remove the key; otherwise, replace key's old value
             // return old value
             switch value {
-              case (null) { (tl, ?hd_v) };
-              case (?value) { (?((hd_k, value), tl), ?hd_v) }
+              case (null) { return (tl, ?hd_v) };
+              case (?value) { return (?((hd_k, value), tl), ?hd_v) }
             }
           } else {
             let (tl2, old_v) = rec(tl);
-            (?((hd_k, hd_v), tl2), old_v)
+            return (?((hd_k, hd_v), tl2), old_v)
           }
         }
       }

Why? Because we support unboxed tuple returns from functions, but not from blocks! So a switch block cannot return multiple values.

This should hopefully work out of the box once we can use the multi-value extension (dfinity/motoko#1459)

The relevant backend code is

https://github.com/dfinity/motoko/blob/409363d6bef896a3ebbd041c432e3233a12160b0/src/codegen/compile.ml#L9781-L9796

There is probably a reason I am not using a local and not a branch instruction to exit the block, which I do not recall at the moment.

@nomeata
Copy link
Contributor Author

nomeata commented Mar 9, 2023

In dfinity/motoko#3566 I improved the situation for if; arguably switch should behave the same.

@nomeata
Copy link
Contributor Author

nomeata commented Mar 9, 2023

This is part of #539

@nomeata nomeata closed this Mar 9, 2023
@nomeata nomeata deleted the joachim/assoc-list-replace branch March 9, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant