Skip to content

Fix panics on In-place update optimization#1149

Merged
swernli merged 2 commits into
mainfrom
swernli/issue1146
Feb 14, 2024
Merged

Fix panics on In-place update optimization#1149
swernli merged 2 commits into
mainfrom
swernli/issue1146

Conversation

@swernli
Copy link
Copy Markdown
Collaborator

@swernli swernli commented Feb 12, 2024

This checks ahead of time if the array we want to update in-place is uniquely referenced and correctly falls back to the previous, unoptimized path that makes a copy.

Fixes #1146

This checks ahead of time if the array we want to update in-place is uniquely referenced and correctly falls back to the previous, unoptimized path that makes a copy.

Fixes #1146
@github-actions
Copy link
Copy Markdown

Benchmark for b702a95

Click to view benchmark
Test Base PR %
Array append evaluation 773.0±6.94µs 803.4±19.95µs +3.93%
Array literal evaluation 519.9±24.39µs 515.4±4.77µs -0.87%
Array update evaluation 849.0±3.78µs 863.8±12.05µs +1.74%
Deutsch-Jozsa evaluation 30.5±0.24ms 30.6±0.22ms +0.33%
Large file parity evaluation 35.2±0.11ms 35.5±0.16ms +0.85%
Large input file 42.6±1.28ms 43.2±1.44ms +1.41%
Standard library 24.6±0.98ms 24.4±0.86ms -0.81%
Teleport evaluation 97.0±1.88µs 100.4±1.84µs +3.51%

@github-actions
Copy link
Copy Markdown

Benchmark for a286bb7

Click to view benchmark
Test Base PR %
Array append evaluation 767.5±3.69µs 750.3±10.18µs -2.24%
Array literal evaluation 535.1±6.58µs 520.1±29.94µs -2.80%
Array update evaluation 844.9±6.90µs 821.5±6.26µs -2.77%
Deutsch-Jozsa evaluation 30.4±0.23ms 30.4±0.33ms 0.00%
Large file parity evaluation 35.3±0.12ms 35.2±0.25ms -0.28%
Large input file 41.9±1.70ms 41.9±1.57ms 0.00%
Standard library 23.2±0.84ms 23.6±0.88ms +1.72%
Teleport evaluation 97.9±1.74µs 96.1±1.59µs -1.84%

@sezna
Copy link
Copy Markdown
Contributor

sezna commented Feb 13, 2024

I notice we do an ad-hoc in-place check to see if the array is used elsewhere, and then decide whether or not to make a copy. Do we have a formal/documented notion of the pass-by-value or pass-by-reference semantics of data structures in Q#? If we were to define such a thing, and use something like linear typing, we could automatically perform this optimization on all data types.

@swernli swernli added this pull request to the merge queue Feb 14, 2024
@swernli
Copy link
Copy Markdown
Collaborator Author

swernli commented Feb 14, 2024

Do we have a formal/documented notion of the pass-by-value or pass-by-reference semantics of data structures in Q#?

All values are read-only in Q#, even on mutable variables. The expectation for mutable variables is that they are allowed to be updated to "point" to a new value, rather than the actual value being mutated. This is why array index updates are only possible with "copy-update" expressions which are expected to copy. But when looking for ways to improve performance, we made it so aggregate data types, Arrays and Tuples, are actually Rc of value, which means we can avoid the overhead of copying the items every time. This made it so things like:

let x = [1, 2, 3];
let y = x;

would still only have one copy of [1, 2, 3] in memory and each variable has a reference. The further optimization we tried to do was in the case where "assign-update" and "assign-add" are used to update a mutable array and use that as an opportunity to work in-place:

mutable arr = [1, 2, 3];
set arr += [4, 5, 6];

Such that the append (or index update) can happen without creating an entire copy of the source array. The bug was that we always did it, running into a panic if there was another reference:

mutable arr = [1, 2, 3];
let copy = arr;
set arr += [4, 5, 6]; // in-place update fails because the source is not uniquely owned

With this change, we've effectively made it so that uniquely owned arrays can be updated in-place, otherwise we fall back to the old behavior of copy-on-write.

@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Feb 14, 2024
@swernli swernli added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit a3b9d04 Feb 14, 2024
@swernli swernli deleted the swernli/issue1146 branch February 14, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

In-place update optimization can cause panic on array of arrays

3 participants