Fix comment in initialize_builtins()

[FrancoGiachetta]

TITLE

Description

This PR fixes the comment initialize_builtins which is not matching the function's actual behavior.

Checklist

• Linked to Github Issue
• Unit tests added
• Integration tests added.
• This change requires new documentation.
  • Documentation has been added/updated.
  • CHANGELOG has been updated.

[github-actions]

**Hyper Thereading Benchmark results**




hyperfine -r 2 -n "hyper_threading_main threads: 1" 'RAYON_NUM_THREADS=1 ./hyper_threading_main' -n "hyper_threading_pr threads: 1" 'RAYON_NUM_THREADS=1 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 1
  Time (mean ± σ):     26.299 s ±  0.016 s    [User: 25.564 s, System: 0.733 s]
  Range (min … max):   26.288 s … 26.310 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 1
  Time (mean ± σ):     26.238 s ±  0.030 s    [User: 25.512 s, System: 0.724 s]
  Range (min … max):   26.216 s … 26.259 s    2 runs
 
Summary
  hyper_threading_pr threads: 1 ran
    1.00 ± 0.00 times faster than hyper_threading_main threads: 1




hyperfine -r 2 -n "hyper_threading_main threads: 2" 'RAYON_NUM_THREADS=2 ./hyper_threading_main' -n "hyper_threading_pr threads: 2" 'RAYON_NUM_THREADS=2 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 2
  Time (mean ± σ):     14.594 s ±  0.021 s    [User: 25.552 s, System: 0.783 s]
  Range (min … max):   14.579 s … 14.608 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 2
  Time (mean ± σ):     14.565 s ±  0.016 s    [User: 25.557 s, System: 0.786 s]
  Range (min … max):   14.553 s … 14.577 s    2 runs
 
Summary
  hyper_threading_pr threads: 2 ran
    1.00 ± 0.00 times faster than hyper_threading_main threads: 2




hyperfine -r 2 -n "hyper_threading_main threads: 4" 'RAYON_NUM_THREADS=4 ./hyper_threading_main' -n "hyper_threading_pr threads: 4" 'RAYON_NUM_THREADS=4 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 4
  Time (mean ± σ):     10.233 s ±  0.004 s    [User: 38.242 s, System: 0.940 s]
  Range (min … max):   10.230 s … 10.235 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 4
  Time (mean ± σ):     10.777 s ±  0.050 s    [User: 37.532 s, System: 0.947 s]
  Range (min … max):   10.742 s … 10.813 s    2 runs
 
Summary
  hyper_threading_main threads: 4 ran
    1.05 ± 0.00 times faster than hyper_threading_pr threads: 4




hyperfine -r 2 -n "hyper_threading_main threads: 6" 'RAYON_NUM_THREADS=6 ./hyper_threading_main' -n "hyper_threading_pr threads: 6" 'RAYON_NUM_THREADS=6 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 6
  Time (mean ± σ):     10.505 s ±  0.136 s    [User: 37.959 s, System: 0.969 s]
  Range (min … max):   10.409 s … 10.601 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 6
  Time (mean ± σ):     10.351 s ±  0.013 s    [User: 38.297 s, System: 0.975 s]
  Range (min … max):   10.341 s … 10.360 s    2 runs
 
Summary
  hyper_threading_pr threads: 6 ran
    1.01 ± 0.01 times faster than hyper_threading_main threads: 6




hyperfine -r 2 -n "hyper_threading_main threads: 8" 'RAYON_NUM_THREADS=8 ./hyper_threading_main' -n "hyper_threading_pr threads: 8" 'RAYON_NUM_THREADS=8 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 8
  Time (mean ± σ):     10.358 s ±  0.067 s    [User: 38.375 s, System: 0.982 s]
  Range (min … max):   10.311 s … 10.405 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 8
  Time (mean ± σ):     10.339 s ±  0.093 s    [User: 38.223 s, System: 0.987 s]
  Range (min … max):   10.274 s … 10.405 s    2 runs
 
Summary
  hyper_threading_pr threads: 8 ran
    1.00 ± 0.01 times faster than hyper_threading_main threads: 8




