Remove unnecessary randomize() call

[masaori335]

QUICConnectionId::randomize() is always called by QUICPacketHandlerIn::_recv_packet() from here.

trafficserver/iocore/net/QUICPacketHandler.cc

Line 289 in 08042e6

QUICConnectionId cid_in_retry_token;

This looks bit expensive.

It looks like QUICConnectionId::randomize() is called explicitly when it is needed. Is there any place assuming randomizing is done in constructor?

[maskit]

Those calls are probably from these two lines.

trafficserver/iocore/net/QUICPacketHandler.cc

Lines 98 to 99 in 9fc03ee

	QUICConnectionId dcid = QUICConnectionId::ZERO();
	QUICConnectionId scid = QUICConnectionId::ZERO();

I think we should probably have appropriate constructor and make randomize() private. Then we can remove calls of randomize() from many places.

[maskit]

They are wrong lines but the same thing.

trafficserver/iocore/net/QUICPacketHandler.cc

Lines 216 to 217 in 9fc03ee

	QUICConnectionId dcid = QUICConnectionId::ZERO();
	QUICConnectionId scid = QUICConnectionId::ZERO();

[maskit]

Also, _rerandomize_original_cid is not used at all.

[masaori335]

Those calls are probably from these two lines.

I don't think QUICConnectionId::ZERO() calls QUICConnectionId::randomize().

trafficserver/iocore/net/quic/QUICTypes.cc

Lines 559 to 563 in 9fc03ee

	QUICConnectionId::ZERO()
	{
	uint8_t zero[MAX_LENGTH] = {0};
	return QUICConnectionId(zero, sizeof(zero));
	}

[maskit]

ZERO() will not call randomize() but I guess the default constructor is called before assigning one returned from ZERO().

[masaori335]

That = is (copy?) initialization, so default constructor is not called in this case. Also I checked on debugger. Line 289 is calling default constructor and it go to randomize().

(lldb) bt
* thread #2, name = '[ET_UDP 0]', stop reason = breakpoint 1.1
  * frame #0: 0x00000001003e5980 traffic_server`QUICConnectionId::randomize(this=0x0000000008ae09b0) at QUICTypes.cc:595:22
    frame #1: 0x00000001003e5969 traffic_server`QUICConnectionId::QUICConnectionId(this=0x0000000008ae09b0) at QUICTypes.cc:567:9
    frame #2: 0x00000001003e4db5 traffic_server`QUICConnectionId::QUICConnectionId(this=0x0000000008ae09b0) at QUICTypes.cc:566:1
    frame #3: 0x0000000100377696 traffic_server`QUICPacketHandlerIn::_recv_packet(this=0x0000000000b1c240, event=210, udp_packet=0x0000000002047f60) at QUICPacketHandler.cc:289:20
    frame #4: 0x0000000100376b4c traffic_server`QUICPacketHandlerIn::acceptEvent(this=0x0000000000b1c240, event=210, data=0x0000000008ae0c58) at QUICPacketHandler.cc:174:13
    frame #5: 0x0000000100010d51 traffic_server`Continuation::handleEvent(this=0x0000000000b1c240, event=210, data=0x0000000008ae0c58) at I_Continuation.h:190:12
    frame #6: 0x000000010036d580 traffic_server`UnixUDPConnection::callbackHandler(this=0x0000000000d4ba50, event=0, data=0x0000000000000000) at UnixUDPConnection.cc:89:21
    frame #7: 0x000000010036f034 traffic_server`UDPNetProcessorInternal::udp_callback(this=0x000000010062f8b0, nh=0x00000000084e5a40, xuc=0x0000000000d4ba50, thread=0x00000000084d4000) at UnixUDPNet.cc:276:9
    frame #8: 0x0000000100373623 traffic_server`UDPNetHandler::waitForActivity(this=0x00000000084e5a40, timeout=60000000) at UnixUDPNet.cc:1111:24
    frame #9: 0x00000001003b887b traffic_server`EThread::execute_regular(this=0x00000000084d4000) at UnixEThread.cc:277:14
    frame #10: 0x00000001003b8ce8 traffic_server`EThread::execute(this=0x00000000084d4000) at UnixEThread.cc:338:11
    frame #11: 0x00000001003b7042 traffic_server`spawn_thread_internal(a=0x0000000004015be0) at Thread.cc:92:12
    frame #12: 0x00007fff5acce2eb libsystem_pthread.dylib`_pthread_body + 126
    frame #13: 0x00007fff5acd1249 libsystem_pthread.dylib`_pthread_start + 66
    frame #14: 0x00007fff5accd40d libsystem_pthread.dylib`thread_start + 13
(lldb) frame select 3
frame #3: 0x0000000100377696 traffic_server`QUICPacketHandlerIn::_recv_packet(this=0x0000000000b1c240, event=210, udp_packet=0x0000000002047f60) at QUICPacketHandler.cc:289:20
   286
   287    // Server Stateless Retry
   288    QUICConfig::scoped_config params;
-> 289    QUICConnectionId cid_in_retry_token;
   290    if (!vc && params->stateless_retry() && QUICInvariants::is_long_header(buf)) {
   291      int ret = this->_stateless_retry(buf, buf_len, udp_packet->getConnection(), udp_packet->from, dcid, scid, &cid_in_retry_token);
   292      if (ret < 0) {

---

Update: I set breakpoint on randomize() and the line 289 is the first place which hit the breakpoint.

[maskit]

OK, then we should be able to make it cid_in_retry_token = QUICConnectionId::ZERO();

Which case is more common?

[maskit]

My preference is not having public randomize(), and having constructors for each use cases.

[masaori335]

It could be another option, but which means when somebody just declare ConnectionId like this case, it goes to expensive function. It looks trap from performance perspective.

[maskit]

Is it better than use of non-randomized value at where it has to be?

[masaori335]

In this case the ConnectionId doesn't have to be randomized nor ZERO. It's just declared.

[masaori335]

After discussion of default constructor design of QUICConnectionId (performance vs security) on slack, we decided to push workaround fix (initializing with ZERO) for now. 4aa5ba2

With this commit, the 6.5% usage of randomize() is gone.