Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

event changes for DATAS #103405

Merged
merged 3 commits into from
Jun 19, 2024
Merged

event changes for DATAS #103405

merged 3 commits into from
Jun 19, 2024

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.

@Maoni0
Copy link
Member Author

Maoni0 commented Jun 13, 2024

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

src/coreclr/gc/gc.h Outdated Show resolved Hide resolved
temp_change = 0x0020
};

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

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.

src/coreclr/gc/gc.h Outdated Show resolved Hide resolved
@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
Member

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));
}

@Maoni0 Maoni0 merged commit 20fc99b into dotnet:main Jun 19, 2024
85 of 88 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
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