🌈 optimize(victim task):remove unused copy #2607
Conversation
volcano-sh-bot
added
the
size/S
|
/assign @shinytang6 |
| // different filters may add the same task to victims, so use a map to remove duplicate tasks. | ||
| victimSet := make(map[*api.TaskInfo]bool) | ||
| victimSet := make(map[*api.TaskInfo]struct{}) |
There was a problem hiding this comment.
It doesn't make much sense to do this. I read an article before, but I forgot the source. The author of the article did some tests, so it doesn't save much memory space. It also makes the code less readable.
There was a problem hiding this comment.
It doesn't make much sense to do this. I read an article before, but I forgot the source. The author of the article did some tests, so it doesn't save much memory space. It also makes the code less readable.
Actually, changing value type from bool to struct{} is not for memory save, but because the definition confused me when I first read these codes.
From the code we know that victimSet is just used to deduplicate the victim tasks, no other uses.
If we define it as map[*api.TaskInfo]struct{} ,we can determine directly from the defination that victimSet is just used for deduplication.
But if we define it as map[*api.TaskInfo]bool, which means that the value may be true or false. Function Session.VictimTasks collects victimTask by victimSet[victim] = true, which has no difference with victimSet[victim] = false, because the map value never been checked.
There was a problem hiding this comment.
A boolean value can indicate the presence or absence of.
There was a problem hiding this comment.
A boolean value can indicate the presence or absence of.
But in your codes , the boolean map value is never checked.
And in your codes , there is no code to set the map value to false.
vimtimSet is a map, if task in map, means presence; if not, means absence.
There was a problem hiding this comment.
Considering this point, I agree with @hwdef that it seems to make less sense to make the value to be an empty struct object.
There was a problem hiding this comment.
Considering this point, I agree with @hwdef that it seems to make less sense to make the value to be an empty struct object.
Has restore back to bool.
| for task := range victimTasksMap { | ||
| victimTasks = append(victimTasks, task) | ||
| } | ||
| for _, victim := range victimTasks { |
There was a problem hiding this comment.
@kingeasternsun Thanks for the optimization. I approve this patch.
| // different filters may add the same task to victims, so use a map to remove duplicate tasks. | ||
| victimSet := make(map[*api.TaskInfo]bool) | ||
| victimSet := make(map[*api.TaskInfo]struct{}) |
There was a problem hiding this comment.
Considering this point, I agree with @hwdef that it seems to make less sense to make the value to be an empty struct object.
volcano-sh-bot
added
size/XS
There was a problem hiding this comment.
@kingeasternsun It' OK for me now. Thanks for your contribution. Please rebase all the commits into one and I'll try to merge it.
Signed-off-by: kingeasternsun <kingeasternsun@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
size/L
Because my Incorrect operation， the commits include a merge, when I |
Signed-off-by: kingeasternsun kingeasternsun@gmail.com
stmt.Evict, so there is no need to convertvictimTasksMapto Slice