Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

bench: Use _ instead of :: in auto-generated file names #12332

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Sep 22, 2022

benchmark pallet currently stringifies the pallet path + instance path and uses that as output name.
This does not work when the path contains ::, since the resulting file path should be a valid rust module.
Changes:

  • Replace :: with _ in the auto-generated output path. It does not change the path when it is explicitly specified by the user.
  • Use println! instead of log since that module currently mixes it which causes disordered output.
  • Print a warning when multiple results got written to the same file, as this is most likely a usage error.
  • Put the file creation at the end of the code to not miss any error output.

That should allow us to update these lines in the benchbot and make it possible to benchmark multiple instances with the bot.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 22, 2022
@ggwpez ggwpez requested a review from shawntabrizi September 22, 2022 18:21
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

probably everything should use log instead of println, but not really of a strong opinion

@ggwpez
Copy link
Member Author

ggwpez commented Sep 22, 2022

probably everything should use log instead of println, but not really of a strong opinion

Yes for sure. Just wanted to hot-fix it here and keep the diff compact.

@ggwpez
Copy link
Member Author

ggwpez commented Sep 23, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 397da27 into master Sep 23, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-file-naming-bench-pallet branch September 23, 2022 10:42
ordian added a commit that referenced this pull request Sep 23, 2022
* master:
  [Fix] parameter_types! dead code errors (#12340)
  [Feature] Sequential migration execution for try-runtime (#12319)
  bench: Use `_` instead  of `::` in auto-generated file names (#12332)
  Fast Unstake Pallet (#12129)
  Rename anonymous to pure proxy (#12283)
  Migrate remaining old decl_* macros to the new pallet attribute macros (#12271)
  pallet-utility: Disallow none origin (#12321)
  Make automatic storage deposits resistant against changing deposit prices (#12083)
  Format templates and fix `--steps` default value (#12286)
  Bump `wasmtime` to 1.0.0 (#12317)
  Introduce 'intermediate_insert' method to hide implementation details (#12215)
  Bound staking storage items (#12230)
  Use `array-bytes` for All Array/Bytes/Hex Operations (#12190)
  BREAKING: Rename Origin (#12258)
  Use temporary db for benchmarking (#12254)
  rpc: Implement `chainSpec` RPC API (#12261)
  Import target block body during warp sync (#12300)
  Proper naming wrt expectations (#12311)
  [ci] Revert cancel-pipeline job (#12309)
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ch#12332)

* Replace :: with _ in auto-generated file names

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Development

Successfully merging this pull request may close these issues.

4 participants