Skip to content

fix: Use correct feature name for canbench profiling #296

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

Merged
merged 5 commits into from
May 16, 2025

Conversation

dsarlis
Copy link
Member

@dsarlis dsarlis commented Apr 30, 2025

The feature that enables specific canbench profiling of scoped code parts is named canbench-rs in Cargo.toml since a while ago. It seems that some of these were missed when the other cases were updated.

@dsarlis dsarlis requested a review from a team as a code owner April 30, 2025 14:14
@dsarlis dsarlis changed the title fix: Use correct feature name fix: Use correct feature name for canbench profiling Apr 30, 2025
@dsarlis dsarlis requested a review from Copilot April 30, 2025 14:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR corrects the feature flag used for canbench profiling to match the intended feature name defined in Cargo.toml.

  • Update feature flag from "canbench" to "canbench-rs" in node_load and node_save functions in both v1 and v2 nodes
  • Ensures that the profiling is only enabled when the correct feature is activated

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/btreemap/node/v2.rs Updated feature flag in node_load_v2 and save_v2
src/btreemap/node/v1.rs Updated feature flag in node_load_v1 and save_v1

@dsarlis dsarlis marked this pull request as draft April 30, 2025 14:25
@dsarlis
Copy link
Member Author

dsarlis commented Apr 30, 2025

Seems there's an issue with using scopes that can be repeated in benchmarks (e.g. when you call load multiple times) that I will need to figure out.

@maksymar
Copy link
Contributor

maksymar commented May 1, 2025

Seems there's an issue with using scopes that can be repeated in benchmarks (e.g. when you call load multiple times) that I will need to figure out.

I had that issue too. Initially I thought to create a key-value storage to record measurements for each unique scope. but it was adding too much of measurement overhead.

so I went with another approach, to record the name and timestamp data point (the fastest I came up with) and only do a post-processing to aggregate time spans for each scope when printing the stats.

see #274

@dsarlis
Copy link
Member Author

dsarlis commented May 2, 2025

Seems there's an issue with using scopes that can be repeated in benchmarks (e.g. when you call load multiple times) that I will need to figure out.

I had that issue too. Initially I thought to create a key-value storage to record measurements for each unique scope. but it was adding too much of measurement overhead.

so I went with another approach, to record the name and timestamp data point (the fastest I came up with) and only do a post-processing to aggregate time spans for each scope when printing the stats.

see #274

I was thinking that the right solution would be for canbench to allow repeated scopes. Isn't that something that others might potentially need/be restricted by? Would everyone need to implement their own custom solution like you did?

@dsarlis
Copy link
Member Author

dsarlis commented May 2, 2025

Seems there's an issue with using scopes that can be repeated in benchmarks (e.g. when you call load multiple times) that I will need to figure out.

I had that issue too. Initially I thought to create a key-value storage to record measurements for each unique scope. but it was adding too much of measurement overhead.
so I went with another approach, to record the name and timestamp data point (the fastest I came up with) and only do a post-processing to aggregate time spans for each scope when printing the stats.
see #274

I was thinking that the right solution would be for canbench to allow repeated scopes. Isn't that something that others might potentially need/be restricted by? Would everyone need to implement their own custom solution like you did?

I've got a draft in canbench that allows a repeated scope to cover the cases of loops or a function be called repeatedly. I think that's a better approach to deal with this instead of canbench users trying to go around it.

dsarlis added a commit to dfinity/canbench that referenced this pull request May 2, 2025
This change allows for repeated scopes to be used in canbench
benchmarks. Previously the code would straight up refuse to have a
similarly named scope -- I think this is a mistake, there are very valid
scenarios ([example
case](dfinity/stable-structures#296) from stable
structures) where someone might have a loop and they want to declare a
scope inside it to measure some operation.

People should already be careful not declaring the same names for
unrelated scopes, so I don't think a protection against it is worth it
(that's I guess the main reason for this restriction currently) and the
extra benefit you get to measure things inside loops (which can usually
be where heavier parts of processing happens) outweighs some risk that
someone re-uses the same name of scope.
Copy link

github-actions bot commented May 16, 2025

canbench 🏋 (dir: .) 41d6ec5 2025-05-16 12:44:47 UTC

./canbench_results.yml is up to date
📦 canbench_results_benchmark.csv available in artifacts

---------------------------------------------------

Summary:
  instructions:
    status:   Regressions detected! 🔴
    counts:   [total 124 | new 0 | improved 0 | regressed 98 | unchanged 26]
    change:   [min 0 | med +79.27 M | max +163.44 M]
    change %: [min 0.00% | med +9.06% | max +34.25%]

  heap_increase:
    status:   Regressions detected! 🔴
    counts:   [total 124 | new 0 | improved 0 | regressed 40 | unchanged 84]
    change:   [min 0 | med 0 | max +52]
    change %: [min 0.00% | med 0.00% | max +inf%]

  stable_memory_increase:
    status:   No significant changes detected 👍
    counts:   [total 124 | new 0 | improved 0 | regressed 0 | unchanged 124]
    change:   [min 0 | med 0 | max 0]
    change %: [min 0.00% | med 0.00% | max 0.00%]

---------------------------------------------------
Successfully saved CSV results to canbench_results.csv

@dsarlis dsarlis marked this pull request as ready for review May 16, 2025 12:19
@dsarlis dsarlis requested a review from maksymar May 16, 2025 12:22
@dsarlis dsarlis requested a review from maksymar May 16, 2025 12:36
@dsarlis
Copy link
Member Author

dsarlis commented May 16, 2025

Detailed results of the change where it can be confirmed that a constant overhead is added due to counting the scopes again, so the regressions are expected. The overhead results in higher % regression on cheap operations like get, and lower % regressions on expensive operations like insert.

@dsarlis dsarlis enabled auto-merge (squash) May 16, 2025 12:39
@dsarlis dsarlis merged commit df2d8fe into main May 16, 2025
10 checks passed
@dsarlis dsarlis deleted the dimitris/fix-feature-name branch May 16, 2025 12:45
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.

2 participants