[CI][Benchmark] add UMF benchmarks for fragmentation measurements #17608
Conversation
EuphoricThinking
force-pushed
the
umf_fragmentation
branch
from
55533e6 to
e634182
Compare
|
This will need to wait until #17617 is merged. After that happens, please retarget to sycl branch. |
|
This is missing title tags - |
EuphoricThinking
force-pushed
the
umf_fragmentation
branch
from
e634182 to
32a094a
Compare
EuphoricThinking
changed the title
EuphoricThinking
force-pushed
the
umf_fragmentation
branch
2 times, most recently
from
b2bd7b2 to
979038a
Compare
| class GBench(ComputeUMFBenchmark): | ||
| def __init__(self, bench): | ||
| super().__init__(bench, "umf-benchmark") | ||
| self.num_cols_with_memory = 13 |
There was a problem hiding this comment.
hardcoding column ids is error prone, see example how you can use column names instead:
https://github.com/intel/llvm/blob/sycl/devops/scripts/benchmarks/benches/llamacpp.py#L139-L151
| def __init__(self, bench): | ||
| super().__init__(bench) | ||
|
|
||
| self.is_memory_overhead_checked = True |
There was a problem hiding this comment.
this should be a constructor argument.
| @@ -40,96 +41,44 @@ def benchmarks(self) -> list[Benchmark]: | |||
| GBenchUmfProxy(self), | |||
| GBenchJemalloc(self), | |||
| GBenchTbbProxy(self), | |||
| GBenchMemoryOverhead(self), | |||
There was a problem hiding this comment.
Correct me if I'm wrong, but memory overhead/fragmentation is simply an additional column in existing benchmark scenarios, right?
If that's the case, I don't think you want to add another benchmark since that will run the same scenario twice, and simply extract a different value.
Instead, you probably should return two results from a single benchmark. With different unit and result value.
| statistics = None | ||
| is_row_matched_to_statistics_type = False | ||
|
|
||
| if not self.is_memory_overhead_checked: |
There was a problem hiding this comment.
Please reverse this condition. As a general rule, if possible, avoid negation in conditions. It's easier to read.
if self.is_memory_overhead_checked:
| is_row_matched_to_statistics_type = True | ||
|
|
||
| if is_row_matched_to_statistics_type: | ||
| results.append((config, pool, statistics)) |
There was a problem hiding this comment.
You can simplify this by just appending results twice - once with mean and once with fragmentation. And then you will be able to remove the is_memory_overhead_checked variable and GBenchMemoryOverhead class.
EuphoricThinking
force-pushed
the
umf_fragmentation
branch
2 times, most recently
from
cab6519 to
3894e26
Compare
redesign of the base class for UMF benchmarks run each benchmark separately benchmarks using glibc are excluded from the fragmentation benchmark
EuphoricThinking
force-pushed
the
umf_fragmentation
branch
from
3894e26 to
2d0c3bc
Compare
|
@intel/llvm-gatekeepers please merge. The CI failure is unrelated, see #17562 (comment). |
|
@EuphoricThinking you need to tweak your Github settings as when merged this commit will say authored by there is a CI job that should have caught this, I'll open an issue for it |
I think it should be fine now? |
Beside the new class for fragmentation measurements, ComputeUMFBenchmark base class is directly replaced by GBench. Methods and attributes previously included in ComputeUMFBenchmark have been relevant only for Google Benchmark, therefore inheriting from ComputeUMFBenchmark would have been pointless for benchmarks using other frameworks.
Additionally, each benchmark is run separately in order to avoid any impact of benchmarks on each other. Benchmarks using glibc are excluded from fragmentation measurements.