You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2023/01/10 18:54:15 UTC
[GitHub] [trafficserver] maskit opened a new pull request, #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
maskit opened a new pull request, #9294:
URL: https://github.com/apache/trafficserver/pull/9294
This is a tiny optimization, but it allows us to use History and SourceLocation without worrying about performance at more places.
History and SourceLocation are just for debugging, and the values are not read by the code. Having uninitialized random values in each entry should not affect anything, and a human can see whether an entry is an actual record by checking the count of entries in History.
Client:
```
h2load -n 100000 -c 1 -m 10 -t 1 "https://localhost:8443/8k"
```
Before:
<img width="729" alt="image" src="https://user-images.githubusercontent.com/153144/211635672-6ed9773e-d250-4f97-8d46-aea6c40c05a1.png">
After:
<img width="695" alt="image" src="https://user-images.githubusercontent.com/153144/211635735-9b2d56cb-ed23-41e0-99e4-fc0f469029c5.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#issuecomment-1377813728
approve ci autest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#discussion_r1073995304
##########
include/tscore/SourceLocation.h:
##########
@@ -34,9 +34,9 @@
class SourceLocation
{
public:
- const char *file = nullptr;
- const char *func = nullptr;
- int line = 0;
+ const char *file;
+ const char *func;
+ int line;
SourceLocation() = default;
SourceLocation(const SourceLocation &rhs) = default;
Review Comment:
Unfortunately those causes compile errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#discussion_r1066510815
##########
include/tscore/SourceLocation.h:
##########
@@ -34,9 +34,9 @@
class SourceLocation
{
public:
- const char *file = nullptr;
- const char *func = nullptr;
- int line = 0;
+ const char *file;
+ const char *func;
+ int line;
SourceLocation() = default;
SourceLocation(const SourceLocation &rhs) = default;
Review Comment:
The following indicates we can probably just eliminate the default constructor:
```
wkaras ~/REPOS/TS
O$ grep '[^e]SourceLocation[^,]*[({]' $(findsrc) # | fgrep -v 'MakeSourceLocation('
./include/tscore/Regression.h:#define RegressionMakeLocation(f) SourceLocation(__FILE__, f, __LINE__)
./include/tscore/SourceLocation.h:#define MakeSourceLocation() SourceLocation(__FILE__, __FUNCTION__, __LINE__)
./include/tscore/SourceLocation.h: SourceLocation() = default;
./include/tscore/SourceLocation.h: SourceLocation(const SourceLocation &rhs) = default;
./include/tscore/SourceLocation.h: SourceLocation(const char *_file, const char *_func, int _line) : file(_file), func(_func), line(_line) {}
./include/tscore/Diags.h: static const SourceLocation DiagsError_loc = MakeSourceLocation(); \
./include/tscore/Diags.h: static const SourceLocation STDE_loc = MakeSourceLocation(); \
./include/tscore/Diags.h: static const SourceLocation DiagsErrorV_loc = MakeSourceLocation(); \
./include/tscore/Diags.h: static const SourceLocation STDEV_loc = MakeSourceLocation(); \
./include/tscore/Diags.h: static const SourceLocation Diag_loc = MakeSourceLocation(); \
./include/tscore/Diags.h: static const SourceLocation DbgPrintf_loc = MakeSourceLocation(); \
./iocore/eventsystem/I_Lock.h: m->srcloc = SourceLocation(nullptr, nullptr, 0);
wkaras ~/REPOS/TS
O$ grep '^SourceLocation[^,]*[({]' $(findsrc) # | fgrep -v 'MakeSourceLocation('
./src/tscore/SourceLocation.cc:SourceLocation::str(char *buf, int buflen) const
./src/tscore/SourceLocation.cc:SourceLocation::print(ts::BufferWriter &w, ts::BWFSpec const &) const
wkaras ~/REPOS/TS
O$
```
We can probably also change the data members to:
```
const char * const file;
const char * const func;
const int line;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
maskit merged PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#discussion_r1073995304
##########
include/tscore/SourceLocation.h:
##########
@@ -34,9 +34,9 @@
class SourceLocation
{
public:
- const char *file = nullptr;
- const char *func = nullptr;
- int line = 0;
+ const char *file;
+ const char *func;
+ int line;
SourceLocation() = default;
SourceLocation(const SourceLocation &rhs) = default;
Review Comment:
Unfortunately those cause compile errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#discussion_r1073999175
##########
include/tscore/History.h:
##########
@@ -32,8 +32,8 @@
struct HistoryEntry {
SourceLocation location;
- unsigned short event = 0;
- short reentrancy = 0;
+ unsigned short event;
+ short reentrancy;
Review Comment:
I hope it would not, but we can ignore the issue if it was raised. I'm pretty sure it's safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#discussion_r1066502162
##########
include/tscore/History.h:
##########
@@ -32,8 +32,8 @@
struct HistoryEntry {
SourceLocation location;
- unsigned short event = 0;
- short reentrancy = 0;
+ unsigned short event;
+ short reentrancy;
Review Comment:
I would hope Coverty would not raise an issue with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#issuecomment-1377814135
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9294: Don't initialize the member variables of HistoryEntry and SourceLocation
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #9294:
URL: https://github.com/apache/trafficserver/pull/9294#discussion_r1080648262
##########
include/tscore/SourceLocation.h:
##########
@@ -34,9 +34,9 @@
class SourceLocation
{
public:
- const char *file = nullptr;
- const char *func = nullptr;
- int line = 0;
+ const char *file;
+ const char *func;
+ int line;
SourceLocation() = default;
SourceLocation(const SourceLocation &rhs) = default;
Review Comment:
You're right, sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org