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

Nit enhance 25 03 05 #2911

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SamYuan1990
Copy link

What problem does this PR solve?

Issue Number:
n/A

Problem Summary:
A nit fix using const instead of static new, I am not C++ expert, but it seems we just have static std::string* with a fix value in previous implications of this PR. I suppose:

  • static string with a fix value equals with const, as 1st commit.
  • After 1st commit of this PR, I notice that if a function just returns a const string, in fact, we can use a const string replace.

as I am not C++ expert, I used another 3 commits to fix the compile issue.

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects(性能影响):
    I suppose those implements either previous impl for this PR or changes in this PR moves to compiler optimization.
    I searched and learned through www, I suppose there only difference between the memory location for static(previous) and const(this PR), but compiler optimization is out of my knowledge, as previous adding additional function stack....

  • Breaking backward compatibility(向后兼容性):


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

Signed-off-by: SamYuan1990 <[email protected]>
Signed-off-by: SamYuan1990 <[email protected]>
Signed-off-by: SamYuan1990 <[email protected]>
Signed-off-by: SamYuan1990 <[email protected]>
Signed-off-by: SamYuan1990 <[email protected]>
@SamYuan1990
Copy link
Author

SamYuan1990 commented Mar 6, 2025

the test passed on https://github.com/SamYuan1990/brpc/actions/runs/13688832022/job/38278130143?pr=33 let me test with 22.04.

@SamYuan1990
Copy link
Author

https://github.com/SamYuan1990/brpc/actions/runs/13689813245/job/38280860800?pr=34 tested my local github action, it works.

@SamYuan1990 SamYuan1990 marked this pull request as ready for review March 6, 2025 02:47
@SamYuan1990
Copy link
Author

ok, test passed, waiting for maintainer's review and feedback.

@wwbmmm
Copy link
Contributor

wwbmmm commented Mar 6, 2025

LGTM

@zhangqiongyu
Copy link

Perhaps the original author wanted to use the lazy initialization mechanism to reduce memory usage.

@zhangqiongyu
Copy link

zhangqiongyu commented Mar 7, 2025

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
    static const std::string s = "unlimited";
    return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
    static const std::string s = "constant";
    return s;
}

1.This way of initialization is much more common,uncontrolled use of global variables may cause dependency order problems(UB).
2.These short strings can be optimized to be stored on the stack using the SSO(Small String Optimization) mechanism of std::string.
3.The original lazy initialization mechanism is retained to reduce unnecessary memory usage.
4.It is thread-safe.

@SamYuan1990
Copy link
Author

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
    static const std::string s = "unlimited";
    return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
    static const std::string s = "constant";
    return s;
}

1.This way of initialization is much more common,uncontrolled use of global variables may cause dependency order problems(UB). 2.These short strings can be optimized to be stored on the stack using the SSO(Small String Optimization) mechanism of std::string. 3.The original lazy initialization mechanism is retained to reduce unnecessary memory usage. 4.It is thread-safe.

Hi @zhangqiongyu ,

I hope you're doing well! I wanted to check if this is a maintainer change request, as it doesn't seem to be in a CR. I also tried searching via this link but didn't find anything relevant.

I'm relatively new to C++, and I'm here to learn from everyone. If I make any mistakes, please feel free to correct me—I really appreciate it!

I believe we both agree on using const std::string instead of static std::string* s = new to ensure thread safety. However, I'm curious about whether we should use lazy initialization or not. When would be the best time to initialize this variable?

Initially, I considered using the following approach:

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
static const std::string s = "constant";
return s;
}

But when I realized that the function simply returns a constant string, I thought about making a broader change.

From what I understand, the differences are:

  • Defining a const in a class affects the scope of the class and its instances (and possibly all child classes and their instances). It’s unique across all instances.
    
    Regarding memory usage:
    As far as I know, and as mentioned in the PR description, compiler optimization is a bit beyond my current knowledge. Since it's a short string, it might benefit from SSO (Short String Optimization). However, I'm not entirely sure how the compiler will handle it.
    
    I’m aware of the concept of dependency order problems (UB), but I lack practical experience in C++. Could you provide an example in this context?
    
    Regarding performance, when invoking this value from other parts of the code, previously it involved a function call. After changing it to a const, it becomes a direct access to a constant. From my understanding, if the compiler optimizes a function call that returns a static value, it should be equivalent to accessing a constant. If the function isn't loaded onto the stack, it might involve a page change/reload. If SSO is in play, the string would be on the stack in either case (function call or const).
    
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}


