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

feat(sozo): in build stats add bytecode and class size of casm class #2004

Merged
merged 10 commits into from
May 29, 2024

Conversation

itzlambda
Copy link

@itzlambda itzlambda commented May 26, 2024

fix: #1952

  • add stats of casm class
  • sort table by contract name

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 93.43066% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 67.54%. Comparing base (6153192) to head (2d2e189).
Report is 1 commits behind head on main.

Files Patch % Lines
crates/sozo/ops/src/statistics.rs 88.46% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2004      +/-   ##
==========================================
+ Coverage   67.50%   67.54%   +0.03%     
==========================================
  Files         318      318              
  Lines       37880    37979      +99     
==========================================
+ Hits        25572    25654      +82     
- Misses      12308    12325      +17     

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

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

thanks @lambda-0x for those fixes! It will be great to have those numbers and also see how the Sierra / casm sizes are evolving.

Some minor comment, but overall it looks great.

@@ -55,19 +93,30 @@ pub fn get_contract_statistics_for_dir(
let contract_name: String =
path.file_stem().context("Error getting file name")?.to_string_lossy().to_string();

// To ignore files like `contract.contract_class.json` or
// `contract.compiled_contract_class.json`
if contract_name.contains('.') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those one are not usually present in a dojo project, but good to keep this in case. 👍

Copy link
Author

Choose a reason for hiding this comment

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

it would cause issue when someone adds sierra = true or casm = true in manifest file manually for any reason, which i did for testing file size of casm class.

get_file_size(&sierra_json_file).context("Error getting file size")?;
let sierra_bytecode_size = get_sierra_byte_code_size(sierra_class);

let casm_contract_class_size = serde_json::to_string(&casm_class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There isn't a way to factorize how the byte size of a file is computed? (for Sierra and casm)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@kariy kariy left a comment

Choose a reason for hiding this comment

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

hmm the table takes a considerable amount of screen real estate to display it without wrapping.

what do you think about compressing the table somehow? like deferring the units in the help message? or if the table lib that we're using can somehow do the header row like so:

+-------------+
|     Col1    |
+-------------+
| Col1 | Col2 |
+------+------+
|    1 |    2 |
|    3 |    4 |
+------+------+

@glihm
Copy link
Collaborator

glihm commented May 27, 2024

hmm the table takes a considerable amount of screen real estate to display it without wrapping.

what do you think about compressing the table somehow? like deferring the units in the help message? or if the table lib that we're using can somehow do the header row like so:

+-------------+
|     Col1    |
+-------------+
| Col1 | Col2 |
+------+------+
|    1 |    2 |
|    3 |    4 |
+------+------+

Good idea, could help to have more concise report. We could have an introductory message before the table to add enough context for users to understand the table and get used to it.

@glihm glihm added the sozo label May 27, 2024
@itzlambda
Copy link
Author

hmm the table takes a considerable amount of screen real estate to display it without wrapping.

what do you think about compressing the table somehow? like deferring the units in the help message? or if the table lib that we're using can somehow do the header row like so:

+-------------+
|     Col1    |
+-------------+
| Col1 | Col2 |
+------+------+
|    1 |    2 |
|    3 |    4 |
+------+------+

the library we are using doesn't support it atm, see: phsym/prettytable-rs#91

wdyt about something like:
image

@kariy
Copy link
Member

kariy commented May 28, 2024

hmm the table takes a considerable amount of screen real estate to display it without wrapping.
what do you think about compressing the table somehow? like deferring the units in the help message? or if the table lib that we're using can somehow do the header row like so:

+-------------+
|     Col1    |
+-------------+
| Col1 | Col2 |
+------+------+
|    1 |    2 |
|    3 |    4 |
+------+------+

the library we are using doesn't support it atm, see: phsym/prettytable-rs#91

wdyt about something like: image

Not bad, but i think should remove the parentheses in the non-header cells.

We could have an introductory message before the table to add enough context for users to understand the table and get used to it.

yeah i think looking at it from the perspective of user that haven't seen the starknet docs, Bytecode size [felts] doesn't exactly explain what unit is felts and what the value it's based on. maybe worth putting some extra messages either embedded in the CLI help message, or displayed along with the table itself.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Works for me for this iteration after addressing the changes. A little detail is that we don't have good alignment with bigger numbers.
image

But it still more compact than before, that's for sure. :)

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Let's go for the first iteration, nice @lambda-0x!

@itzlambda itzlambda merged commit d14bc1c into main May 29, 2024
13 checks passed
@itzlambda itzlambda deleted the improve-stats branch May 29, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sozo] sozo build --stats Bytecode size displayed
3 participants