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

[LibOS] Fix memmgr free list performance bug #2052

Merged

Conversation

adrianlut
Copy link
Contributor

@adrianlut adrianlut commented Nov 7, 2024

[LibOS] Fix memmgr free list performance bug

Fixes the memmgr performance bug described in #2033 by changing the CHECK_LIST_HEAD macro in common/include/list.h in release mode to (void)0.

Description of the changes

To fix the issue described in #2033, this PR surrounds the CHECK_LIST_HEAD macro defined in
common/include/list.h
with #ifdef DEBUG. In release mode, the macro is now replaced with (void)0.

This macro is currently only used in memmgr.h. Thus, changing it directly was the simplest fix of the performance bug.

Fixes #2033

How to test this PR?


This change is Reviewable

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adrianlut)


-- commits line 4 at r1:
Please wrap (see https://commit.style/).


-- commits line 4 at r1:
We don't reference GitHub issues in the commits, we try to keep the git repo standalone. One reason for that is that this will turn into a dangling pointer if we'd move the repository somewhere else.

Code quote:

#2033

@mkow
Copy link
Member

mkow commented Nov 7, 2024

Jenkins, test this please

@adrianlut adrianlut force-pushed the adrianlut/memmgr-free-list-perf-fix branch from 4f42e8d to b44d41b Compare November 7, 2024 12:59
Copy link
Contributor Author

@adrianlut adrianlut left a comment

Choose a reason for hiding this comment

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

Thank you for the review. I changed the commit message as requested.

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)


-- commits line 4 at r1:

Previously, mkow (Michał Kowalczyk) wrote…

Please wrap (see https://commit.style/).

Done.


-- commits line 4 at r1:

Previously, mkow (Michał Kowalczyk) wrote…

We don't reference GitHub issues in the commits, we try to keep the git repo standalone. One reason for that is that this will turn into a dangling pointer if we'd move the repository somewhere else.

Done.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

@mkow
Copy link
Member

mkow commented Nov 7, 2024

Jenkins, retest this please

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @adrianlut)


-- commits line 5 at r2:
Can be fixed during rebase -- I suggest to briefly mention the issue and add more back quotes, e.g., sth like:

The `CHECK_LIST_HEAD` macro in `common/include/list.h` is used in memmgr
to traverse the free list and perform assertion checks. However, the
loop is not removed by the compiler even in release mode, leading to
unnecessary overhead. This commit fixes this by changing the macro to
`(void)0` in release mode.

Code quote:

  Fixes a memmgr performance bug by changing the CHECK_LIST_HEAD macro in
  common/include/list.h in release mode to `(void)0`.

The `CHECK_LIST_HEAD` macro in `common/include/list.h` is used in memmgr
to traverse the free list and perform assertion checks. However, the
loop is not removed by the compiler even in release mode, leading to
unnecessary overhead. This commit fixes this by changing the macro to
`(void)0` in release mode.

Signed-off-by: Adrian Lutsch <adrian.lutsch@cs.tu-darmstadt.de>
@mkow mkow force-pushed the adrianlut/memmgr-free-list-perf-fix branch from b44d41b to 0997103 Compare November 12, 2024 12:41
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


-- commits line 5 at r2:

Previously, kailun-qin (Kailun Qin) wrote…

Can be fixed during rebase -- I suggest to briefly mention the issue and add more back quotes, e.g., sth like:

The `CHECK_LIST_HEAD` macro in `common/include/list.h` is used in memmgr
to traverse the free list and perform assertion checks. However, the
loop is not removed by the compiler even in release mode, leading to
unnecessary overhead. This commit fixes this by changing the macro to
`(void)0` in release mode.

Done.

@mkow
Copy link
Member

mkow commented Nov 12, 2024

Jenkins, retest this please

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow
Copy link
Member

mkow commented Nov 14, 2024

Jenkins, retest this please (a lot of connectivity errors on Jenkins, e.g. Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@258ebcb3:penguins-3-noble": Remote call on penguins-3-noble failed. The channel is closing down or has closed down)

@mkow
Copy link
Member

mkow commented Nov 14, 2024

Jenkins, retest Jenkins-Direct-24.04 please (connectivity issues in CI again)

@mkow
Copy link
Member

mkow commented Nov 14, 2024

Jenkins, retest Jenkins-SGX-24.04-musl please (connectivity issues in CI again)

@mkow mkow merged commit 0997103 into gramineproject:master Nov 14, 2024
26 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.

memmgr performance bug caused by high number of free vmas
3 participants