Skip to content

Cleanup of HdrHeap::HeapGuard and HdrStrHeap classes.#10961

Merged
ywkaras merged 2 commits intoapache:masterfrom
ywkaras:HdrHeapCleanup
Jan 9, 2024
Merged

Cleanup of HdrHeap::HeapGuard and HdrStrHeap classes.#10961
ywkaras merged 2 commits intoapache:masterfrom
ywkaras:HdrHeapCleanup

Conversation

@ywkaras
Copy link
Contributor

@ywkaras ywkaras commented Jan 4, 2024

No description provided.

@ywkaras ywkaras self-assigned this Jan 4, 2024
@ywkaras ywkaras added the Cleanup label Jan 4, 2024
@ywkaras ywkaras added this to the 10.0.0 milestone Jan 4, 2024
@bryancall bryancall added the HTTP label Jan 8, 2024
@bryancall bryancall requested a review from masaori335 January 8, 2024 23:31
m_free_start += nbytes;
m_free_size -= nbytes;
if (_avail_size >= static_cast<unsigned>(nbytes)) {
char *new_space = reinterpret_cast<char *>(this) + _total_size - _avail_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add a function that calculates this pointer with checking values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checking is already present, on line 1181. It results in a nullptr return if the checking fails. The only place this function is called is in HdrHeap::allocate_str(), which does handle the null return value.

case 2:
Warning("HdrHeap=%p coalescing twice", this);
break;
case 3:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you ever seen coalesce become more than 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The original code allowed coalescing to be repeated an unlimited number of times. I think it should only happen once, but I'm not 100% sure, so I didn' t want to add assert to check it. If we see coalescing happening repeatedly, we should try to figure out if it make sense for that to happen.

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like just cleanups.

@ywkaras ywkaras merged commit 46a6570 into apache:master Jan 9, 2024
phongn pushed a commit to phongn/trafficserver that referenced this pull request Feb 1, 2024
* Cleanup of HdrStrHeap class.

* Encapsulate HdrHeap::HeapGuard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants