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

scheduler: only add allocs to network and device trackers if required #9286

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nickethier
Copy link
Member

There are two structs used in in scheduling to track usage of networks and devices. While they are only used if an alloc requires a network or device, they always are initialized regardless. In many cases this initialization overhead is minimal. However, in cases where the cardinality of allocs assigned to ranked nodes are high, memory and cpu cycles are wasted accounting the devices and networks used by these allocs if that accounting is never used to rank the node, e.g. the proposed allocation doesn't require a network or device.

This PR moves the initialization to a function that is called by sync.Once when needed. We do call netIdx.Release() always so I made sure that accepted a nil receiver without panicing.

Comment on lines +206 to +212
var netIdx *structs.NetworkIndex
initNetIdx := func() {
netIdx = structs.NewNetworkIndex()
netIdx.SetNode(option.Node)
netIdx.AddAllocs(proposed)
}
var initNetIdxOnce sync.Once
Copy link
Member

Choose a reason for hiding this comment

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

I like think the functionality is right, but let's rework this not require a closure or a sync.Once. Since netIdx is never shared between goroutines a bool or if netIdx == nil { init() } check is sufficient.

However, I want us to at least consider refactoring NetworkIndex instead before merging because like many of networking structs it's used for slightly different purposes in slightly different places.

Here NetworkIndex should lazily evaluate SetNode+AddAllocs only when AssignPorts is called.

The collision detection logic that requires immediate (non-lazy) evaluation is only used in the planner.

Perhaps SetNode+AddAllocs could be made lazy (just setters that copy the pointer they're passed; probably inlined by the compiler), and a new Check() (ok bool) method could be added?

At any rate, this is great work and the refactorings may not pan out, but I think pushing a bit further on it could make it more readable and perhaps even a teeny tiny bit more efficient? That may not be true since I think in the no-networks path your approach will incur 0 heap allocations.

@nickethier nickethier force-pushed the f-optimize-ranker-netidx branch from cf86bae to 0fb560e Compare November 13, 2020 17:29
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 13, 2020

CLA assistant check
All committers have signed the CLA.

@nickethier nickethier force-pushed the f-optimize-ranker-netidx branch from 0fb560e to 7342826 Compare November 13, 2020 17:32
Base automatically changed from master to main March 8, 2021 19:25
@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants