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

[fix]: try to reduce escapes to heap #1886

Conversation

SamYuan1990
Copy link
Collaborator

https://github.com/sustainable-computing-io/kepler/actions/runs/12177539929/job/33966954289
seems we have many nits can be improved to reduce our memory escapes to heap case.
in this PR, try to improve parts of them.

Copy link
Contributor

github-actions bot commented Dec 6, 2024

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This PR focuses on internal optimizations to reduce memory escapes to the heap, improving code efficiency without affecting the external interface or behavior.

Key Modifications:

  • Refactored functions: NewUInt64Stat, getIntConfig, getModelOutputTypeConverter, getModelTypeConverter, ReadMSR, and init
  • Introduced global variables
  • Optimized internal implementation details

Impact: These changes aim to improve memory usage and efficiency, making the code more performant without altering its external functionality.

Observations: The PR appears to be a well-contained set of internal optimizations, which is a good practice for maintaining a healthy codebase. However, it would be beneficial to include more detailed comments or documentation explaining the reasoning behind these changes, especially for future maintainers who may not be familiar with the original motivation.

@@ -14,10 +14,10 @@ type UInt64Stat struct {
}

func NewUInt64Stat(aggr, delta uint64) *UInt64Stat {
stat := &UInt64Stat{}
stat := UInt64Stat{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -234,7 +234,7 @@ func getBoolConfig(configKey string, defaultBool bool) bool {
}

func getIntConfig(configKey string, defaultInt int) int {
defaultValue := fmt.Sprintf("%d", defaultInt)
defaultValue := strconv.Itoa(defaultInt)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -57,18 +57,16 @@ var (
LogisticTrainer,
ExponentialTrainer,
}
ModelOutputTypeConverter = []string{"AbsPower", "DynPower"}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Error: pkg/model/types/types.go:63:17: []string{...} escapes to heap
Error: pkg/model/types/types.go:69:1[7](https://github.com/sustainable-computing-io/kepler/actions/runs/12177539929/job/33966954289#step:4:8): []string{...} escapes to heap

fixed

@SamYuan1990 SamYuan1990 force-pushed the tryToreduceescapestoHeap branch from a8104f8 to d161bba Compare December 6, 2024 10:49
@@ -50,6 +51,8 @@ func init() {
port = "10250"
}
podURL = "https://" + nodeName + ":" + port + "/pods"
http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
client = http.Client{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Error: pkg/kubelet/kubelet_pod_lister.go:69:12: &http.Client{} escapes to heap
make http client reuse

@SamYuan1990 SamYuan1990 marked this pull request as ready for review December 6, 2024 11:32
@SamYuan1990 SamYuan1990 enabled auto-merge (squash) December 6, 2024 11:33
@SamYuan1990 SamYuan1990 merged commit c9524a8 into sustainable-computing-io:main Dec 10, 2024
23 checks passed
@rootfs rootfs requested a review from vimalk78 December 10, 2024 16:34
@rootfs
Copy link
Contributor

rootfs commented Dec 10, 2024

lgtm, @vimalk78 can you take a final check? thanks

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

Successfully merging this pull request may close these issues.

3 participants