moonchen commented on code in PR #13455:
URL: https://github.com/apache/trafficserver/pull/13455#discussion_r3752947200
##########
src/proxy/hdrs/MIME.cc:
##########
@@ -1002,6 +1020,8 @@ mime_hdr_destroy(HdrHeap *heap, MIMEHdrImpl *mh)
// heap->deallocate_obj(mh);
}
+static void mime_hdr_rebuild_field_free_list(MIMEHdrImpl *mh);
Review Comment:
Optional suggestion: `mime_hdr_rebuild_field_free_list` could be moved up
here.
##########
src/proxy/hdrs/unit_tests/test_Hdrs.cc:
##########
@@ -657,6 +657,175 @@ test_arena_aux(Arena &arena, int len)
} // end anonymous namespace
+TEST_CASE("MIME fields reuse deleted slots", "[proxy][hdrtest][mime]")
+{
+ mime_init();
+ http_init();
+
+ auto add_field = [](HTTPHdr &hdr, std::string_view name) {
+ MIMEField *field = hdr.field_create(name);
+
+ hdr.field_attach(field);
+ return field;
+ };
+
+ SECTION("A fully deleted field block is retained and reused")
+ {
+ HTTPHdr hdr;
+ hdr.create(HTTPType::RESPONSE);
+ ts::PostScript cleanup([&]() -> void { hdr.destroy(); });
+
+ std::array<MIMEField *, MIME_FIELD_BLOCK_SLOTS * 2> fields;
+ for (unsigned index = 0; index < fields.size(); ++index) {
+ fields[index] = add_field(hdr, "X-Field-" + std::to_string(index));
+ }
+
+ MIMEFieldBlockImpl *second_block = hdr.m_mime->m_first_fblock.m_next;
+ REQUIRE(second_block != nullptr);
+ REQUIRE(second_block == hdr.m_mime->m_fblock_list_tail);
+
+ for (unsigned index = MIME_FIELD_BLOCK_SLOTS; index < fields.size();
++index) {
+ hdr.field_delete(fields[index], false);
+ }
+
+ CHECK(hdr.m_mime->m_first_fblock.m_next == second_block);
+ CHECK(hdr.m_mime->m_fblock_list_tail == second_block);
+
+ MIMEField *reused = hdr.field_create("X-Reused");
+ CHECK(reused == fields.back());
+ CHECK(hdr.m_mime->m_fblock_list_tail == second_block);
+ }
+
+ SECTION("Reused duplicate fields remain ordered by slot")
+ {
+ HTTPHdr hdr;
+ hdr.create(HTTPType::RESPONSE);
+ ts::PostScript cleanup([&]() -> void { hdr.destroy(); });
+
+ MIMEField *first = add_field(hdr, "X-Duplicate");
+ MIMEField *filler = add_field(hdr, "X-Filler");
+ MIMEField *last = add_field(hdr, "X-Duplicate");
+ for (unsigned index = 3; index < MIME_FIELD_BLOCK_SLOTS; ++index) {
+ add_field(hdr, "X-Filler-" + std::to_string(index));
+ }
+
+ REQUIRE(first->m_next_dup == last);
+ hdr.field_delete(first, false);
+ REQUIRE(hdr.field_find("X-Duplicate") == last);
+
+ MIMEField *reused = add_field(hdr, "X-Duplicate");
+ CHECK(reused == first);
+ CHECK(hdr.field_find("X-Duplicate") == reused);
+ CHECK(reused->m_next_dup == last);
+ CHECK(last->m_next_dup == nullptr);
+ CHECK(filler->is_live());
+ }
+
+ SECTION("Unused tail slots preserve field insertion order")
+ {
+ HTTPHdr hdr;
+ hdr.create(HTTPType::RESPONSE);
+ ts::PostScript cleanup([&]() -> void { hdr.destroy(); });
+
+ MIMEField *deleted = add_field(hdr, "X-Deleted");
+ MIMEField *kept = add_field(hdr, "X-Kept");
+
+ hdr.field_delete(deleted, false);
+ MIMEField *appended = add_field(hdr, "X-Appended");
+
+ CHECK(appended != deleted);
+ CHECK(mime_hdr_field_slotnum(hdr.m_mime, kept) <
mime_hdr_field_slotnum(hdr.m_mime, appended));
+ }
Review Comment:
This passes on master and fails here -- `field_find` returns the newly added
field, slot 15 vs slot 0.
```suggestion
}
SECTION("A reused earlier slot preserves same-name field order")
{
HTTPHdr hdr;
hdr.create(HTTPType::RESPONSE);
ts::PostScript cleanup([&]() -> void { hdr.destroy(); });
MIMEField *victim = add_field(hdr, "X-Victim");
for (unsigned index = 1; index < MIME_FIELD_BLOCK_SLOTS - 1; ++index) {
add_field(hdr, "X-Filler-" + std::to_string(index));
}
MIMEField *first = add_field(hdr, "Set-Cookie");
REQUIRE(hdr.m_mime->m_first_fblock.m_freetop == MIME_FIELD_BLOCK_SLOTS);
hdr.field_delete(victim, false);
MIMEField *second = add_field(hdr, "Set-Cookie");
CHECK(hdr.field_find("Set-Cookie") == first);
CHECK(mime_hdr_field_slotnum(hdr.m_mime, first) <
mime_hdr_field_slotnum(hdr.m_mime, second));
}
```
##########
src/proxy/hdrs/MIME.cc:
##########
@@ -1338,21 +1359,63 @@ mime_field_init(MIMEField *field)
field->m_wks_idx = -1;
}
+static void
+mime_hdr_rebuild_field_free_list(MIMEHdrImpl *mh)
+{
+ MIMEField *free_field = nullptr;
+ int32_t slotnum = 0;
+
+ mh->m_free_slot = MIME_FIELD_FREE_SLOT_NONE;
+ for (MIMEFieldBlockImpl *fblock = &mh->m_first_fblock; fblock != nullptr;
fblock = fblock->m_next) {
+ for (uint32_t index = 0; index < fblock->m_freetop; ++index) {
+ MIMEField *field = &fblock->m_field_slots[index];
+
+ if (field->m_readiness == MIME_FIELD_SLOT_READINESS_DELETED ||
field->m_readiness == MIME_FIELD_SLOT_READINESS_EMPTY) {
+ field->m_readiness = MIME_FIELD_SLOT_READINESS_DELETED;
+ field->m_next_dup = free_field;
+ free_field = field;
+ mh->m_free_slot = slotnum + static_cast<int32_t>(index);
+ }
+ }
+ slotnum += MIME_FIELD_BLOCK_SLOTS;
+ }
+}
+
MIMEField *
mime_field_create(HdrHeap *heap, MIMEHdrImpl *mh)
{
MIMEField *field;
MIMEFieldBlockImpl *tail_fblock, *new_fblock;
tail_fblock = mh->m_fblock_list_tail;
- if (tail_fblock->m_freetop >= MIME_FIELD_BLOCK_SLOTS) {
- new_fblock = (MIMEFieldBlockImpl
*)heap->allocate_obj(sizeof(MIMEFieldBlockImpl), HdrHeapObjType::FIELD_BLOCK);
- _mime_hdr_field_block_init(new_fblock);
- tail_fblock->m_next = new_fblock;
- tail_fblock = new_fblock;
- mh->m_fblock_list_tail = new_fblock;
+ if (tail_fblock->m_freetop < MIME_FIELD_BLOCK_SLOTS) {
+ field = &(tail_fblock->m_field_slots[tail_fblock->m_freetop]);
+ ++tail_fblock->m_freetop;
+
+ mime_field_init(field);
+ return field;
+ }
+
+ if (mh->m_free_slot == MIME_FIELD_FREE_SLOT_UNINITIALIZED) {
+ mime_hdr_rebuild_field_free_list(mh);
}
+ if (mh->m_free_slot != MIME_FIELD_FREE_SLOT_NONE) {
+ field = mime_hdr_field_get_slotnum(mh, mh->m_free_slot);
+ ink_release_assert(field != nullptr);
+ ink_release_assert(field->m_readiness ==
MIME_FIELD_SLOT_READINESS_DELETED);
+
+ mh->m_free_slot = field->m_next_dup ? mime_hdr_field_slotnum(mh,
field->m_next_dup) : MIME_FIELD_FREE_SLOT_NONE;
Review Comment:
Each field delete, and each insert that reuses a slot, now traverses the
field block list twice -- once in `mime_hdr_field_get_slotnum` and once in
`mime_hdr_field_slotnum`. The chain is bounded by this change so it's not a
blocker, but may be worth a quick benchmark against alternative approaches.
Copilot flagged this too; its confidence filter suppressed the comment into the
review summary.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