I’d love to hear your thoughts on this! Thank you for your time and guidance. 😊

@zhangqiongyu
Copy link

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
    static const std::string s = "unlimited";
    return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
    static const std::string s = "constant";
    return s;
}

1.This way of initialization is much more common,uncontrolled use of global variables may cause dependency order problems(UB). 2.These short strings can be optimized to be stored on the stack using the SSO(Small String Optimization) mechanism of std::string. 3.The original lazy initialization mechanism is retained to reduce unnecessary memory usage. 4.It is thread-safe.

Hi @zhangqiongyu ,  I hope you're doing well! I wanted to check if this is a maintainer change request, as it doesn't seem to be in a CR. I also tried searching via this link but didn't find anything relevant.  I'm relatively new to C++, and I'm here to learn from everyone. If I make any mistakes, please feel free to correct me—I really appreciate it!  I believe we both agree on using const std::string instead of static std::string* s = new to ensure thread safety. However, I'm curious about whether we should use lazy initialization or not. When would be the best time to initialize this variable?  Initially, I considered using the following approach:

const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}

const std::string& AdaptiveMaxConcurrency::CONSTANT() {
static const std::string s = "constant";
return s;
}

But when I realized that the function simply returns a constant string, I thought about making a broader change.  From what I understand, the differences are:

  • Defining a const in a class affects the scope of the class and its instances (and possibly all child classes and their instances). It’s unique across all instances.
    
    Regarding memory usage:
    As far as I know, and as mentioned in the PR description, compiler optimization is a bit beyond my current knowledge. Since it's a short string, it might benefit from SSO (Short String Optimization). However, I'm not entirely sure how the compiler will handle it.
    
    I’m aware of the concept of dependency order problems (UB), but I lack practical experience in C++. Could you provide an example in this context?
    
    Regarding performance, when invoking this value from other parts of the code, previously it involved a function call. After changing it to a const, it becomes a direct access to a constant. From my understanding, if the compiler optimizes a function call that returns a static value, it should be equivalent to accessing a constant. If the function isn't loaded onto the stack, it might involve a page change/reload. If SSO is in play, the string would be on the stack in either case (function call or const).
const std::string& AdaptiveMaxConcurrency::UNLIMITED() {
static const std::string s = "unlimited";
return s;
}

 I’d love to hear your thoughts on this! Thank you for your time and guidance. 😊

Hi,It's a pleasure to have a friendly discussion with you.
1.Google's programming standards clearly state that global variables should be used with caution.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

2.Dependency order problems (UB). The snippet below is nothing todo with brpc, but what if someone wants to use this global variable in the future?
File1.cpp

#include <iostream>

// Declaration of the global variable in another file
extern int g_var2;

int g_var1 = g_var2 + 1; // Depends on the initialization order of g_var2

void func1() {
    std::cout << "func1: g_var1 = " << g_var1 << std::endl;
}

File2.cpp

#include <iostream>

// Declaration of the global variable in another file
extern int g_var1;

int g_var2 = g_var1 + 1; // Depends on the initialization order of g_var1

void func2() {
    std::cout << "func2: g_var2 = " << g_var2 << std::endl;
}

int main() {
    func1();
    func2();
    return 0;
}

3.About memory usage, lazy initialization is achieved by using functions to return static local variables, which are initialized only when needed, saving unnecessary resource overhead.
4.About performance, I have the same view with you.

In my usual projects, I prefer to use functions. In fact, I have done a lot of performance profiling, and this method has never become a performance bottleneck. Maybe what we are discussing is just some programming habits and trade-offs, but I don't think this will hinder the inclusion of PRs.

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.

3 participants