Skip to content

mem optimization #277

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

mem optimization #277

wants to merge 5 commits into from

Conversation

asm582
Copy link
Member

@asm582 asm582 commented Feb 22, 2023

No description provided.

Copy link
Collaborator

@dmatch01 dmatch01 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Will also need some logic that will fail the AW if the items field is used from this PR point forward.

@@ -310,6 +310,9 @@ func (gr *GenericResources) SyncQueueJob(aw *arbv1.AppWrapper, awr *arbv1.AppWra
}
}
}
//force nil so that Go can GC
unstruct.Object = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand this logic? Can you provide more context?

Comment on lines +381 to +385
//force nil so that Go can GC
m = nil
spec = nil
template = nil
marshal = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I understand this logic? Can you provide more context?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't this variables stack allocated? If yes, they will be GC-ed once the stack is cleaned up.

Comment on lines +498 to +521
var replicasFromCustomPodResources int
if awr.GenericTemplate.Raw != nil {
hasContainer, replicas, containers := hasFields(awr.GenericTemplate)
//Turning off logic to extract resource requirements from containers.
//hasContainer, replicas, containers := hasFields(awr.GenericTemplate)
hasContainer := false
if hasContainer {
// Add up all the containers in a pod
for _, container := range containers {
res := getContainerResources(container, 1)
podTotalresource = podTotalresource.Add(res)
}
// for _, container := range containers {
// res := getContainerResources(container, 1)
// podTotalresource = podTotalresource.Add(res)
// }
klog.V(8).Infof("[GetListOfPodResourcesFromOneGenericItem] Requested total pod allocation resource from containers `%v`.\n", podTotalresource)
} else {
podresources := awr.CustomPodResources
for _, item := range podresources {
replicasFromCustomPodResources += item.Replicas
res := getPodResources(item)
podTotalresource = podTotalresource.Add(res)
}
klog.V(8).Infof("[GetListOfPodResourcesFromOneGenericItem] Requested total allocation resource from 1 pod `%v`.\n", podTotalresource)
}

// Addd individual pods to results
var replicaCount int = int(replicas)
var replicaCount int = replicasFromCustomPodResources
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably need a walk through of this logic.

@@ -629,12 +630,12 @@ func (qjrPod *QueueJobResPod) createQueueJobPod(qj *arbv1.AppWrapper, ix int32,
templateObjMetadata.SetNamespace(qj.Namespace)
templateObjMetadata.SetOwnerReferences([]metav1.OwnerReference{
*metav1.NewControllerRef(qj, queueJobKind),
},)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was the , intentionally removed?

Replicas: 1,
Type: arbv1.ResourceTypeDeployment,
Template: runtime.RawExtension{
DesiredAvailable: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Replicas still apply here.

Replicas: 1,
Type: arbv1.ResourceTypeDeployment,
Template: runtime.RawExtension{
DesiredAvailable: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Replicas still apply here.

Replicas: 1,
Type: arbv1.ResourceTypeDeployment,
Template: runtime.RawExtension{
DesiredAvailable: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Replicas still apply here.

Replicas: 1,
Type: arbv1.ResourceTypeDeployment,
Template: runtime.RawExtension{
DesiredAvailable: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Replicas still apply here.

Replicas: 1,
Type: arbv1.ResourceTypeNamespace,
Template: runtime.RawExtension{
DesiredAvailable: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Replicas still apply here.

Replicas: 1,
Type: arbv1.ResourceTypeStatefulSet,
Template: runtime.RawExtension{
DesiredAvailable: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Replicas still apply here.

Copy link
Contributor

@z103cb z103cb left a comment

Choose a reason for hiding this comment

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

Setting things explicitly to nil is not needed and confusing.

Comment on lines +381 to +385
//force nil so that Go can GC
m = nil
spec = nil
template = nil
marshal = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't this variables stack allocated? If yes, they will be GC-ed once the stack is cleaned up.

// for _, container := range containers {
// res := getContainerResources(container, 1)
// podTotalresource = podTotalresource.Add(res)
// }
klog.V(8).Infof("[GetListOfPodResourcesFromOneGenericItem] Requested total pod allocation resource from containers `%v`.\n", podTotalresource)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dead code in the new logic, can't it be removed altogether ?

@@ -617,6 +628,8 @@ func (gr *GenericResources) IsItemCompleted(awgr *arbv1.AppWrapperGenericResourc
klog.Errorf("[IsItemCompleted] mapping error from raw object: `%v`", err)
return false
}
//Force nil for go GC
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed.

@@ -550,7 +550,8 @@ func (qjrPod *QueueJobResPod) GetPodTemplate(qjobRes *arbv1.AppWrapperResource)
if !ok {
return nil, fmt.Errorf("Job resource template item not define as a PodTemplate")
}

//Force nil for go GC
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed.

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