You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "maskit (via GitHub)" <gi...@apache.org> on 2023/03/01 22:29:33 UTC
[GitHub] [trafficserver] maskit opened a new pull request, #9482: Add interface for NetVC services
maskit opened a new pull request, #9482:
URL: https://github.com/apache/trafficserver/pull/9482
TLSomethingSupport, which enables using SSLNetVC and QUICNetVC in the same way, required use of dynamic_cast at many places. I don't know how much it affects performance, but this is one of ways to remove the dynamic_cast.
It still relies on RTTI, so maybe it's not as fast as you want, but I guess it's faster than looking up vtable.
We probably want to avoid std::unordered_map here, and I'm going to change it later if we go this way. Current code is just to show the idea.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1458452628
> To keep changes minimal, I suggest this pattern: https://godbolt.org/z/E86E4zxGs .
I'd say it's a worse version of `is_feature_x_supprted`. It has the same cons, plus it also requires forward declarations.
I'm happy to make big changes if we can solve this cleanly.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608373189
None of the dynamic_casts need casting from a base class to another base class.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1251376986
##########
iocore/net/I_NetVConnection.h:
##########
@@ -857,7 +857,20 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
bool has_proxy_protocol(IOBufferReader *);
bool has_proxy_protocol(char *, int64_t *);
+ template <typename S> S *get_service() const;
+
Review Comment:
I made the change as suggested but actually I don't like either place. Although forward declaration does not bring dependency, the class names have to be on this header file. Now the header file know of the concrete classes. This is the reason I didn't have these helper functions.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1563114660
[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 pull request #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1564646201
[approve ci autest]
polite_hook_wait failed.
--
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] jpeach commented on a diff in pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1244726602
##########
iocore/net/I_NetVConnection.h:
##########
@@ -876,6 +889,14 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
int write_buffer_empty_event = 0;
/// NetVConnection Context.
NetVConnectionContext_t netvc_context = NET_VCONNECTION_UNSET;
+
+ void _set_service(enum Service service, void *instance);
+ void *_services[static_cast<unsigned int>(Service::N_SERVICES)] = {
Review Comment:
`_services` can be private too.
##########
proxy/http/HttpSM.cc:
##########
@@ -1821,10 +1821,10 @@ PoolableSession *
HttpSM::create_server_session(NetVConnection *netvc, MIOBuffer *netvc_read_buffer, IOBufferReader *netvc_reader)
{
// Figure out what protocol was negotiated
- int proto_index = SessionProtocolNameRegistry::INVALID;
- auto const *sslnetvc = dynamic_cast<ALPNSupport *>(netvc);
- if (sslnetvc) {
- proto_index = sslnetvc->get_negotiated_protocol_id();
+ int proto_index = SessionProtocolNameRegistry::INVALID;
+ auto const *alpn = netvc->get_service<ALPNSupport>();
+ if (alpn) {
+ proto_index = alpn->get_negotiated_protocol_id();
}
Review Comment:
```suggestion
int proto_index = SessionProtocolNameRegistry::INVALID;
if (auto const *alpn = netvc->get_service<ALPNSupport>(); alpn) {
proto_index = alpn->get_negotiated_protocol_id();
}
```
##########
proxy/http/HttpSM.cc:
##########
@@ -1513,7 +1513,7 @@ plugins required to work with sni_routing.
t_state.hdr_info.client_request.url_set(&u);
NetVConnection *netvc = ua_txn->get_netvc();
- auto *tts = dynamic_cast<TLSTunnelSupport *>(netvc);
+ auto *tts = netvc->get_service<TLSTunnelSupport>();
if (tts && tts->has_tunnel_destination()) {
auto tunnel_host = tts->get_tunnel_host();
Review Comment:
```suggestion
if (auto *tts = netvc->get_service<TLSTunnelSupport>(); tts) { : TLSTunnelSupport *
if (tts->has_tunnel_destination()) {
auto tunnel_host = tts->get_tunnel_host(); : std::string_view
t_state.hdr_info.client_request.url_get()->host_set(tunnel_host.data(), tunnel_host.size()); value: length:
t_state.hdr_info.client_request.url_get()->port_set((tts->get_tunnel_port() > 0) ? tts->get_tunnel_port() : port:
netvc->get_local_port());
} else {
t_state.hdr_info.client_request.url_get()->host_set(netvc->get_server_name(), strlen(netvc->get_server_name())); value: length: s:
t_state.hdr_info.client_request.url_get()->port_set(netvc->get_local_port()); port:
}
}
```
Not really related to your change, but I think that this makes the whole TLSTunnelSuport block logic to read.
##########
proxy/http/HttpSM.cc:
##########
@@ -6595,12 +6595,12 @@ HttpSM::attach_server_session()
UnixNetVConnection *server_vc = static_cast<UnixNetVConnection *>(server_txn->get_netvc());
// set flag for server session is SSL
- TLSBasicSupport *tbs = dynamic_cast<TLSBasicSupport *>(server_vc);
+ TLSBasicSupport *tbs = server_vc->get_service<TLSBasicSupport>();
if (tbs) {
server_connection_is_ssl = true;
}
- if (auto tsrs = dynamic_cast<TLSSessionResumptionSupport *>(server_vc)) {
+ if (auto tsrs = server_vc->get_service<TLSSessionResumptionSupport>()) {
Review Comment:
Huh, c++ has so many ways to say the same thing! 👍
##########
iocore/net/I_NetVConnection.h:
##########
@@ -857,7 +857,20 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
bool has_proxy_protocol(IOBufferReader *);
bool has_proxy_protocol(char *, int64_t *);
+ template <typename S> S *get_service() const;
+
Review Comment:
```suggestion
template <> ALPNSupport * get_service() const {
return static_cast<ALPNSupport *>(this->_get_service(Service::TLS_ALPN));
}
template <> TLSBasicSupport * get_service() const {
return static_cast< TLSBasicSupport *>(this->_get_service(Service::TLS_Basic));
}
```
My suggestion is to forward declare all the support classes in this header and inline the template specializations directly here. This approach
* lets the reader see everything in one place
* removes the need to include NetVConnection headers in more places
* lets the compiler inline the specializations
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -101,7 +101,7 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->connection_state.mutex = this->mutex;
- TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ TLSEarlyDataSupport *eds = new_vc->get_service<TLSEarlyDataSupport>();
if (eds != nullptr) {
Review Comment:
```suggestion
if (auto *eds = new_vc->get_service<TLSEarlyDataSupport>()) {
```
##########
proxy/http/HttpSM.cc:
##########
@@ -6595,12 +6595,12 @@ HttpSM::attach_server_session()
UnixNetVConnection *server_vc = static_cast<UnixNetVConnection *>(server_txn->get_netvc());
// set flag for server session is SSL
- TLSBasicSupport *tbs = dynamic_cast<TLSBasicSupport *>(server_vc);
+ TLSBasicSupport *tbs = server_vc->get_service<TLSBasicSupport>();
if (tbs) {
server_connection_is_ssl = true;
}
Review Comment:
```suggestion
if (auto *tbs = server_vc->get_service<TLSBasicSupport>(); tbs) {
server_connection_is_ssl = true;
}
```
##########
src/traffic_server/InkAPI.cc:
##########
@@ -9660,7 +9660,7 @@ TSVConnProtocolEnable(TSVConn connp, const char *protocol_name)
TSReturnCode retval = TS_ERROR;
int protocol_idx = globalSessionProtocolNameRegistry.toIndexConst(std::string_view{protocol_name});
auto net_vc = reinterpret_cast<UnixNetVConnection *>(connp);
- auto alpn_vc = dynamic_cast<ALPNSupport *>(net_vc);
+ auto alpn_vc = net_vc->get_service<ALPNSupport>();
if (alpn_vc) {
Review Comment:
```suggestion
if (auto *alpn = net_vc->get_service<ALPNSupport>()) {
```
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -120,7 +120,7 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->write_buffer = new_MIOBuffer(buffer_block_size_index);
uint32_t buffer_water_mark;
- TLSSNISupport *snis = dynamic_cast<TLSSNISupport *>(this->_vc);
+ TLSSNISupport *snis = this->_vc->get_service<TLSSNISupport>();
Review Comment:
Not sure why this can't be something like:
```c++
this->write_buffer->water_mark = Http2::buffer_water_mark;
if (auto *sni = this->_vc->get_service<TLSSNISupport>(); snis && snis->hints_from_sni.http2_buffer_water_mark.has_value()) {
buffer_water_mark = snis->hints_from_sni.http2_buffer_water_mark.value();
}
```
##########
src/traffic_server/InkAPI.cc:
##########
@@ -9674,7 +9674,7 @@ TSVConnProtocolDisable(TSVConn connp, const char *protocol_name)
TSReturnCode retval = TS_ERROR;
int protocol_idx = globalSessionProtocolNameRegistry.toIndexConst(std::string_view{protocol_name});
auto net_vc = reinterpret_cast<UnixNetVConnection *>(connp);
- auto alpn_vc = dynamic_cast<ALPNSupport *>(net_vc);
+ auto alpn_vc = net_vc->get_service<ALPNSupport>();
if (alpn_vc) {
Review Comment:
```suggestion
if (auto *alpn = net_vc->get_service<ALPNSupport>()) {
```
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608277276
How about this approach?
```
wkaras ~/STUFF/GET_SERVICE
$ cat x.cc
#include <cstdint>
namespace ts
{
// Returns offset from Base1 to Base2 in any instance of Derived.
//
template <class Derived, class Base1, class Base2>
std::ptrdiff_t base_to_base_offset()
{
// Don't use 0 for dummy address of derived, since compiler may treat null address specially.
//
auto addr = reinterpret_cast<Derived *>(0x100);
return reinterpret_cast<char *>(static_cast<Base2 *>(addr)) - reinterpret_cast<char *>(static_cast<Base1 *>(addr));
}
} // end namespace ts
enum class X_SERVICE {
Y1, Y2, Y3, NUM
};
template <class>
struct X_Service;
static std::ptrdiff_t const X_SERVICE_NONE{PTRDIFF_MIN};
class X
{
public:
template <class C>
C const * get_service() const
{
auto ofs = _service_ofs()[std::size_t(X_Service<C>::value)];
if (X_SERVICE_NONE == ofs) {
return nullptr;
} else {
return reinterpret_cast<C const *>(reinterpret_cast<char const *>(this) + ofs);
}
}
template <class C>
C * get_service()
{
std::ptrdiff_t ofs = _service_ofs()[std::size_t(X_Service<C>::value)];
if (X_SERVICE_NONE == ofs) {
return nullptr;
} else {
return reinterpret_cast<C *>(reinterpret_cast<char *>(this) + ofs);
}
}
private:
static std::ptrdiff_t const * const _service_ofs_table;
// Returns array (A), indexed by service (S), where A[S] is the offset from base class X to the base class for service
// S in the same object, or NO_SERVICE if there is no base class in the object for service S.
//
virtual std::ptrdiff_t const * _service_ofs() const { return _service_ofs_table; }
int dummy_data;
};
std::ptrdiff_t const * const X::_service_ofs_table{[]() -> std::ptrdiff_t *
{
static std::ptrdiff_t tbl[std::size_t(X_SERVICE::NUM)];
tbl[std::size_t(X_SERVICE::Y1)] = X_SERVICE_NONE;
tbl[std::size_t(X_SERVICE::Y2)] = X_SERVICE_NONE;
tbl[std::size_t(X_SERVICE::Y3)] = X_SERVICE_NONE;
return tbl;
}()};
struct Y1
{
int dummy_data;
};
template <>
struct X_Service<Y1>
{
static X_SERVICE const value{X_SERVICE::Y1};
};
struct Y2
{
int dummy_data;
};
template <>
struct X_Service<Y2>
{
static X_SERVICE const value{X_SERVICE::Y2};
};
struct Y3
{
int dummy_data;
};
template <>
struct X_Service<Y3>
{
static X_SERVICE const value{X_SERVICE::Y3};
};
class D1 : public X, public Y1, public Y3
{
public:
int dummy_data;
private:
static std::ptrdiff_t const * const _service_ofs_table;
// Returns array (A), indexed by service (S), where A[S] is the offset from base class X to the base class for service
// S in the same object, or NO_SERVICE if there is no base class in the object for service S.
//
std::ptrdiff_t const * _service_ofs() const override { return _service_ofs_table; }
};
std::ptrdiff_t const * const D1::_service_ofs_table{[]() -> std::ptrdiff_t *
{
static std::ptrdiff_t tbl[std::size_t(X_SERVICE::NUM)];
tbl[std::size_t(X_SERVICE::Y1)] = ts::base_to_base_offset<D1, X, Y1>();
tbl[std::size_t(X_SERVICE::Y2)] = X_SERVICE_NONE;
tbl[std::size_t(X_SERVICE::Y3)] = ts::base_to_base_offset<D1, X, Y3>();
return tbl;
}()};
#include <cassert>
int main()
{
D1 d1;
X *xp = &d1;
assert(xp->get_service<Y1>() == static_cast<Y1 *>(&d1));
assert(xp->get_service<Y2>() == nullptr);
assert(xp->get_service<Y3>() == static_cast<Y3 *>(&d1));
return 0;
}
wkaras ~/STUFF/GET_SERVICE
$ gcc -Wall -Wextra -pedantic -std=c++17 x.cc -lstdc++
wkaras ~/STUFF/GET_SERVICE
$ ./a.out
wkaras ~/STUFF/GET_SERVICE
$
```
--
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 #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1127286359
##########
proxy/http/HttpSM.cc:
##########
@@ -529,7 +529,7 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
+ if (auto tbs = static_cast<TLSBasicSupport *>(netvc->get_service(typeid(TLSBasicSupport)))) {
Review Comment:
In any case, this is logically incorrect. If the type with NetVConnection as a (direct or indirect) base does not also have TLSBasicSupport as a (direct or indirect) base, static_cast will not return null It will return a garbage, non-null value.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1128837887
##########
iocore/net/NetVConnection.cc:
##########
@@ -118,3 +118,19 @@ NetVCOptions::get_family_string() const
}
return {};
}
+
+void *
+NetVConnection::get_service(const std::type_info &info)
+{
+ if (this->_services.find(std::type_index(info)) != this->_services.end()) {
+ return this;
+ } else {
+ return nullptr;
+ }
+}
Review Comment:
Ok, looks like this approach doesn't work at a minimum. What about other approaches? Do you see any technical issues?
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1564660385
[approve ci autest]
polite_hook_wait failed again.
--
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] jpeach commented on pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1609104417
> ### Before (w/o this change)
> ```
> $ h2load -n 100000 -m 10 -c 10 -t 5 https://localhost:8443/8k
> starting benchmark...
> spawning thread #0: 2 total client(s). 20000 total requests
> spawning thread #1: 2 total client(s). 20000 total requests
> spawning thread #2: 2 total client(s). 20000 total requests
> spawning thread #3: 2 total client(s). 20000 total requests
> spawning thread #4: 2 total client(s). 20000 total requests
> TLS Protocol: TLSv1.3
> Cipher: TLS_AES_128_GCM_SHA256
> Server Temp Key: X25519 253 bits
> Application protocol: h2
> progress: 10% done
> progress: 20% done
> progress: 30% done
> progress: 40% done
> progress: 50% done
> progress: 60% done
> progress: 70% done
> progress: 80% done
> progress: 90% done
> progress: 100% done
>
> finished in 3.44s, 29042.21 req/s, 227.72MB/s
> requests: 100000 total, 100000 started, 100000 done, 100000 succeeded, 0 failed, 0 errored, 0 timeout
> status codes: 100000 2xx, 0 3xx, 0 4xx, 0 5xx
> traffic: 784.11MB (822202500) total, 1.15MB (1202140) headers (space savings 96.37%), 781.25MB (819200000) data
> min max mean sd +/- sd
> time for request: 309us 63.06ms 3.37ms 1.43ms 84.61%
> time for connect: 11.93ms 71.32ms 43.61ms 19.76ms 60.00%
> time to 1st byte: 74.94ms 76.78ms 75.38ms 517us 90.00%
> req/s : 2904.63 2932.24 2911.70 9.42 80.00%
> ```
>
> 
>
> ### After (w/ this change)
> ```
> $ h2load -n 100000 -m 10 -c 10 -t 5 https://localhost:8443/8k
> starting benchmark...
> spawning thread #0: 2 total client(s). 20000 total requests
> spawning thread #1: 2 total client(s). 20000 total requests
> spawning thread #2: 2 total client(s). 20000 total requests
> spawning thread #3: 2 total client(s). 20000 total requests
> spawning thread #4: 2 total client(s). 20000 total requests
> TLS Protocol: TLSv1.3
> Cipher: TLS_AES_128_GCM_SHA256
> Server Temp Key: X25519 253 bits
> Application protocol: h2
> progress: 10% done
> progress: 20% done
> progress: 30% done
> progress: 40% done
> progress: 50% done
> progress: 60% done
> progress: 70% done
> progress: 80% done
> progress: 90% done
> progress: 100% done
>
> finished in 3.31s, 30198.57 req/s, 236.79MB/s
> requests: 100000 total, 100000 started, 100000 done, 100000 succeeded, 0 failed, 0 errored, 0 timeout
> status codes: 100000 2xx, 0 3xx, 0 4xx, 0 5xx
> traffic: 784.11MB (822202500) total, 1.15MB (1202140) headers (space savings 96.37%), 781.25MB (819200000) data
> min max mean sd +/- sd
> time for request: 328us 65.54ms 3.23ms 1.43ms 84.80%
> time for connect: 12.12ms 74.18ms 46.18ms 20.57ms 60.00%
> time to 1st byte: 77.60ms 78.46ms 77.90ms 235us 70.00%
> req/s : 3020.03 3040.60 3027.50 8.17 80.00%
> ```
>
> 
>
> `HttpSM::attach_client_session` is gone.
Note that I haven't looked at the new code changes, but this is a 0.03% increase in RPS, so not necessarily something I would sacrifice very much code readability for :)
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1603683489
### Before (w/o this change)
```
$ h2load -n 100000 -m 10 -c 10 -t 5 https://localhost:8443/8k
starting benchmark...
spawning thread #0: 2 total client(s). 20000 total requests
spawning thread #1: 2 total client(s). 20000 total requests
spawning thread #2: 2 total client(s). 20000 total requests
spawning thread #3: 2 total client(s). 20000 total requests
spawning thread #4: 2 total client(s). 20000 total requests
TLS Protocol: TLSv1.3
Cipher: TLS_AES_128_GCM_SHA256
Server Temp Key: X25519 253 bits
Application protocol: h2
progress: 10% done
progress: 20% done
progress: 30% done
progress: 40% done
progress: 50% done
progress: 60% done
progress: 70% done
progress: 80% done
progress: 90% done
progress: 100% done
finished in 3.44s, 29042.21 req/s, 227.72MB/s
requests: 100000 total, 100000 started, 100000 done, 100000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 100000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 784.11MB (822202500) total, 1.15MB (1202140) headers (space savings 96.37%), 781.25MB (819200000) data
min max mean sd +/- sd
time for request: 309us 63.06ms 3.37ms 1.43ms 84.61%
time for connect: 11.93ms 71.32ms 43.61ms 19.76ms 60.00%
time to 1st byte: 74.94ms 76.78ms 75.38ms 517us 90.00%
req/s : 2904.63 2932.24 2911.70 9.42 80.00%
```
### After (w/ this change)
```
$ h2load -n 100000 -m 10 -c 10 -t 5 https://localhost:8443/8k
starting benchmark...
spawning thread #0: 2 total client(s). 20000 total requests
spawning thread #1: 2 total client(s). 20000 total requests
spawning thread #2: 2 total client(s). 20000 total requests
spawning thread #3: 2 total client(s). 20000 total requests
spawning thread #4: 2 total client(s). 20000 total requests
TLS Protocol: TLSv1.3
Cipher: TLS_AES_128_GCM_SHA256
Server Temp Key: X25519 253 bits
Application protocol: h2
progress: 10% done
progress: 20% done
progress: 30% done
progress: 40% done
progress: 50% done
progress: 60% done
progress: 70% done
progress: 80% done
progress: 90% done
progress: 100% done
finished in 3.31s, 30198.57 req/s, 236.79MB/s
requests: 100000 total, 100000 started, 100000 done, 100000 succeeded, 0 failed, 0 errored, 0 timeout
status codes: 100000 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 784.11MB (822202500) total, 1.15MB (1202140) headers (space savings 96.37%), 781.25MB (819200000) data
min max mean sd +/- sd
time for request: 328us 65.54ms 3.23ms 1.43ms 84.80%
time for connect: 12.12ms 74.18ms 46.18ms 20.57ms 60.00%
time to 1st byte: 77.60ms 78.46ms 77.90ms 235us 70.00%
req/s : 3020.03 3040.60 3027.50 8.17 80.00%
```
`HttpSM::attach_client_session` is gone.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608408248
> None of the dynamic_casts need casting from a base class to another base class.
X corresponds to NetVConnection, and Y1, Y2, ... correspond to TLSTunnelSupport, TLSSNISupport, etc. D1 would correspond to some class like SSLNetVConnection.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608527478
> > It avoids having to pass an SERVICE enum value to get_service() and then cast to the corresponding pointer type (error prone and verbose).
>
> I think this was addressed by having `NetConnectinoService`, which James suggested.
OK true but I don't like the void-returning get_service() having to be a public member.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1460870642
Sure, but again, if we do that the base class has to be modified every time we add new mixins, and the base class has to know all mixin names.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1457446215
To keep changes minimal, I suggest this pattern: https://godbolt.org/z/enn1G89qK .
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1457215973
[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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1460850813
This would be faster: https://godbolt.org/z/66YofcchT
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1468493173
Not as clean as `get_mixin(type_info x)`, and not as fast as `get_mixin_a()`, but can be a compromise? It should be faster than if-else and hash table.
https://godbolt.org/z/T68zTPG15
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1619286298
Looks like `my3` and `my4` have to be always created. Can we have `Y3 *my3` instead? Otherwise we cannot make the base class small. Although composition is an interesting idea, I'm not going to make a change for it now. Especially the dynamic instantiation can make this slow again.
This PR touches many places and I don't want to keep this open for long time to find the best way (there's already a conflict). It looks like we all agreed on the interface at a minimum. I'm going to make the changes James suggested, so we can start using the new interface.
--
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] jpeach commented on a diff in pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1252471343
##########
iocore/net/I_NetVConnection.h:
##########
@@ -857,7 +857,20 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
bool has_proxy_protocol(IOBufferReader *);
bool has_proxy_protocol(char *, int64_t *);
+ template <typename S> S *get_service() const;
+
Review Comment:
I'm not sure that I follow. Something needs to bind the enum to the actual concrete type - intrinsically that requires knowing about both the enum values and the concrete classes.
People better at metaprograming than me can probably think of something cleaner, but here's another approach to mapping the types. You could put it in a separate header
```C++
#include <typeinfo>
#include <iostream>
#include <cxxabi.h>
class HttpServiceType;
class SshServiceType;
class FtpServiceType;
enum class service : int {
http,
ssh,
ftp,
};
template <service s> struct service_type;
template<> struct service_type<service::http> {
using type = HttpServiceType;
};
template<> struct service_type<service::ssh> {
using type = SshServiceType;
};
#define typename_of(expr) abi::__cxa_demangle(typeid(expr).name(), nullptr, nullptr, nullptr)
int main()
{
service_type<service::http>::type * h;
service_type<service::ssh>::type * s;
std::cout
<< typename_of(h) << "\n"
<< typename_of(s) << "\n";
}
```
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1621830473
> Can we have Y3 *my3 instead?
No. If the service object is not within the derived object's memory footprint, there would not be a fixed offset to it. You could perhaps use std::optional.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1460696518
> There is some chance that this would be faster than dynamic_cast: https://godbolt.org/z/fvzWsxj5T .
I don't think we need to cast MixinA to MixinB. In that sense this should be enough, it's so naive though. https://godbolt.org/z/fjnMxfj85
But your code suggests that I could store pre-casted pointers into a map instead of just having supported types in a set.
I wonder if this kind of ways can be ten times faster than dynamic_cast. Only 5% faster way is not a win here.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608464595
Oh you mean the mixins.
What's the advantage of your code? Looks like doing static_cast in a complicated way.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608492690
> It avoids having to pass an SERVICE enum value to get_service() and then cast to the corresponding pointer type (error prone and verbose).
I think this was addressed by having `NetConnectinoService`, which James suggested.
> It avoids the need for a _services[] array in every instance of NetVConnection. This might increase cache hits and thus improve performance.
Maybe? I'm not sure how much it helps. Your `get_service` has a conditional branch and an addition. Need some numbers.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1459344525
There is some chance that this would be faster than dynamic_cast: https://godbolt.org/z/vcc7Kr8bM
--
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 #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1128826609
##########
iocore/net/NetVConnection.cc:
##########
@@ -118,3 +118,19 @@ NetVCOptions::get_family_string() const
}
return {};
}
+
+void *
+NetVConnection::get_service(const std::type_info &info)
+{
+ if (this->_services.find(std::type_index(info)) != this->_services.end()) {
+ return this;
+ } else {
+ return nullptr;
+ }
+}
Review Comment:
This will not work properly, for reasons illustrated by this example code: https://godbolt.org/z/bGofcYhse . Not that f() and g() do not have the same assembly code. With multiple inheritance, there is no known implementation (and probably impossible) where all the base classes to have the same (start) address as the derived class. Even with no multiple inheritance, I think it's undefined behavior to assume that (direct and indirect) base classes have the same address as the derived class. Although the de facto portability of this assumption is very high in that case.
--
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] jpeach commented on pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1579694769
>I don't know how much it affects performance, but this is one of ways to remove the dynamic_cast.
Were you able to do any benchmarks to demonstrate the performance effect?
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1608468905
- It avoids having to pass an SERVICE enum value to get_service() and then cast to the corresponding pointer type (error prone and verbose).
- It avoids the need for a _services[] array in every instance of NetVConnection. This might increase cache hits and thus improve performance.
--
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] jpeach commented on pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1612227043
> I suggest using this alternate approach. It only requires each NetVConnection object to have an additional pointer, rather than an array of pointers:
IIUC this requires the use of inheritance, where the existing approach allows the option of composition. I agree that is would be nice to store fewer pointers though.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1611644462
I suggest using this alternate approach. It only requires each NetVConnection object to have an additional pointer, rather than an array of pointers:
```
#include <cstdint>
namespace ts
{
// Returns offset from Base1 to Base2 in any instance of Derived.
//
template <class Derived, class Base1, class Base2>
std::ptrdiff_t base_to_base_offset()
{
// Don't use 0 for dummy address of derived, since compiler may treat null address specially.
//
auto addr = reinterpret_cast<Derived *>(0x100);
return reinterpret_cast<char *>(static_cast<Base2 *>(addr)) - reinterpret_cast<char *>(static_cast<Base1 *>(addr));
}
} // end namespace ts
enum class X_SERVICE {
Y1, Y2, Y3, NUM
};
template <class>
struct X_Service;
static std::ptrdiff_t const X_SERVICE_NONE{PTRDIFF_MIN};
class X
{
public:
X(std::ptrdiff_t const *service_ofs) : _service_ofs(service_ofs) {}
template <class C>
C const * get_service() const
{
auto ofs = _service_ofs[std::size_t(X_Service<C>::value)];
if (X_SERVICE_NONE == ofs) {
return nullptr;
} else {
return reinterpret_cast<C const *>(reinterpret_cast<char const *>(this) + ofs);
}
}
template <class C>
C * get_service()
{
std::ptrdiff_t ofs = _service_ofs[std::size_t(X_Service<C>::value)];
if (X_SERVICE_NONE == ofs) {
return nullptr;
} else {
return reinterpret_cast<C *>(reinterpret_cast<char *>(this) + ofs);
}
}
private:
// Indexed by service (S), where _service_ofs[S] is the offset from base class X to the base class for service
// S in the same object, or NO_SERVICE if there is no base class in the object for service S.
//
std::ptrdiff_t const * const _service_ofs;
int dummy_data;
};
struct Y1
{
int dummy_data;
};
template <>
struct X_Service<Y1>
{
static X_SERVICE const value{X_SERVICE::Y1};
};
struct Y2
{
int dummy_data;
};
template <>
struct X_Service<Y2>
{
static X_SERVICE const value{X_SERVICE::Y2};
};
struct Y3
{
int dummy_data;
};
template <>
struct X_Service<Y3>
{
static X_SERVICE const value{X_SERVICE::Y3};
};
class D1 : public X, public Y1, public Y3
{
public:
D1() : X(_service_ofs_table) {}
private:
static std::ptrdiff_t const * const _service_ofs_table;
int dummy_data;
};
std::ptrdiff_t const * const D1::_service_ofs_table{[]() -> std::ptrdiff_t *
{
static std::ptrdiff_t tbl[std::size_t(X_SERVICE::NUM)];
tbl[std::size_t(X_SERVICE::Y1)] = ts::base_to_base_offset<D1, X, Y1>();
tbl[std::size_t(X_SERVICE::Y2)] = X_SERVICE_NONE;
tbl[std::size_t(X_SERVICE::Y3)] = ts::base_to_base_offset<D1, X, Y3>();
return tbl;
}()};
#include <cassert>
int main()
{
D1 d1;
X *xp = &d1;
assert(xp->get_service<Y1>() == static_cast<Y1 *>(&d1));
assert(xp->get_service<Y2>() == nullptr);
assert(xp->get_service<Y3>() == static_cast<Y3 *>(&d1));
return 0;
}
```
This performance test indicates the performance would be the same or slightly better:
```
wkaras ~/STUFF/IF_VS_ARR
$ cat scr2
cat x2.cc
echo ==============
gcc -O3 -Wall -Wextra -pedantic -std=c++17 x2.cc -lstdc++
time ./a.out
echo ==============
gcc -DWALT -O3 -Wall -Wextra -pedantic -std=c++17 x2.cc -lstdc++
time ./a.out
wkaras ~/STUFF/IF_VS_ARR
$ . scr2
int const Dim = 7;
int const Idx = 4;
int const Num_s = 1 << 14;
int const Iters = 1 << 17;
using Ofs_t = long long;
char dummy;
Ofs_t the_ofs_arr[Dim];
struct S
{
char * volatile arr[Dim];
Ofs_t volatile * volatile ofs_arr{the_ofs_arr};
};
S s[Num_s];
S * volatile sp[Num_s];
char * volatile cp;
int main()
{
for (int i{0}; i < Num_s; ++i) {
s[i].arr[Idx] = &dummy + i;
sp[i] = s + i;
}
the_ofs_arr[Idx] = 42;
#ifndef WALT
for (int i{Iters}; i; --i) {
for (int j{0}; j < Num_s; ++j) {
char *p = sp[j]->arr[Idx];
if (p) {
cp = p;
}
}
}
#else
char *dp = &dummy;
for (int i{Iters}; i; --i) {
for (int j{0}; j < Num_s; ++j) {
Ofs_t ofs = sp[j]->ofs_arr[Idx];
if (666 != ofs) {
cp = dp + ofs;
}
}
}
#endif
return 0;
}
==============
real 0m2.657s
user 0m2.653s
sys 0m0.004s
==============
real 0m2.541s
user 0m2.541s
sys 0m0.000s
wkaras ~/STUFF/IF_VS_ARR
$
```
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1619143006
OK, here's composition, and the option of multiple services with the same type. For a total of three shrubberies.
```
#include <cstdint>
namespace ts
{
// Returns offset from Base1 to Base2 in any instance of Derived.
//
template <class Derived, class Base1, class Base2>
std::ptrdiff_t base_to_base_offset()
{
// Don't use 0 for dummy address of derived, since compiler may treat null address specially.
//
auto addr = reinterpret_cast<Derived *>(0x100);
return reinterpret_cast<char *>(static_cast<Base2 *>(addr)) - reinterpret_cast<char *>(static_cast<Base1 *>(addr));
}
// Returns offset from Base1 to data member of Derived.
//
template <class Derived, class Base1, typename MbrT, MbrT Derived::*Mbr>
std::ptrdiff_t base_to_mbr_offset()
{
// Don't use 0 for dummy address of derived, since compiler may treat null address specially.
//
auto addr = reinterpret_cast<Derived *>(0x100);
return reinterpret_cast<char *>(&(addr->*Mbr)) - reinterpret_cast<char *>(static_cast<Base1 *>(addr));
}
} // end namespace ts
enum class X_SERVICE {
Y1, Y2, Y3_0, Y3_1, Y4, NUM
};
template <class, int>
struct X_Service;
static std::ptrdiff_t const X_SERVICE_NONE{PTRDIFF_MIN};
class X
{
public:
X(std::ptrdiff_t const *service_ofs) : _service_ofs(service_ofs) {}
template <class C, int Selector = 0>
C const * get_service() const
{
auto ofs = _service_ofs[std::size_t(X_Service<C, Selector>::value)];
if (X_SERVICE_NONE == ofs) {
return nullptr;
} else {
return reinterpret_cast<C const *>(reinterpret_cast<char const *>(this) + ofs);
}
}
template <class C, int Selector = 0>
C * get_service()
{
std::ptrdiff_t ofs = _service_ofs[std::size_t(X_Service<C, Selector>::value)];
if (X_SERVICE_NONE == ofs) {
return nullptr;
} else {
return reinterpret_cast<C *>(reinterpret_cast<char *>(this) + ofs);
}
}
private:
// Indexed by service (S), where _service_ofs[S] is the offset from base class X to the base class or data member for
// service S in the same object, or NO_SERVICE if there is no such base/member in the object for service S.
//
std::ptrdiff_t const * const _service_ofs;
int dummy_data;
};
struct Y1
{
int dummy_data;
};
template <>
struct X_Service<Y1, 0>
{
static X_SERVICE const value{X_SERVICE::Y1};
};
struct Y2
{
int dummy_data;
};
template <>
struct X_Service<Y2, 0>
{
static X_SERVICE const value{X_SERVICE::Y2};
};
struct Y3
{
int dummy_data;
};
template <>
struct X_Service<Y3, 0>
{
static X_SERVICE const value{X_SERVICE::Y3_0};
};
template <>
struct X_Service<Y3, 1>
{
static X_SERVICE const value{X_SERVICE::Y3_1};
};
struct Y4
{
int dummy_data;
};
template <>
struct X_Service<Y4, 0>
{
static X_SERVICE const value{X_SERVICE::Y4};
};
class D1 : public X, public Y1, public Y3
{
public:
D1() : X(_service_ofs_table) {}
Y3 my3;
Y4 my4;
private:
static std::ptrdiff_t const * const _service_ofs_table;
int dummy_data;
};
std::ptrdiff_t const * const D1::_service_ofs_table{[]() -> std::ptrdiff_t *
{
static std::ptrdiff_t tbl[std::size_t(X_SERVICE::NUM)];
tbl[std::size_t(X_SERVICE::Y1)] = ts::base_to_base_offset<D1, X, Y1>();
tbl[std::size_t(X_SERVICE::Y2)] = X_SERVICE_NONE;
tbl[std::size_t(X_SERVICE::Y3_0)] = ts::base_to_base_offset<D1, X, Y3>();
tbl[std::size_t(X_SERVICE::Y3_1)] = ts::base_to_mbr_offset<D1, X, Y3, &D1::my3>();
tbl[std::size_t(X_SERVICE::Y4)] = ts::base_to_mbr_offset<D1, X, Y4, &D1::my4>();
return tbl;
}()};
#include <cassert>
int main()
{
D1 d1;
X *xp = &d1;
assert(xp->get_service<Y1>() == static_cast<Y1 *>(&d1));
assert(xp->get_service<Y2>() == nullptr);
assert((xp->get_service<Y3, 0>()) == static_cast<Y3 *>(&d1));
assert((xp->get_service<Y3, 1>()) == &d1.my3);
assert(xp->get_service<Y4>() == &d1.my4);
return 0;
}
```
--
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] jpeach commented on a diff in pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1257393172
##########
iocore/net/I_NetVConnection.h:
##########
@@ -540,3 +562,71 @@ NetVConnection::trapWriteBufferEmpty(int event)
{
write_buffer_empty_event = event;
}
+
+inline void *
+NetVConnection::_get_service(enum NetVConnection::Service service) const
+{
+ return _services[static_cast<unsigned int>(service)];
+}
+
+inline void
+NetVConnection::_set_service(enum NetVConnection::Service service, void *instance)
Review Comment:
Note that this isn't type-safe. I expect it would be possible to make it typesafe if you had a template that mapped the enum to the type directly.
##########
proxy/http/Http1ClientSession.cc:
##########
@@ -141,7 +141,7 @@ Http1ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
trans.mutex = mutex; // Share this mutex with the transaction
in_destroy = false;
- TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ TLSEarlyDataSupport *eds = new_vc->get_service<TLSEarlyDataSupport>();
if (eds != nullptr) {
Review Comment:
```suggestion
if (TLSEarlyDataSupport *eds = new_vc->get_service<TLSEarlyDataSupport>()) {
```
We should be consistent about how we call `get_service`.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1622041823
I think it's in a good shape now, and I'd like to merge this if there's no blocker. Let me know if there are comments/issues that are not resolved yet.
--
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] jpeach commented on pull request #9482: Add interface for NetVC services
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1579775837
My 2c o the API here is that there's too great a reliance on always having to match the enum and type pointer. You need to do it in every place that registers a helper, and every place that consumes one, which is error-prone. It would be great if the mapping was in exactly one place.
For example, you could keep the mapping in a template specialization:
```C++
template <typename T> T* NetConnectionService(const NetVConnection *);
template<> ALPNSupport* NetConnectionService(const NetVConnection * vc)
{
return static_cast< ALPNSupport*>(vc.get_thing(TLS_ALPN))
}
/// etc ..
```
This approach could also let you get rid of the nil checks:
```C++
template <typename T, F>
WithNetConnectionService(const NetVConnection * vc, F&& handler)
{
T * svc = NetConnectionService<T>(vc);
if (svc) {
handler(svc)
}
}
```
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1457402164
`virtual void do_something()` is one of options. But in that way, NetVConnection would have a lot of that kind of functions. And I think it would be a mess eventually. I can imagine somebody will propose `do_something_detail_for_xyz` just for one of NetVCs without abstraction. Also, some of such functions would probably need some parameters. I guess it'd be difficult to abstract everything.
`virtual bool is_feature_x_supprted()` for each mix-in may be better in that sense. But NetVC knows the name of x, and we have to modify NetVC base class whenever we add a new mix-in.
`virtual bool has_support_for(enum_or_string feature_name)` is clean. It would not bring any detail to NetVC itself. If `get_service()` isn't acceptable, I can live with this one.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1457215185
The virtual member function I added for the tunnel metrics may show a pattern could be used for this: https://github.com/apache/trafficserver/pull/9403/files#diff-28fd9480778c3af26dbe33816ff4f4d35a616444bb9d19adaecd6735e29ca546R365
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1603188200
I'm going to abstain on this one. The clean way to get rid of dynamic_cast is to move the distinct behavior for derived classes into the derived class. But it's very difficult to do that in this case. I feel like this is adding complexity, for only a speculative benefit of significantly improved performance. Maybe find another reviewer, or start a thread on the dev mailing list.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1253300095
##########
iocore/net/I_NetVConnection.h:
##########
@@ -857,7 +857,20 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
bool has_proxy_protocol(IOBufferReader *);
bool has_proxy_protocol(char *, int64_t *);
+ template <typename S> S *get_service() const;
+
Review Comment:
> Something needs to bind the enum to the actual concrete type - intrinsically that requires knowing about both the enum values and the concrete classes.
Yes, I understand it. With dynamic_cast, such thing did not exist at all. But now with the enum, something has to know the both. The enum is already a compromise for better performance, and I can live with the helper functions. I don't like to make code cryptic.
--
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 #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1564740709
[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 pull request #9482: Add interface for NetVC services
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1613597924
>> I suggest using this alternate approach. It only requires each NetVConnection object to have an additional pointer, rather than an array of pointers:
> IIUC this requires the use of inheritance, where the existing approach allows the option of composition. I agree that is would be nice to store fewer pointers though.
Composition is an interesting idea. It can reduce the size of Unix/SSL/QUICNetVConnection. For example, if TLS early data is disabled by setting, we don't need to have TLSEarlyData and related variables in the first place.
--
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 pull request #9482: Add interface for NetVC services
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#issuecomment-1609578180
Looks to me like a 4% increase in RPS: https://search.yahoo.com/search?ei=UTF-8&fr=crmas&p=30198.87%2F29042.21
--
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