Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

feat(concurrency): add concurrency flag to block info #1791

Merged

Conversation

meship-starkware
Copy link
Contributor

@meship-starkware meship-starkware commented Apr 15, 2024

This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

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

Project coverage is 75.68%. Comparing base (5488256) to head (c700aaf).

Files Patch % Lines
crates/blockifier/src/context.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1791      +/-   ##
==========================================
- Coverage   75.71%   75.68%   -0.03%     
==========================================
  Files          61       61              
  Lines        8675     8681       +6     
  Branches     8675     8681       +6     
==========================================
+ Hits         6568     6570       +2     
- Misses       1675     1679       +4     
  Partials      432      432              

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

@meship-starkware meship-starkware changed the title concurrency: add concurrency flag to block info feat(concurrency): add concurrency flag to block info Apr 15, 2024
@meship-starkware meship-starkware force-pushed the meship/blockifier/add_concurrency_flag_to_block_info branch from 0285d88 to 7e9149f Compare April 15, 2024 13:22
@meship-starkware meship-starkware force-pushed the meship/blockifier/add_concurrency_flag_to_block_info branch from 7e9149f to c700aaf Compare April 15, 2024 14:13
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

@meship-starkware meship-starkware merged commit 34a802f into main Apr 16, 2024
10 checks passed
@meship-starkware meship-starkware deleted the meship/blockifier/add_concurrency_flag_to_block_info branch April 16, 2024 06:29
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
ref starkware-libs#1369 

Add metrics on katana executor; tracking the total L1 **gas** and Cairo **steps** used.

There were two approaches that i thought of; 
1. record the metrics on every tx execution, or
2. on every block

~Decided to go with (1) as it would allow to measure it in realtime (as the tx is being executed), instead of having to wait until the block is finished being processed.~

Thought im not exactly sure which one is the ideal one.

Doing (1) might be less performant bcs we have to acquire the lock to the metrics recorder more frequently (ie every tx), as opposed to only updating the metrics once every block.

another thing to note, currently doing (1) would require all executor implementations to define the metrics in their own implmentations, meaning have to duplicate code. If do (2) can just define it under `block_producer` scope and be executor agnostic.

EDIT: doing (2). metrics are collected upon completion of block production

---

some changes are made to gather the value after block production:
- simplify params on `backend::do_mine_block`, now only accept two args; `BlockEnv` and `ExecutionOutput`
- add a new type `ExecutionStats` under `katana-executor`, this is where executor would store the gas and steps value
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
…bs#1818)

ref starkware-libs#1791 starkware-libs#1369

<img width="1420" alt="Screenshot 2024-04-12 at 2 36 56 AM" src="https://github.com/dojoengine/dojo/assets/26515232/b97b49df-c5fc-429a-9cb0-bf66138a00b6">

showing total gas and steps using a simple line charts, tracking its growth over time
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants