Skip to content

feat: track atomic memory access size#116

Merged
ggeier merged 2 commits into
open-s4c:mainfrom
mariaxarisi:issue49
Jul 10, 2026
Merged

feat: track atomic memory access size#116
ggeier merged 2 commits into
open-s4c:mainfrom
mariaxarisi:issue49

Conversation

@mariaxarisi

Copy link
Copy Markdown
Contributor

This PR introduces the ability to trace the memory access size for atomic operations.

What Changed:

  • Added logic to record the byte size of atomic read and write operations.
  • Added the coldtrace_atomic_access_entry structure to correctly save the size data.
  • Testing: Added a new test (trace_aread_awrite_size) to verify that atomic sizes are captured correctly.

Related Issue:

@ggeier ggeier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please check the value placeholders comment, besides that, great addition!

Comment thread test/trace_aread_awrite_size.cpp
@ggeier ggeier requested a review from db7 June 19, 2026 14:43

@db7 db7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but the pipeline is failing. Why is that?

@ggeier

ggeier commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

LGTM, but the pipeline is failing. Why is that?

The benchmark workflow currently fails due to runner flakyness. #117 is should improve this, so we can merge it first. Then the pipeline should be green after rebasing

@mariaxarisi

mariaxarisi commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

The scratchapixel benchmark keeps failing with a ~20% slowdown on Ubuntu 22.04 / GNU 11.4.0. We tried reproducing it locally to fix it, but it only seems to happen on the GitHub runners. Is it still ok to merge?

@db7

db7 commented Jul 6, 2026

Copy link
Copy Markdown
Member

Is it still ok to merge?

No idea. @ggeier

@ggeier

ggeier commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

I'd be ok with it. I can't override the check though.

@db7

db7 commented Jul 8, 2026

Copy link
Copy Markdown
Member

I'd be ok with it. I can't override the check though.

I’d remove that benchmark from the cicd rather than changing rules permanently.

@ggeier

ggeier commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

changing rules permanently

I thought maybe there’s a way for you to override it just once. The baseline would be lower afterwards, so I think it would not be a permanent problem.
Removing the benchmark from the ci is also permanent. I think it should be possible to classify it as not required. Then it would still run, but not be necessary to pass.
However, I can’t see or change any of these rules with my permissions.

@db7

db7 commented Jul 9, 2026

Copy link
Copy Markdown
Member

Ok, let’s do that. Just fix the merge conflict first.

Introduces a new `coldtrace_atomic_access_entry` struct to record the
exact byte size of atomic reads and writes in the binary trace files.

- Extends atomic read/write trace entries from 16 to 24 bytes to include `size`.
- Modifies TSan subscriber macros in `subs_tsan.c` to extract and serialize the size parameter.
- Updates the `trace_dump.py` tool to correctly unpack the new 24-byte format and print the access size.

Signed-off-by: mariaxarisi <maria.charisi@huawei.com>
Signed-off-by: mariaxarisi <maria.charisi@huawei.com>
@ggeier ggeier merged commit 3769041 into open-s4c:main Jul 10, 2026
21 of 22 checks passed
@ggeier

ggeier commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Sorry, I didn’t realize the merge was blocked by a merge conflict 🫣. This UI is a bit confusing for me in that regard. Hope I’ll remember next time.

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.

add size for atomic reads and write to trace

3 participants