Battery-Intelligence-Lab / dtw-cpp

DTWC++
https://battery-intelligence-lab.github.io/dtw-cpp/
Other
6 stars 1 forks source link

Memory profiler overhead in benchmarks #18

Closed i64 closed 1 month ago

i64 commented 1 month ago

Hi authors, I am reviewing the code from https://github.com/openjournals/joss-reviews/issues/6881

For other libraries, benchmark code includes a memory profiler (trace malloc), while the authors' library does not include a memory profiler.

When obtaining benchmarks, is the overhead caused by the memory profiler considered?

beckyperriment commented 1 month ago

Hi, thank you for bringing this to our attention. We have partially rerun the benchmarks for DTAIDistance without the memory profiler to check this (commit #a9d6168 ), and there is no significant speed increase when it is removed. There is the expected variation in the smaller clustering problems, which are solved in<10 seconds. For a couple larger clustering problems, as examples:

  1. Chlorine concentration

    • DTW-C++ = 70 seconds
    • DTAIDistance with tracemalloc = 201 seconds
    • DTAIDistance without tracmalloc =182 seconds
  2. cinCECGTorse

    • DTW-C++ = 1104 seconds
    • DTAIDistance with tracemalloc = 1955 seconds
    • DTAIDistance without tracmalloc =2079 seconds

We believe this satisfies concerns over the impact of the memory tracker on benchmarking, but if you would like further clarification, please let us know!

ElektrikAkar commented 1 month ago

In addition to Becky's comment/changes, I have added a remark to the table indicating that the results were obtained in the presence of tracemalloc, which may slightly affect the results. However, I also did not see any significant effect of tracemalloc, probably because we did not collect multiple snapshots.

See 4750f5837be4a97807b2e69cf92e98f66887fcfe for the details.

i64 commented 1 month ago

Hi @ElektrikAkar and @beckyperriment,

Thank you for adding the remark and calculating the tracemallocs overhead