-
Notifications
You must be signed in to change notification settings - Fork 12
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
Don't exceed maximum resources #16
base: master
Are you sure you want to change the base?
Don't exceed maximum resources #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR has been updated to match master
.
-- The resource count in 'poolMaxResources' will be split evenly among | ||
-- the available stripes. If there are fewer stripes than resources, then | ||
-- this will only create as many stripes as resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code currently on master
will treat this case as a programmer error
. I feel like this is a bad state, and fewer error
calls would be preferable. This PR clamps the value such that maxResources
is always less than or equal to numStripes
, preferring to have more stripes with fewer resources each.
I feel like error
is worse than value clamping, but neither are great.
Prior to this PR, the max resources
parameter was not really establishing a "maximum count of resources." This PR makes it so that a Pool
never has more resources than the maxResources
parameter.
Alternatively, we could replace poolMaxResources
with poolResourcesPerStripe
. This has an unambiguous meaning, and could really simplify the code. The place where we currently calculate can be relocated to the deprecated createPool
, which would determine (given a numStripes
and maxResources
) a way to have resourcesPerStripe * numStripes <= maxResources
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing as the number of capabilities is a runtime environment-specific detail that the programmer cannot know in advance, I think replacing poolMaxResources
with poolResourcesPerStripe
makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to look at this any further, but I did wonder if it wouldn't be a pain in the ass to make the resource limits more expressive, something like
-- | Limits can be expressed in two different ways.
--
-- To set the stage, it's better for performance to have individual pools for
-- each capability (i.e., physical CPU) available to the program. This is
-- accomplished with "stripes".
--
-- Some resources, like database connections, are relatively cheap to hold on
-- to. In that case it makes sense to size the pool by choosing the size of
-- the stripes. You can specify number of stripes, but the default is the number
-- of capabilities, which is sensible.
--
-- Other resources (example would be nice?) are expensive to hold on to, in which
-- case you may want to limit their absolute number __across__ all stripes. You
-- can still specify the number of stripes, but only as a maximum. The actual
-- number of stripes will be the min of maxResources and maxStripes.
--
-- To match behavior of resource-pool <4, use @LimitStripeSize@.
data ResourceLimits
= LimitStripeSize
{ maxStripeSize :: Int
, maxStripes :: Maybe Int
}
| LimitTotalResources
{ maxResources :: Int
, maxStripes :: Maybe Int
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, this PR seems like an improvement over the status quo, and this whole discussion here could be inspiration for a followup. (I'm not asking for changes.)
Co-authored-by: ruicc <[email protected]>
-- The resource count in 'poolMaxResources' will be split evenly among | ||
-- the available stripes. If there are fewer stripes than resources, then | ||
-- this will only create as many stripes as resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears swapped.
-- The resource count in 'poolMaxResources' will be split evenly among | |
-- the available stripes. If there are fewer stripes than resources, then | |
-- this will only create as many stripes as resources. | |
-- The resource count in 'poolMaxResources' will be split evenly among | |
-- the available stripes. If there are fewer resources than stripes, then | |
-- this will only create as many stripes as resources. |
@arybczak any chance to have a look at this? |
This PR builds on #15
Fixes #13
This PR has two main behavior changes:
numStripes
is greater thanmaxResources
, then we only createmaxResources
of stripes, each receiving a single resource.numStripes
does not evenly dividemaxResources
, then resources are divided among stripes such that no stripe has two more resources than any other stripe.With these two changes,
poolMaxResources
is guaranteed to be a true maximum of resources.