Skip to content

Conversation

Maoni0
Copy link
Member

@Maoni0 Maoni0 commented Jun 13, 2024

since we want to enable this, I needed to add necessary diag info in the events.

will be doing some testing later today.

@ghost ghost added the area-GC-coreclr label Jun 13, 2024
@Maoni0 Maoni0 requested a review from cshung June 13, 2024 09:33
@Maoni0
Copy link
Member Author

Maoni0 commented Jun 13, 2024

changes in gc.h are temporary and will be reverted.

temp_change = 0x0020
};

bool should_change (float tcp, float* tcp_to_consider, size_t current_gc_index,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we put the implementation of all these functions in the header file?

Copy link
Member Author

Choose a reason for hiding this comment

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

just a personal choice, I found it easier to look at them side by side (so have gcpriv.h in one window and gc.cpp with the callersites in a different window) since I often look at them together. it's easier than having to scroll around in one file.

@mrsharm mrsharm self-requested a review June 17, 2024 23:06
Copy link
Member

@mrsharm mrsharm left a comment

Choose a reason for hiding this comment

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

LGTM - only comment I have is the same stylistic one as what @markples does re: adding another space to the TRACE_GC and SIMPLE_DPRINTF.

@Maoni0
Copy link
Member Author

Maoni0 commented Jun 17, 2024

reverted the white space only changes.

dec_multiple = 0x0004,
fluctuation = 0x0008
};

Copy link
Contributor

Choose a reason for hiding this comment

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

can remove the clutter from the code with something like

inline hc_change_freq_reason operator| (hc_change_freq_reason _a, hc_change_freq_reason _b) {
    return static_cast<hc_change_freq_reason>(static_cast<int>(_a) | static_cast<int>(_b));
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants