only expand resources when strictly needed #5791
Conversation
|
Test here: https://github.com/openshift/origin/blob/master/test/integration/authorization_test.go#L137-L174 ensures expansion still happens. |
| // NormalizeResources expands all resource groups and forces all resources to lower case. | ||
| // If the rawResources are already normalized, it returns the original set to avoid the | ||
| // allocation and GC cost, since this is hit multiple times for every REST call. | ||
| // That means you should NEVER MODIFY THE RESULT of this call. |
There was a problem hiding this comment.
clarify you can get the original back... if you were comfortable modifying the original, you can modify the result
|
now with more ugly |
| // strings can be cast to []byte for free. We range over the bytes to avoid | ||
| // the conversion to runes that happens when ranging over strings. This results in | ||
| // zero allocations here. | ||
| for _, currByte := range []byte(in) { |
There was a problem hiding this comment.
I started weaving my way through string ranging and unicode.IsLower() to convince myself that it was cheap for ascii strings... a isLowerString(s string) would make me happier, but I hadn't proven it to myself yet
There was a problem hiding this comment.
I started weaving my way through string ranging and unicode.IsLower() to convince myself that it was cheap for ascii strings... a isLowerString(s string) would make me happier, but I hadn't proven it to myself yet
Given the pain of analysis, is it worth it? We all wrote this once upon a time in school, so it may look stupid, but it's easy to read, it's never a false negative, and it doesn't require delving into the depths of runes (that no one else will ever read).
There was a problem hiding this comment.
yeah, this should work:
func isLower(s string) bool {
for _, r := range s {
if !unicode.IsLower(r) {
return false
}
}
return true
}edit: sorry, condition was backwards... it was the memory allocation patterns of IsLower() I was interested in
There was a problem hiding this comment.
edit: sorry, condition was backwards... it was the memory allocation patterns of IsLower() I was interested in
Future-david and future-jordan both hate us.
There was a problem hiding this comment.
edit: sorry, condition was backwards... it was the memory allocation patterns of IsLower() I was interested in
Alright, since you've dug deep and I haven't. Does it think '/', '-', or '.' are lowers of anything?
deads2k
force-pushed
the
fix-gc
branch
2 times, most recently
from
b857565 to
1f25e90
Compare
|
[test] |
|
@liggitt bump |
|
bump |
|
updated. It still looks insane. |
| @@ -38,6 +62,19 @@ func ExpandResources(rawResources sets.String) sets.String { | |||
| return ret | |||
| } | |||
|
|
|||
| func needsNormalizing(in string) bool { | |||
There was a problem hiding this comment.
sorry again...
func needsNormalizing(in string) bool {
if strings.HasPrefix(in, ResourceGroupPrefix) {
return true
}
for _, r := range in {
if unicode.IsUpper(r) {
return true
}
}
return false
}
0afbe46 to
8c5d174
Compare
|
LGTM, [merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4277/) (Image: devenv-rhel7_2911) |
|
Evaluated for origin test up to c076be3 |
|
Evaluated for origin merge up to c076be3 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7702/) |
Related to #5737.
This only allocates new sets for resources if the set of resources needs expansion. Since we no longer use resource groups by default, this should alleviate most allocations.