Regexp tidy up #978
Regexp tidy up #978
Conversation
| @@ -172,9 +172,12 @@ func (self *dockerFactory) DebugInfo() map[string][]string { | |||
| return map[string][]string{} | |||
| } | |||
|
|
|||
| var ( | |||
| version_regexp_string = "(\\d+)\\.(\\d+)\\.(\\d+)" | |||
| version_re = regexp.MustCompile(version_regexp_string) | |||
There was a problem hiding this comment.
Why move this out of the function? Personally I find it more readable if I can see the regexp right where it's used (and I don't think this should be used outside that function).
There was a problem hiding this comment.
Putting it in the function means the RE gets wastefully compiled every time the function is called. In this case probably not called more than once, but normally best to move it outside so trying to keep it consistent.
There was a problem hiding this comment.
Ah, makes sense. Maybe move the declaration to just above the function in that case? Also, consider making the string const.
| tcpStateMap[state[3]]++ | ||
|
|
||
| state := strings.Fields(line) | ||
| //#file header tcp state is the 4th field: |
There was a problem hiding this comment.
Also this comment reads a little weird. Maybe rephrase to:
// TCP state is the 4th field.
// Format: sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode
|
@timstclair Thanks for a thorough code review - this is making it a much more worthwhile endeavour than it would have otherwise been :) |
| disk_info.Scheduler = "none" | ||
| blkSched, err := sysfs.GetBlockDeviceScheduler(name) | ||
| if err == nil { | ||
| matches := schedulerRegExp.FindSubmatch([]byte(blkSched)) |
There was a problem hiding this comment.
This regex should use a raw string too.
No problem! Thanks for the cleanup! |
|
All comments addressed. Good review! |
|
As an observation there is a lot we could do to lower resource utilisation. Regexps are hungry & need to be used judiciously. They're overused here where we could probably do some cheaper string parsing. |
|
Integration tests pass, so it must be good right? 😜 |
| @@ -157,6 +160,7 @@ func GetTopology(sysFs sysfs.SysFs, cpuinfo string) ([]info.Node, int, error) { | |||
| lastNode = -1 | |||
| } | |||
| lastThread = thread | |||
| continue | |||
There was a problem hiding this comment.
This whole loop could use some cleanup. I think it would be much more efficient and readable to switch on strings.TrimSpace(line[:strings.Index(line, ":")-1])
That's sort of a separate change, so up to you where / whether you want to address it.
There was a problem hiding this comment.
Agreed. Not going to do it here, luckily AFAICT this is only called once on start of cadvisor so efficiency isn't paramount. Would like to improve it though, for vanity more than anything else :-b
|
LGTM (please squash)
Of course! I always like to have some simple unit tests for regexes, but I won't force you to add them if you aren't up for it...
Agreed. Although I don't think it's worth spending too much time on this without first doing some profiling to identify the places it really matters. |
|
Yeah we need to do more profiling of cadvisor in general. There have been quite a few performance issues when embedded in the kubelet & we need to identify areas for improvement. Regexes were a recurrent theme in those perf problems, hence me looking at them here. I think with the work we've done recently in cadvisor this is improved significantly, but further work needs to be done to keep cadvisor's footprint as low as possible. |
jimmidyson
force-pushed
the
regexp-perf
branch
from
0610d03 to
d1fce20
Compare
|
retest this please |
|
Self-merging after LGTM. |
Cleaned up some areas where regexps were either messy or unnecessary.