hyperfine -r 2 -n "hyper_threading_main threads: 16" 'RAYON_NUM_THREADS=16 ./hyper_threading_main' -n "hyper_threading_pr threads: 16" 'RAYON_NUM_THREADS=16 ./hyper_threading_pr'
Benchmark 1: hyper_threading_main threads: 16
  Time (mean ± σ):     10.374 s ±  0.050 s    [User: 38.527 s, System: 1.081 s]
  Range (min … max):   10.339 s … 10.410 s    2 runs
 
Benchmark 2: hyper_threading_pr threads: 16
  Time (mean ± σ):     10.253 s ±  0.008 s    [User: 38.628 s, System: 1.049 s]
  Range (min … max):   10.247 s … 10.258 s    2 runs
 
Summary
  hyper_threading_pr threads: 16 ran
    1.01 ± 0.00 times faster than hyper_threading_main threads: 16

[github-actions]

Benchmark Results for unmodified programs 🚀

Command	Mean [s]	Min [s]	Max [s]	Relative
base big_factorial	2.096 ± 0.007	2.086	2.106	1.00 ± 0.01
head big_factorial	2.095 ± 0.009	2.086	2.115	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base big_fibonacci	2.013 ± 0.004	2.009	2.019	1.00
head big_fibonacci	2.072 ± 0.028	2.050	2.114	1.03 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base blake2s_integration_benchmark	7.560 ± 0.120	7.408	7.684	1.00 ± 0.02
head blake2s_integration_benchmark	7.545 ± 0.100	7.451	7.742	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base compare_arrays_200000	2.165 ± 0.031	2.136	2.227	1.00
head compare_arrays_200000	2.198 ± 0.018	2.171	2.240	1.02 ± 0.02

Command	Mean [s]	Min [s]	Max [s]	Relative
base dict_integration_benchmark	1.444 ± 0.007	1.438	1.458	1.00
head dict_integration_benchmark	1.448 ± 0.017	1.436	1.486	1.00 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base field_arithmetic_get_square_benchmark	1.210 ± 0.011	1.200	1.239	1.00
head field_arithmetic_get_square_benchmark	1.212 ± 0.015	1.198	1.252	1.00 ± 0.02

Command	Mean [s]	Min [s]	Max [s]	Relative
base integration_builtins	7.577 ± 0.157	7.470	7.963	1.01 ± 0.02
head integration_builtins	7.531 ± 0.027	7.478	7.566	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base keccak_integration_benchmark	7.807 ± 0.155	7.721	8.244	1.00 ± 0.02
head keccak_integration_benchmark	7.798 ± 0.085	7.739	8.020	1.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base linear_search	2.129 ± 0.026	2.114	2.201	1.00
head linear_search	2.173 ± 0.014	2.149	2.201	1.02 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base math_cmp_and_pow_integration_benchmark	1.508 ± 0.006	1.500	1.520	1.00
head math_cmp_and_pow_integration_benchmark	1.522 ± 0.004	1.518	1.528	1.01 ± 0.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base math_integration_benchmark	1.457 ± 0.004	1.451	1.462	1.00
head math_integration_benchmark	1.475 ± 0.006	1.468	1.485	1.01 ± 0.00

Command	Mean [s]	Min [s]	Max [s]	Relative
base memory_integration_benchmark	1.205 ± 0.003	1.202	1.209	1.00
head memory_integration_benchmark	1.240 ± 0.018	1.225	1.282	1.03 ± 0.02

Command	Mean [s]	Min [s]	Max [s]	Relative
base operations_with_data_structures_benchmarks	1.569 ± 0.010	1.560	1.593	1.00
head operations_with_data_structures_benchmarks	1.589 ± 0.012	1.580	1.612	1.01 ± 0.01

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base pedersen	530.5 ± 2.4	528.0	534.8	1.00
head pedersen	531.8 ± 1.9	528.6	534.5	1.00 ± 0.01

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base poseidon_integration_benchmark	624.8 ± 2.5	618.3	626.5	1.00
head poseidon_integration_benchmark	630.5 ± 4.9	622.6	636.6	1.01 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base secp_integration_benchmark	1.848 ± 0.008	1.842	1.864	1.00
head secp_integration_benchmark	1.852 ± 0.009	1.845	1.874	1.00 ± 0.01

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
base set_integration_benchmark	623.2 ± 3.7	620.8	632.5	1.00
head set_integration_benchmark	626.2 ± 1.0	624.7	628.2	1.00 ± 0.01

