-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add memory profiling support #103
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
base: master
Are you sure you want to change the base?
Conversation
55eeef1 to
4bd794f
Compare
2351156 to
c6a08d7
Compare
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
| # Manually call the library function to avoid an extra stack frame. Also | ||
| # call the callgrind markers directly to avoid extra overhead. | ||
| self.instrument_hooks.lib.callgrind_start_instrumentation() | ||
| self.instrument_hooks.start_benchmark() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to the original comment here ?
Is the extra stack frame no longer an issue ?
| finally: | ||
| # Ensure instrumentation is stopped even if the test failed | ||
| self.instrument_hooks.lib.callgrind_stop_instrumentation() | ||
| self.instrument_hooks.stop_benchmark() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda same remark here
| f"callgraph: {'enabled' if SUPPORTS_PERF_TRAMPOLINE else 'not supported'}" | ||
| ) | ||
| warnings = [] | ||
| warns = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about this renaming, is there any reason?
| instrument_hooks: InstrumentHooks | None | ||
|
|
||
| def __init__(self, config: CodSpeedConfig) -> None: | ||
| def __init__(self, config: CodSpeedConfig, mode: MeasurementMode) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt this lead to some kind of linter error for unused argument ? Just curious
| - name: Run benchmarks | ||
| uses: CodSpeedHQ/action@main | ||
| with: | ||
| runner-version: branch:cod-2105-support-memory-profiling-for-python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not forget to drop this commit
| sudo apt-get install valgrind -y | ||
| uv sync --dev | ||
| sudo apt-get remove valgrind -y | ||
| - uses: dtolnay/rust-toolchain@stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| def get_instrument_config_str_and_warns(self) -> tuple[str, list[str]]: | ||
| config = ( | ||
| f"instrument: {self.mode.value}, " | ||
| f"mode: {self.mode.value}, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont forget to fixup aswell
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant to request changes
Especially we need to validate the extra stack frame thing in instrumentation by comparing the output to a real prod run!
No description provided.