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/05/31 14:08:15 UTC
[PR] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
JosiahWI opened a new pull request, #11407:
URL: https://github.com/apache/trafficserver/pull/11407
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 adds two new methods to that class: `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.
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#issuecomment-2148449374
LSan detected the same leak after rerolling the CI.
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#issuecomment-2152766510
[approve ci docs]
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#issuecomment-2150659246
[approve ci rocky]
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "vmamidi (via GitHub)" <gi...@apache.org>.
vmamidi commented on PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#issuecomment-2165425171
looks good to me.
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#issuecomment-2148371872
[approve ci rocky 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
Re: [PR] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#discussion_r1625221380
##########
src/iocore/cache/AggregateWriteBuffer.cc:
##########
@@ -30,6 +34,23 @@
#include <cstring>
+void
+AggregateWriteBuffer::add(Doc const *doc, int approx_size)
+{
+ std::memcpy(this->_buffer + this->_buffer_pos, doc, doc->len);
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(approx_size);
+}
+
+Doc *
+AggregateWriteBuffer::emplace(int approx_size)
+{
+ Doc *result{new (this->_buffer + this->_buffer_pos) Doc};
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(-approx_size);
Review Comment:
See L76-78 - the implementation of `add` is in clear disagreement with the stated requirement.
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#discussion_r1625199264
##########
src/iocore/cache/AggregateWriteBuffer.cc:
##########
@@ -30,6 +34,23 @@
#include <cstring>
+void
+AggregateWriteBuffer::add(Doc const *doc, int approx_size)
+{
+ std::memcpy(this->_buffer + this->_buffer_pos, doc, doc->len);
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(approx_size);
+}
+
+Doc *
+AggregateWriteBuffer::emplace(int approx_size)
+{
+ Doc *result{new (this->_buffer + this->_buffer_pos) Doc};
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(-approx_size);
Review Comment:
I'm confused with why `_bytes_pending_aggregation` is increased by `add` and it's reduced by `emplace`.
##########
src/iocore/cache/P_CacheVol.h:
##########
@@ -329,6 +329,7 @@ class Stripe : public Continuation
void _init_dir();
void _init_data_internal();
void _init_data();
+ int agg_copy(CacheVC *vc);
Review Comment:
`_agg_copy` to follow naming convention?
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#discussion_r1625221380
##########
src/iocore/cache/AggregateWriteBuffer.cc:
##########
@@ -30,6 +34,23 @@
#include <cstring>
+void
+AggregateWriteBuffer::add(Doc const *doc, int approx_size)
+{
+ std::memcpy(this->_buffer + this->_buffer_pos, doc, doc->len);
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(approx_size);
+}
+
+Doc *
+AggregateWriteBuffer::emplace(int approx_size)
+{
+ Doc *result{new (this->_buffer + this->_buffer_pos) Doc};
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(-approx_size);
Review Comment:
See L76-78 of the header - the implementation of `add` is in clear disagreement with the stated requirement.
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#discussion_r1625217999
##########
src/iocore/cache/AggregateWriteBuffer.cc:
##########
@@ -30,6 +34,23 @@
#include <cstring>
+void
+AggregateWriteBuffer::add(Doc const *doc, int approx_size)
+{
+ std::memcpy(this->_buffer + this->_buffer_pos, doc, doc->len);
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(approx_size);
+}
+
+Doc *
+AggregateWriteBuffer::emplace(int approx_size)
+{
+ Doc *result{new (this->_buffer + this->_buffer_pos) Doc};
+ this->_buffer_pos += approx_size;
+ this->add_bytes_pending_aggregation(-approx_size);
Review Comment:
It should be decreased by `add`. Might have slipped in during a rebase - I'll have to add a test for it, because even the regression tests apparently missed 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
Re: [PR] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI merged PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407
--
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] Clean up the insertion of documents into the aggregate write buffer [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11407:
URL: https://github.com/apache/trafficserver/pull/11407#issuecomment-2148371632
LSan detected a leak in the ImageMagick unit test, and the AuTest ja3_fingerprint 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