-
Notifications
You must be signed in to change notification settings - Fork 9
chore: make pure/List mostly tail-recursive
#189
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
Conversation
|
No description provided. |
af53a1f to
772d184
Compare
772d184 to
a6cbcb0
Compare
7ed377d to
f4ee7e2
Compare
f4ee7e2 to
38281ef
Compare
test/pure/List.test.mo
Outdated
| @@ -1,3 +1,5 @@ | |||
| // @testmode wasi | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just to speed up the tests? We might want to run this with the interpreter (default) to check for stack overflows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it appeared that the interpreter was more tolerant in terms stack. Maybe we should have a // @testmode both directive, if not already possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove just before merging. Then we'll have both ways tested. But we should think about a combined strategy (maybe nightlies) for the future.
src/pure/List.mo
Outdated
| list, | ||
| func(item : T) { | ||
| if first { | ||
| if (text.size() > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure text.size() is constant time when using ropes (in compiled code)? I don't recall off-hand. If not, revert!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll revert!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It's no 0(1), I checked)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, compiling the size() > 1 as a unit would be constant time even in the presence of ropes. I expect users committing the same mistake in real life, and getting quadratic runtime on code like this (i.e. comparisons of size() with small constants).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, do ropes carry a size around? That would help certainly :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice:
// Layout of a concat node:
//
// ┌────────────┬─────────┬───────┬───────┐
// │ obj header │ n_bytes │ text1 │ text2 │
// └────────────┴─────────┴───────┴───────┘
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is resolved in the good way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting, those are bytes, not chars.
| public func size<T>(list : List<T>) : Nat = ( | ||
| func go(n : Nat, list : List<T>) : Nat = switch list { | ||
| case (?(_, t)) go(n + 1, t); | ||
| case null n | ||
| } | ||
| )(0, list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public func size<T>(list : List<T>) : Nat = ( | |
| func go(n : Nat, list : List<T>) : Nat = switch list { | |
| case (?(_, t)) go(n + 1, t); | |
| case null n | |
| } | |
| )(0, list); | |
| public func size<T>(list : List<T>) : Nat = { | |
| var size = 0; | |
| var cur = list; | |
| loop { | |
| switch tmp { | |
| case (?(_, next)) { | |
| size += 1; | |
| cur:= next | |
| }; | |
| case null { return size } | |
| } | |
| } | |
| } |
Is probably cheaper (no call to a helper).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I was hoping that the compiler could create the same (or better) code by TCO. I.e. not sure if var allocates in this example (it shouldn't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the same transformation in repeat, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverse needs it too. TCO won't inline the auxiliary function so ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually test reverse for stack overflow and I haven't seen any 😖
With depth 100_000!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for TCO one only have to turn the body of the helper into a loop, no need to inline it into the wrapper. Or am I mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course the interpreter won't do TCO, and if we want constant stack there we'll have to manually TCO anyway... Or maybe due to CPS the interpreter will do the right thing. Questions, questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually test
reversefor stack overflow and I haven't seen any 😖 With depth 100_000!
If you test in the interpreter you won't see because the interpreter is in CPS... On wasm, you should see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I test on Wasm, and don't see it :-) I'll look at IR/Wasm soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you reverse is tail recursive, but it allocates and calls a helper that is probably slower than just using a loop and a temp, without the additional function call.
|
There's more TODO (see #195) |
| l := ?(f i, l); | ||
| i += 1 | ||
| }; | ||
| reverse l |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a shame. A clever optimisation would spot the linear heap allocation discipline arising from the recursive algorithm (did somebody say abstract interpretation?) and pre-allocate the list nodes. Then fill them in like an array...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's called tail-modulo-cons optimization but we don't have it.
|
Is this still a draft? |
10dd101 to
4ba2415
Compare
5f43ca0 to
1791ac0
Compare
e98827e to
84bb4ef
Compare
pure/List mostly tail-recursive
84bb4ef to
b975271
Compare
TODO:
sizecould be more efficient with a looprepeatdittotabulateis not tail-recursive (but constructor tailed)concatis not tail-recursive (but constructor tailed)mergeis not tail-recursive (but constructor tailed)fromIteris not tail-recursive (but constructor tailed)reverselooks tail recursive (could be more efficient with a loop)mapResultneeds another look (somewhat convoluted in legacybase)