Command	Mean [s]	Min [s]	Max [s]	Relative
base uint256_integration_benchmark	4.175 ± 0.019	4.138	4.193	1.00
head uint256_integration_benchmark	4.180 ± 0.020	4.151	4.199	1.00 ± 0.01

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (7dde1c6) to head (edf4479).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2005   +/-   ##
=======================================
  Coverage   96.53%   96.53%           
=======================================
  Files         102      102           
  Lines       42725    42725           
=======================================
  Hits        41245    41245           
  Misses       1480     1480           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JulianGCalderon]

I don't think there is an error.

When running in proof_mode

• all builtins in the layout are created: this is true
• only those in the program will be included: this is also true, see that the variable included is given to the creation of each builtin.

[FrancoGiachetta]

I wouldn't say the second point is always true. Take this snippet of the function as an example:

if let Some(instance_def) = self.layout.builtins.pedersen.as_ref() {
    let included = program_builtins.remove(&BuiltinName::pedersen);
    if included || self.is_proof_mode() {
        self.vm
        .builtin_runners
        .push(HashBuiltinRunner::new(instance_def.ratio, included).into());
    }
}

and this cairo code:

%builtins output range_check bitwise keccak poseidon range_check96 add_mod mul_mod

from starkware.cairo.common.cairo_builtins import (
    BitwiseBuiltin,
    KeccakBuiltin,
    PoseidonBuiltin,
    ModBuiltin,
    HashBuiltin,
    SignatureBuiltin,
    EcOpBuiltin,
)
func main{
    output_ptr: felt*,
    range_check_ptr,
    bitwise_ptr: BitwiseBuiltin*,
    keccak_ptr: KeccakBuiltin*,
    poseidon_ptr: PoseidonBuiltin*,
    range_check96_ptr: felt*,
    add_mod_ptr: ModBuiltin*,
    mul_mod_ptr: ModBuiltin*,
}() {
}

Running this with proof_mode, the pedersen builtin would get intiliazed (which should be) and also would be included in the builtin_runners list (which should not according to the previous comment) even though it is not declared in the program.
proof_mode will get every builtin in the layout initialized and included in the builtin_runners even if you haven't declared any of those in the program. This is the expected behavior, but the comment is not describing it correctly.

[JulianGCalderon]

Regarding the snippet:

if let Some(instance_def) = self.layout.builtins.pedersen.as_ref() {
    let included = program_builtins.remove(&BuiltinName::pedersen);
    if included || self.is_proof_mode() {
        self.vm
        .builtin_runners
        .push(HashBuiltinRunner::new(instance_def.ratio, included).into());
    }
}

You could add some debug prints just in case, but the runner is added to the builtin_runners list, but with included = false. Calling runner.included() should return false.

[FrancoGiachetta]

Mmm, I agree with you in that included = false, but it does not mean that calling runner.included() would return false. The thing is this condition included || self.is_proof_mode(). If proof_mode is enabled, it does not matter if the builtin is included or not. In other words, it does not matter if have declared it in your cairo program. It wil be included in the builtin_runner. However, the comment says that if it is not included (which means you haven't explicitly declared it in your program) it won't produce any effect, which is not actually what's happening.

You can try running the cairo code in proof mode, and debug the runner.vm.builtin_runner after the vm initialization. You will see the pedersen builtin there which, according to the original comment, it should not.

[JulianGCalderon]

You will see the pedersen builtin there which, according to the original comment, it should not.

The comment doesn't say that the builtin will not be in the builtin list. The comment says that:

• All the builtins will be created. Meaning that all the builtins will be added to the builtin list.
• Only the program builtins will be included. Note that this not means "included in the list of builtin". A builtin is denoted included with a internal flag called included. See HashBuiltinRunner as an example.

In case of pedersen, that included field has a value of false. If you call pedersen_builtin_runner.included() it will return false.

The comment is correct, but if you want you could add a clarification to avoid future misunderstandings.

[FrancoGiachetta]

So perhaps we are having two different understandings of the comment. In that case, i'll add the clarification to the comment.

[FrancoGiachetta]

Change the comment to add a clarification instead.