You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "JosiahWI (via GitHub)" <gi...@apache.org> on 2024/08/13 14:40:19 UTC
[PR] Separate `Server` from `Connection` [trafficserver]
JosiahWI opened a new pull request, #11675:
URL: https://github.com/apache/trafficserver/pull/11675
The only parts of the interface they share are the `close` operation and the `sock` and `addr` fields. They have very little in common and I think the inheritance is kind of forced just for a minor convenience and not serving a useful purpose. To make sure this doesn't develop into a larger inheritance mess, I'd like to separate these now. This also puts `Server` in its own files for better organization and faster compilation.
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#discussion_r1717223708
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -478,10 +478,10 @@ NetAccept::acceptEvent(int event, void *ep)
if ((res = accept_fn(this, e, false)) < 0) {
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
/* INKqa11179 */
- Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res);
+ Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.accept_addr), res);
Review Comment:
I'm still not convinced we need both, but I think changing it to only one variable might require a behavior change.
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#discussion_r1716985242
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -478,10 +478,10 @@ NetAccept::acceptEvent(int event, void *ep)
if ((res = accept_fn(this, e, false)) < 0) {
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
/* INKqa11179 */
- Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res);
+ Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.accept_addr), res);
Review Comment:
Kinda strange, it looks like accept_addr it to specify the bind address, but addr is actually used internally. Is this change required?
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#issuecomment-2286704887
Reserving space on the main JA4 fingerprint string, and resizing it and replacing the correct regions with the subparts, both introduce a large performance degradation. I'm going to reserve space just on the individual parts, where it may permit some optimization of allocations.
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#discussion_r1716998529
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -478,10 +478,10 @@ NetAccept::acceptEvent(int event, void *ep)
if ((res = accept_fn(this, e, false)) < 0) {
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
/* INKqa11179 */
- Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res);
+ Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.accept_addr), res);
Review Comment:
It looks like it uses accept_addr to specify what address/port to bind to, but then it reads the actual bound address into addr. I'm not sure this needs both since it's not inheriting from Connection.
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#issuecomment-2286682093
I tried reserving space in the JA4 string since I know exactly how much it needs.
## Benchmark
```cpp
TEST_CASE("test")
{
BENCHMARK("a bench")
{
JA4::TLSClientHelloSummary TLS_summary;
TLS_summary.protocol = JA4::Protocol::TLS;
CHECK("t" == call_JA4(TLS_summary).substr(0, 1));
};
}
```
## Release build, without reserve:
```
benchmark name samples iterations estimated
mean low mean high mean
std dev low std dev high std dev
-------------------------------------------------------------------------------
a bench 100 382 1.8718 ms
49.5716 ns 49.5445 ns 49.6664 ns
0.231883 ns 0.0710694 ns 0.52879 ns
```
## Release build, with reserve:
```
benchmark name samples iterations estimated
mean low mean high mean
std dev low std dev high std dev
-------------------------------------------------------------------------------
a bench 100 327 1.8966 ms
58.1452 ns 58.1222 ns 58.1835 ns
0.150304 ns 0.103122 ns 0.272601 ns
```
I'm thinking it's best not to call reserve here. I could try resize but I'd have to change the way the pieces of the string are inserted.
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#discussion_r1716992403
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -478,10 +478,10 @@ NetAccept::acceptEvent(int event, void *ep)
if ((res = accept_fn(this, e, false)) < 0) {
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
/* INKqa11179 */
- Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res);
+ Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.accept_addr), res);
Review Comment:
This needs more investigation - I'm confused about why there are two variables and what the difference is.
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675#discussion_r1717223014
##########
src/iocore/net/UnixNetAccept.cc:
##########
@@ -478,10 +478,10 @@ NetAccept::acceptEvent(int event, void *ep)
if ((res = accept_fn(this, e, false)) < 0) {
Metrics::Gauge::decrement(net_rsb.accepts_currently_open);
/* INKqa11179 */
- Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.addr), res);
+ Warning("Accept on port %d failed with error no %d", ats_ip_port_host_order(&server.accept_addr), res);
Review Comment:
These aren't interchangeable, because the address can be set to something different if `accept_addr` is not an IP:
```cpp
int
Server::listen(bool non_blocking, const NetProcessor::AcceptOptions &opt)
{
ink_assert(!sock.is_ok());
int res = 0;
socklen_t namelen;
int prot = IPPROTO_TCP;
if (!ats_is_ip(&accept_addr)) {
ats_ip4_set(&addr, INADDR_ANY, 0);
} else {
ats_ip_copy(&addr, &accept_addr);
}
```
--
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
Re: [PR] Separate `Server` from `Connection` [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI merged PR #11675:
URL: https://github.com/apache/trafficserver/pull/11675
--
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