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

Remove local static string #8103

Closed
wants to merge 1 commit into from

Conversation

burtonli
Copy link
Contributor

Local static string is not friendly to Jemalloc arena aware implementation, as it will be allocated on the arena of the first caller, which causes crash if the allocated arena gets refunded earlier.

P.S. A Jemalloc arena aware implementation is each rocksdb instance only use certain Jemalloc arenas, and arena will be refunded after associated DB instance is destroyed.

@ajkr
Copy link
Contributor

ajkr commented Mar 24, 2021

To support this use case, it sounds like we would have to disallow local static variables that allocate memory. It sounds like an odd restriction - is this correct, and is there another way?

@burtonli
Copy link
Contributor Author

To support this use case, it sounds like we would have to disallow local static variables that allocate memory. It sounds like an odd restriction - is this correct, and is there another way?

It's ok to have local static variable without heap memory allocation, and it's also ok to have local static variables in global instance, e.g. those singletons like the implementation for Env::Default(), SystemClock::Default(), so that user can explicitly call the constructor from the environment arena (the one last as long as the process).
I agree we may not want to add the restriction to prohibit local static var, but I think the removed ones in the PR are not necessary.

@siying siying requested a review from ltamasi July 29, 2022 19:36
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @burtonli !

@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants