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

Borrow inference #6621

Closed
wants to merge 7 commits into from
Closed

Borrow inference #6621

wants to merge 7 commits into from

Conversation

folkertdev
Copy link
Contributor

Adds borrow inference for arguments of type List _ and Str (not tag unions of any kind, not structs containing these types).

Our observation has been for a while that while perceus etc. are excellent for algebraic types, they cause suboptimal behavior for roc's builtin data structures (list and str).

In the perceus implementation, arguments are always passed as Owned to another function. This is desirable when the callee can perform an update to the argument value: if the argument is unique at runtime, the update can occur in-place.

But passing as owned is suboptimal when the callee only reads from the argument (e.g. List.get). Passing the argument as owned in this case likely requires an extra reference count increment, because it is likely that the code performs another read of the data. That means the data needs to be kept alive, hence the additional increment.

A concrete example:

helper = \list, index -> List.get index

x = helper list i
y = helper list j

x + y

with the old approach, helper takes its arguments as owned values. To make that work, an additional inc is needed:

helper = \list, index -> 
    tmp = List.get list index
    dec list
    tmp

inc list
x = helper list i
y = helper list j

x + y

With borrow inference, because no updates can ever happen to list in the body of helper, the argument is inferred as borrowed.
That means the inferred incs/decs are instead

helper = \list, index -> 
    List.get list index

x = helper list i
y = helper list j
dec list

x + y

I've opened the PR now mostly for CI reasons: I don't know for sure if the implementation is entirely correct.

I've disabled an assert in inc_dec.rs that verifies that initially join point arguments are owned. No idea if that is correct.

cc @JTeeuwissen @bhansconnect

@JTeeuwissen
Copy link
Collaborator

Hi again,
How did you implement borrow inference?
Borrow inference was omitted from Perceus as it causes an unbouded increase in peak memory usage, whenever such a borrowed parameter is the latest usage of such a value.

@folkertdev
Copy link
Contributor Author

Borrow inference was omitted from Perceus as it causes an unbouded increase in peak memory usage, whenever such a borrowed parameter is the latest usage of such a value.

True, but the alternative is worse. We've seen really bad performance in simple programs due to the inc and decs that need to be inserted to pass values as owned. The problem is especially bad, and preventable, for the builtin types list and string. For them we can know before inserting increments and decrement whether in-place mutation is ever possible for a value. (as you know, we can't do this for recursive tag unions, because reset/reuse is based on whether values are dropped, so we can't let ownership depend on in-place mutation opportunities, that would be circular).

So basically, we're giving up the property of bounded memory usage (a problem in theory) for improved performance (a problem in practice). Our assumption is that the difference in peak memory usage in practice will be small.

How did you implement borrow inference?

The current analysis only considers function arguments with a layout of List _ or Str. For those argument symbols, we see whether they ever need to be owned: ultimately this means: are they passed to a lowlevel that needs its argument to be owned; this property is propagated upward.

That is naive in several ways, but relatively straightforward to implement. There is a bug somewhere though, but sadly none of our fine-grained tests hit it. Only the (massive) RustGlue.roc has problems apparently. It very well might be something joinpoint-related.

Copy link

github-actions bot commented May 3, 2024

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

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.

None yet

2 participants