Skip to content

Conversation

basvandijk
Copy link
Contributor

@basvandijk basvandijk commented Apr 24, 2025

This makes the necessary changes to upgrade rust-nightly except actually bumping rust-nightly. This makes it easier to do the eventual upgrade later on, and simplifies the debugging as is done in: #5074.

Copy link
Contributor

github-actions bot commented Apr 24, 2025

Comparing from 5c21198 to f27f356:
In terms of gas, 3 tests regressed and the mean change is +0.0%.
In terms of size, 5 tests regressed and the mean change is +95.0%.

@ggreif ggreif changed the title chore: prepare for a rust-nightly upgrade chore: prepare for a rust-nightly upgrade May 24, 2025
@ggreif ggreif force-pushed the rust-nightly-upgrade-preparation branch 4 times, most recently from c3e42b2 to c801962 Compare May 25, 2025 06:33
@ggreif
Copy link
Contributor

ggreif commented May 25, 2025

@basvandijk in .github/actions/performance/action.yml the patch -p1 -R command fails with code 1 after successfully patching 4 files... Can you think of any reason why this would happen?

EDIT: man patch sez:

1 if some hunks cannot be applied or there were merge conflicts

but: using --verbose none of these were reported. So I am still confused.

@ggreif ggreif force-pushed the rust-nightly-upgrade-preparation branch 2 times, most recently from 4d7f415 to ecc7414 Compare May 25, 2025 10:11
@ggreif ggreif self-assigned this May 25, 2025
@ggreif ggreif force-pushed the rust-nightly-upgrade-preparation branch 2 times, most recently from bc52c19 to 4980586 Compare May 25, 2025 11:25
@@ -27,7 +27,7 @@ runs:
- name: Discover `test/bench` changes
if: ${{ inputs.is_fork == 'false' }}
run: |
git diff --no-index $(nix build --print-out-paths .#tests.bench)/share test/bench/ok | patch -p1 -R
git diff --no-index $(nix build --print-out-paths .#tests.bench)/share test/bench/ok | patch -f -u -p1 -R
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because sometimes we got an exit code 1 that messed up the pipeline. -f fixes that for good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only it doesn't. It is super strange.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied a band-aid until I understand this: 587d130.

@ggreif ggreif marked this pull request as ready for review May 25, 2025 11:28
@ggreif ggreif requested a review from a team as a code owner May 25, 2025 11:28
@basvandijk
Copy link
Contributor Author

Strange. When I execute the command locally patch returns successfully:

bas@devenv-bas:~/d/motoko/motoko$ git diff --no-index $(nix build --print-out-paths .#tests.bench)/share test/bench/ok | patch -f -u -p1 -R
patching file test/bench/ok/candid-subtype-cost.drun-run.ok
patching file test/bench/ok/heap-32.drun-run.ok
patching file test/bench/ok/heap-64.drun-run.ok
patching file test/bench/ok/palindrome.drun-run.ok
bas@devenv-bas:~/d/motoko/motoko$ echo $?
0
bas@devenv-bas:~/d/motoko/motoko$ git diff
diff --git a/test/bench/ok/candid-subtype-cost.drun-run.ok b/test/bench/ok/candid-subtype-cost.drun-run.ok
index fa8d5cdbd..4eedb5c3c 100644
--- a/test/bench/ok/candid-subtype-cost.drun-run.ok
+++ b/test/bench/ok/candid-subtype-cost.drun-run.ok
@@ -1,4 +1,4 @@
 ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
 ingress Completed: Reply: 0x4449444c0000
-debug.print: {cycles = 911_177; heap_bytes = +12_568}
+debug.print: {cycles = 910_817; heap_bytes = +12_568}
 ingress Completed: Reply: 0x4449444c0000
diff --git a/test/bench/ok/heap-32.drun-run.ok b/test/bench/ok/heap-32.drun-run.ok
index 5d175041d..20762a59b 100644
--- a/test/bench/ok/heap-32.drun-run.ok
+++ b/test/bench/ok/heap-32.drun-run.ok
@@ -1,5 +1,5 @@
 ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
 ingress Completed: Reply: 0x4449444c0000
-debug.print: (50_227, +29_863_068, 723_584_959)
-debug.print: (50_070, +32_992_212, 785_200_311)
+debug.print: (50_227, +29_863_068, 723_584_599)
+debug.print: (50_070, +32_992_212, 785_200_671)
 ingress Completed: Reply: 0x4449444c0000
diff --git a/test/bench/ok/heap-64.drun-run.ok b/test/bench/ok/heap-64.drun-run.ok
index d28df15ef..815a4da64 100644
--- a/test/bench/ok/heap-64.drun-run.ok
+++ b/test/bench/ok/heap-64.drun-run.ok
@@ -1,5 +1,5 @@
 ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
 ingress Completed: Reply: 0x4449444c0000
-debug.print: (49_965, +47_942_744, 987_908_675)
-debug.print: (49_806, +47_960_000, 996_956_504)
+debug.print: (49_965, +47_942_744, 987_907_181)
+debug.print: (49_806, +47_960_000, 996_956_864)
 ingress Completed: Reply: 0x4449444c0000
diff --git a/test/bench/ok/palindrome.drun-run.ok b/test/bench/ok/palindrome.drun-run.ok
index e82e65287..1ee8e5701 100644
--- a/test/bench/ok/palindrome.drun-run.ok
+++ b/test/bench/ok/palindrome.drun-run.ok
@@ -1,7 +1,7 @@
 ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
 ingress Completed: Reply: 0x4449444c0000
 debug.print: (true, +1_188, 12_573)
-debug.print: (false, +1_188, 11_622)
+debug.print: (false, +1_188, 12_045)
 debug.print: (false, +1_188, 12_552)
 debug.print: (true, +868, 12_713)
 debug.print: (false, +868, 11_176)

@ggreif
Copy link
Contributor

ggreif commented May 25, 2025

Strange. When I execute the command locally patch returns successfully:

RIght. Same for me (had to change to aarch64-darwin to do it locally).

@@ -38,3 +38,48 @@ fn written_length(buffer: &[u8]) -> usize {
}
buffer.len()
}

#[no_mangle]
pub fn pow(a: f64, b: f64) -> f64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe later we can short-circuit these to avoid the indirection

@@ -145,7 +145,6 @@ impl Stream {
}

/// Ingest a single byte into the stream.
#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change intentional? Seems unrelated to the rest of the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC a newer rustc complained that it's not possible to inline exported functions.

@@ -1,4 +1,4 @@
ingress Completed: Reply: 0x4449444c016c01b3c4b1f204680100010a00000000000000000101
ingress Completed: Reply: 0x4449444c0000
debug.print: {cycles = 911_177; heap_bytes = +12_568}
debug.print: {cycles = 910_817; heap_bytes = +12_568}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, why does this change cycles? also below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly libm has an optimisation?

@ggreif ggreif force-pushed the rust-nightly-upgrade-preparation branch from 4644e46 to f27f356 Compare June 18, 2025 09:30
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.

4 participants