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

set --disable-stats if stats feature is not enabled #82

Merged
merged 2 commits into from
May 10, 2024

Conversation

girlbossceo
Copy link

jemalloc defaults to enabling stats if --enable-stats is not present and --disable-stats was not set: https://github.com/jemalloc/jemalloc/blob/fa451de17fff73cc03c31ec8cd817d62927d1ff9/configure.ac#L1331-L1333

This issue was observed when we were building with stats and noticed the output of malloc_stats_print was basically the same.

Before:
image

After:
image

jemalloc defaults to enabling stats if `--enable-stats` is not
present and `--disable-stats` was not set: https://github.com/jemalloc/jemalloc/blob/fa451de17fff73cc03c31ec8cd817d62927d1ff9/configure.ac#L1331-L1333

Signed-off-by: strawberry <strawberry@puppygock.gay>
@BusyJay
Copy link
Member

BusyJay commented Apr 29, 2024

It's better to add it in the default feature list in this PR. This is still a breaking change for those rely on stats features without enabling it explicitly though.

the stats feature was always enabled by default and never
respected if someone were to remove the stats feature as
no logic was added for setting `--disable-stats`. to avoid
breaking other user's setups who are relying on this behaviour,
set stats as a default feature.

Signed-off-by: strawberry <strawberry@puppygock.gay>
@girlbossceo
Copy link
Author

@BusyJay Added it to the default features

@girlbossceo
Copy link
Author

@BusyJay Anything blocking this PR from being merged? The CI failure looks to be out of my control, the runner timed out. Everything else still passed.

@BusyJay BusyJay merged commit 86e9248 into tikv:main May 10, 2024
8 of 9 checks passed
@BusyJay
Copy link
Member

BusyJay commented May 10, 2024

Thanks!

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.

2 participants