apache / trafficserver

Apache Traffic Serverâ„¢ is a fast, scalable and extensible HTTP/1.1 and HTTP/2 compliant caching proxy server.
https://trafficserver.apache.org/
Apache License 2.0
1.8k stars 798 forks source link

Clean up the insertion of documents into the aggregate write buffer #11407

Closed JosiahWI closed 3 months ago

JosiahWI commented 4 months ago

This is a short preamble to refactoring agg_copy, but I'm PRing it separately because it mostly touches AggregateWriteBuffer and I'm more confident in the change. It moves agg_copy to Stripe::agg_copy and adds two new methods to AggregateWriteBuffer: add and emplace. The implementation of emplace uses placement new to create the document instead of a reinterpret cast because this is precisely what it's designed for and makes the intent crystal clear.

The two new methods also adjust the buffer position and the number of bytes pending copy. This puts directly related logic together that was previously spread out across hundreds of lines.

The AggregateWriteBuffer::add_buffer_pos method has been removed, improving encapsulation and simplifying the interface.

This also moves AggregateWriteBuffer.cc and AggregateWriteBuffer.h to src/ instead of include/ because it's an internal detail at this point, not part of the public interface.

JosiahWI commented 3 months ago

LSan detected a leak in the ImageMagick unit test, and the AuTest ja3_fingerprint failed.

JosiahWI commented 3 months ago

LSan detected the same leak after rerolling the CI.

vmamidi commented 3 months ago

looks good to me.