Skip to content

Conversation

@marcdumais-work
Copy link
Contributor

What it does

The benchmarks were showing slow results for AsyncFileHandler, much slower compared to what they were until v0.4.0. The reason is that a couple of bugs were recently fixed in that handler, that resulted in the following configurable property, used for the benchmark tests, to not have the desired effect:

org.eclipse.tracecompass.traceeventlogger.AsyncFileHandler.maxSize = 1000

After the bugs were fixed, AsyncFileHandler started to correctly use "maxSize" as the size of its record buffer. However, a buffer of 1000 is not enough for the logging load generated by the benchmark tests, making the new async handler look bad.

Setting "bufferSize" to value 100k makes it big enough to "absorb" the logging load of the benchmark tests well, and brings back the previous level of performance for the async handler, such that the benchmark results are comparable to those from until v0.4.0.

To detect such a problem in the future, the benchmark tests now compute the relative performance of the async file handler vs the old sync handler and fails the test if the former is not performing above a certain threshold. The threshold may need to be lowered a bit if e.g. it's not met once in a while during CI (there is variation in the results)

In the benchmark results printed on STDOUT, the relative performance of the async file handlers now displayed as an extra column in the benchmark result tables.

Also, the benchmark test results are now printed in two versions: one un-formatted in "csv" format that's suitable to import e.g. in LibreOffice Calc, and another that's formatted for human readability.

How to test

  • make sure CI passes. Since the benchmark results vary quite a bit, it would be a good idea to restart CI a few times to confirm
  • testing locally, you can roll-back the value of property org.eclipse.tracecompass.traceeventlogger.AsyncFileHandler.maxSize, used in the benchmark tests, to 1000. This should decrease the performance for newAsync making it close to oldSync and cause the benchmark tests to fail.

Follow-ups

N/A

Review checklist

  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

The benchmarks were showing slow results for AsyncFileHandler,much
slower compared to what they were until v0.4.0. The reason is that
a couple of bugs were recently fixed in that handler, that resulted
in the following configurable property, used for the benchmark tests,
to not have the desired effect:

org.eclipse.tracecompass.traceeventlogger.AsyncFileHandler.maxSize = 1000

After the bugs were fixed, AsyncFileHandler started to correctly use
"maxSize" as the size of its record buffer. However, a buffer of 1000
is not enough for the logging load generated by the benchmark tests,
making the new async handler look bad.

Setting "bufferSize" to value 100k makes it big enough to "absorb" the
logging load of the benchmark tests well, and brings back the previous
level of performance for the async handler, such that the benchmark
results are comparable to those from until v0.4.0.

To detect such a problem in the future, the benchmark tests now compute
the relative performance of the async file handler vs the old sync
handler and fails the test if the former is not performing above a
certain threshold. The threshold may need to be lowered a bit if e.g.
it's not met once in a while during CI (there is variation in the
results)

In the benchmark results printed on STDOUT, the relative performance of
the async file handlers now displayed as an extra column in the
benchmark result tables.

Also, the benchmark results are now printed in two versions: one un-
formatted in "csv" format that's suitable to import e.g. in LibreOffice
Calc, and another that's formatted for human readability.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
Copy link
Contributor

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@MatthewKhouzam MatthewKhouzam merged commit ecd78da into eclipse-tracecompass:main Apr 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants