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

Memory Optimization - ICE Agent Stats (#1947) #2074

Merged
merged 17 commits into from
Nov 11, 2024

Conversation

stefankiesz
Copy link
Contributor

@stefankiesz stefankiesz commented Nov 1, 2024

Issue #, if available:

What was changed?

  • Ice agent stats are now back to being enabled by default.
  • Added ENABLE_STATS_CALCULATION_CONTROL CMake option to support enabling/disabling stats at runtime.
  • Added unit tests to test these new paths.
  • Updated ReadMe to reflect the new changes.

Why was it changed?

  • Leaving stats calculations to be on by default for backwards-compatibility purposes.
  • To ensure the new enableIceStats value is used as intended, a compile-time flag is required to enable its use for run-time control over enabling/disabling stats.
    • This is to avoid the risk of using garbage value for enableIceStats in the case that it was not initialized.
    • This will keep the SDK backwards-compatible with applications.

How was it changed?

  • #ifdef's were added to check for the ENABLE_STATS_CALCULATION_CONTROL flag before using the enableIceStats value.

What testing was done for the changes?

  • Allowing the new unit tests to pass in the CI.
  • Locally testing all 4 possible paths:
    1. ENABLE_STATS_CALCULATION_CONTROL=FALSE and enableIceStats=TRUE
    2. ENABLE_STATS_CALCULATION_CONTROL=FALSE and enableIceStats=FALSE
    3. ENABLE_STATS_CALCULATION_CONTROL=TRUE and enableIceStats=TRUE
    4. ENABLE_STATS_CALCULATION_CONTROL=TRUE and enableIceStats=FALSE

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

disa6302 and others added 5 commits November 4, 2024 17:05
* Change params size

* Use dyanmic allocation and flag for ice stats

* Debug 1

* Revert "Debug 1"

This reverts commit ad7d02f.

* Revert "Use dyanmic allocation and flag for ice stats"

This reverts commit bf9a2ee.

* Working version

* enable flag in samples

* Add unit test

* Fix bug

* README for the flag

* Update readme
@stefankiesz stefankiesz force-pushed the ice-memory-reduction-sk branch from 614099b to 81d87b2 Compare November 5, 2024 01:08
@stefankiesz stefankiesz changed the title Ice memory reduction - enable ice stats flag (#1947) Memory reduction - ICE Agent Stats (#1947) Nov 5, 2024
@stefankiesz stefankiesz changed the title Memory reduction - ICE Agent Stats (#1947) Memory Reduction - ICE Agent Stats (#1947) Nov 5, 2024
@stefankiesz stefankiesz marked this pull request as ready for review November 5, 2024 01:38
@stefankiesz stefankiesz changed the title Memory Reduction - ICE Agent Stats (#1947) Memory Optimization - ICE Agent Stats (#1947) Nov 5, 2024
src/source/Ice/IceAgent.c Outdated Show resolved Hide resolved
src/source/Ice/IceAgent.c Outdated Show resolved Hide resolved
src/source/Ice/IceAgent.c Outdated Show resolved Hide resolved
src/source/Metrics/Metrics.c Outdated Show resolved Hide resolved
src/source/Metrics/Metrics.c Outdated Show resolved Hide resolved
tst/MetricsApiTest.cpp Show resolved Hide resolved
src/source/Ice/IceAgent.c Outdated Show resolved Hide resolved
src/source/Metrics/Metrics.c Show resolved Hide resolved
src/source/Ice/IceAgent.c Outdated Show resolved Hide resolved
@stefankiesz stefankiesz requested a review from unicornss November 8, 2024 20:53
@stefankiesz stefankiesz merged commit 2f1a8af into develop Nov 11, 2024
27 checks passed
@stefankiesz stefankiesz deleted the ice-memory-reduction-sk branch November 11, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants