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

[DeviceSanitizer] Memory overhead statistics #1869

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Jul 17, 2024

LLVM: intel/llvm#14592

Statistic memory overhead (usm redzone + shadow memory) on each context, enabled by exporting asan flag UR_LAYER_ASAN_OPTIONS=print_stats:1.

Addtionally, move "AsanOptions" under "SanitizerInterceptor".

@AllanZyne AllanZyne requested a review from a team as a code owner July 17, 2024 03:24
@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Jul 17, 2024
@AllanZyne AllanZyne marked this pull request as draft July 17, 2024 03:24
@AllanZyne AllanZyne marked this pull request as ready for review August 28, 2024 08:57
@yingcong-wu
Copy link
Contributor

yingcong-wu commented Sep 5, 2024

It occurs to me that this only measures the redzone overhead, I think we should also mean the total overhead including shadow memory and other usages. Maybe we can implement a thin layer that keeps track of all memory allocation and compares the data with and without the sanitizer layer. That way we can have the overall memory overhead data instead of only the redzone overhead.

@AllanZyne
Copy link
Contributor Author

It occurs to me that this only measures the redzone overhead, I think we should also mean the total overhead including shadow memory and other usages.

No, actually it includes shadow memory.

@AllanZyne
Copy link
Contributor Author

Hi @pbalcer @kswiecicki, please review. Thanks!

source/loader/layers/sanitizer/asan_options.cpp Outdated Show resolved Hide resolved
@AllanZyne
Copy link
Contributor Author

Hi @pbalcer, I think the failed CIs are unrelated, could you please double check this? Thanks!

@pbalcer
Copy link
Contributor

pbalcer commented Sep 16, 2024

@AllanZyne they are indeed. I'll mark it ready to merge after you get approval from @yingcong-wu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a idea and a nit, can we create a warpper to check PrintStats, and avoid (as much as we can) the overhead of this statistics when we not enable the tracking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label Sep 18, 2024
@pbalcer
Copy link
Contributor

pbalcer commented Sep 19, 2024

I resolved a conflict and rebased the PR.

... but it didn't work :-) please rebase and update the PR. Thanks.

@AllanZyne
Copy link
Contributor Author

I resolved a conflict and rebased the PR.

... but it didn't work :-) please rebase and update the PR. Thanks.

No problem. Done.

@aarongreig
Copy link
Contributor

looking to merge this soon, are @zhaomaosu and @kswiecicki still required reviewers?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loader Loader related feature/bug ready to merge Added to PR's which are ready to merge sanitizer Sanitizer layer issues/changes/specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants