libct/cg/fs: getPageUsageByNUMA: rewrite/optimize, fix panic, add more tests#2755
Merged
Conversation
strings.SplitN not always return N fields if not staify, sometimes cgroup interface add some custom fields make parse memory.numa_stat fails, it will case panic Signed-off-by: acetang <aceapril@126.com>
Contributor
Author
|
CI failure is unrelated (flaky nsexec debug logging, see #2756). Restarted. |
dqminh
previously approved these changes
Jan 20, 2021
| // first item was already validated, | ||
| // so be strict to the rest | ||
| return stats, fmt.Errorf("malformed line %q in %s", | ||
| scanner.Text(), filename) |
Contributor
There was a problem hiding this comment.
let's captured the line before and reuse here, scanner.Text() effectively convert []byte to string, which afaik is a little costly, even though it doesn't matter that much in practice because this is error case.
Contributor
Author
There was a problem hiding this comment.
Right, I thought about it and figured out optimizing the error case makes little sense.
Anyway, fixed, and I also made a few tiny improvements (better comments, use break instead of continue nextline). Please re-lgtm :)
Rewrite getPageUsageByNUMA 1. Be less strict to unknown contents, i.e. skip it. This makes the function more future-proof. Before this commit, if a line like "a=b" is encountered, the function returns an error, which is propagated all the way up to and returned by (CgroupManager).GetStats. 2. Be more strict to contents it recognizes, i.e. return an error. In case the first field in the line is recognized (e.g. "total=123", the rest of the line should be in format "N<id>=<value> ...". 3. Optimize. Before this commit, addNUMAStatsByType was called for every item in the line, which is excessive and might even be slow in case there are many NUMA nodes. It is enough to look up the field once. 4. Remove a bunch of global numaNode* and numaStat* constants. Those were used by only one function, and it does not make sense to have them defined globally. Some were moved to the function, some were eliminated entirely. 5. Improve readability and added code comments. Finally, add some test cases for good and bad contents. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It was already explained why we ignore the error, so let's ignore this deliberately. This fixes > name.go:22:7: Error return value of `join` is not checked (errcheck) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
changed the title
dqminh
approved these changes
Jan 22, 2021
AkihiroSuda
approved these changes
Feb 1, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a rewrite of getPageUsageByNUMA, fixing a number of issues.
Fix a panic for lines that do not contain
=(first commit, by @Ace-Tang).Be less strict to unknown contents, i.e. skip it. This makes the
function more future-proof. Before this commit, if a line like
"a=b" is encountered, the function returns an error, which is
propagated all the way up to and returned by (CgroupManager).GetStats.
Be more strict to contents it recognizes, i.e. return an error.
In case the first field in the line is recognized (e.g. "total=123",
the rest of the line should be in format "N= ...".
Optimize. Before this commit, addNUMAStatsByType was called for every
item in the line, which is excessive and might even be slow in case
there are many NUMA nodes. It is enough to look up the field once.
This probably helps to reduce the load on garbage collector, too.
Remove a bunch of global numaNode* and numaStat* constants. Those
were used by only one function, and it does not make sense to have
them defined globally. Some were moved to the function, some were
eliminated entirely.
Improve readability and added code comments.
Finally, add some test cases for good and bad contents.
NOTE this PR includes #2751.
Closes: #2751