You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2021/06/01 19:25:33 UTC
[GitHub] [mesos] cf-natali opened a new pull request #391: Fixed parsing of `perf` output on some locales.
cf-natali opened a new pull request #391:
URL: https://github.com/apache/mesos/pull/391
If the locale is such that `LC_NUMERIC` uses the comma ',' as decimal
separator, parsing floating point fields won't work, so make sure it's
set to `C`.
Example:
```
[ RUN ]
CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_PERF_PerfTest
../../src/tests/containerizer/cgroups_tests.cpp:1024: Failure
(statistics).failure(): Failed to parse perf sample: Failed to parse
perf sample line
'6376827291,,cycles,mesos_test,2011741096,100,00,3,GHz': Unexpected
number of fields (9)
[ FAILED ]
CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_PERF_PerfTest (2157
ms)
```
Standalone separator, using '/' as separator for readability:
```
root@thinkpad:~# LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/"
-- true
0,31/msec/task-clock/306721/100,00/0/CPUs utilized
0//context-switches/306721/100,00/0/K/sec
0//cpu-migrations/306721/100,00/0/K/sec
44//page-faults/306721/100,00/0/M/sec
788234//cycles/311478/100,00/2/GHz
538077//instructions/311478/100,00/0/insn per cycle
106749//branches/311478/100,00/348/M/sec
4556//branch-misses/311478/100,00/4/of all branches
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali edited a comment on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-854051032
> BTW, I see in the output of your test, the number of tokens is 7 (e.g. 788234//cycles/311478/100,00/2/GHz) which should fail in this code since there is no case 7, right?
Yes this example wouldn't work anyway but that's because it doesn't specify the cgroup, whereas the actual code does (https://github.com/apache/mesos/blob/50b729904e4b4ed7964cd5f2ac8177bca5a5a72c/src/linux/perf.cpp#L288):
No `--cgroup` - 7 columns:
```
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles -- true
1987580//cycles/3437534/100.00//
```
`--cgroup` - 8 columns:
```
root@thinkpad:~# mkdir /sys/fs/cgroup/perf_event/test
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles --cgroup test -- true
<not counted>//cycles/test/0/100.00//
root@thinkpad:~#
```
This MR definitely fixes the test :):
```
root@thinkpad:/home/cf/src/mesos/build# LC_ALL=fr_FR.utf8 ./bin/mesos-tests.sh --gtest_filter="*CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_PERF_PerfTest*"
[...]
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from CgroupsAnyHierarchyWithPerfEventTest
[ RUN ] CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_PERF_PerfTest
[ OK ] CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_PERF_PerfTest (2227 ms)
[----------] 1 test from CgroupsAnyHierarchyWithPerfEventTest (2227 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (2241 ms total)
[ PASSED ] 1 test.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-854051032
> BTW, I see in the output of your test, the number of tokens is 7 (e.g. 788234//cycles/311478/100,00/2/GHz) which should fail in this code since there is no case 7, right?
Yes this example wouldn't work anyway but that's because it doesn't specify the cgroup, whereas the actual code does (https://github.com/apache/mesos/blob/50b729904e4b4ed7964cd5f2ac8177bca5a5a72c/src/linux/perf.cpp#L288):
```
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles -- true
1987580//cycles/3437534/100.00//
```
```
root@thinkpad:~# mkdir /sys/fs/cgroup/perf_event/test
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles --cgroup test -- true
<not counted>//cycles/test/0/100.00//
root@thinkpad:~#
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali edited a comment on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853254919
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853916168
> It's also possible you don't have the `fr_FR.UTF-8` locale generated, in which case it wouldn't work.
Yeah, it's because that I did not have the `fr_FR.UTF-8` locale generated, now it works as expected. Thank you!
BTW, I see in the output of your test, the number of tokens is 7 (e.g. `788234//cycles/311478/100,00/2/GHz`) which should fail in [this code](https://github.com/apache/mesos/blob/1.11.0/src/linux/perf.cpp#L364:L400) since there is no case 7, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-854755987
> Yes this example wouldn't work anyway but that's because it doesn't specify the cgroup, whereas the actual code does (
Aha, I see `perf::sample` will be a no-op if no cgroup is specified:
```
Future<hashmap<string, mesos::PerfStatistics>> sample(
const set<string>& events,
const set<string>& cgroups,
const Duration& duration)
{
// Is this a no-op?
if (cgroups.empty()) {
return hashmap<string, mesos::PerfStatistics>();
}
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali edited a comment on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853254919
> When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like:
> `0//cpu-migrations/779898/100.00/0.000/K/sec` which is different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`).
Might be because `LC_ALL` is set on your machine - try running:
```
# LC_ALL=fr_FR.UTF-8 perf stat --field-separator "/" -- true
```
instead?
It's also possible you don't have the `fr_FR.UTF-8` locale generated, in which case it wouldn't work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853135728
> root@thinkpad:~# LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/"
> -- true
> 0,31/msec/task-clock/306721/100,00/0/CPUs utilized
> 0//context-switches/306721/100,00/0/K/sec
> 0//cpu-migrations/306721/100,00/0/K/sec
> 44//page-faults/306721/100,00/0/M/sec
> 788234//cycles/311478/100,00/2/GHz
> 538077//instructions/311478/100,00/0/insn per cycle
> 106749//branches/311478/100,00/348/M/sec
> 4556//branch-misses/311478/100,00/4/of all branches
When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like:
`0//cpu-migrations/779898/100.00/0.000/K/sec` which seems different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali edited a comment on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-854051032
> BTW, I see in the output of your test, the number of tokens is 7 (e.g. 788234//cycles/311478/100,00/2/GHz) which should fail in this code since there is no case 7, right?
Yes this example wouldn't work anyway but that's because it doesn't specify the cgroup, whereas the actual code does (https://github.com/apache/mesos/blob/50b729904e4b4ed7964cd5f2ac8177bca5a5a72c/src/linux/perf.cpp#L288):
No `--cgroup` - 7 columns:
```
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles -- true
1987580//cycles/3437534/100.00//
```
`--cgroup` - 8 columns:
```
root@thinkpad:~# mkdir /sys/fs/cgroup/perf_event/test
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles --cgroup test -- true
<not counted>//cycles/test/0/100.00//
root@thinkpad:~#
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a change in pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on a change in pull request #391:
URL: https://github.com/apache/mesos/pull/391#discussion_r644077753
##########
File path: src/linux/perf.cpp
##########
@@ -125,6 +125,12 @@ class Perf : public Process<Perf>
private:
void execute()
{
+ // If the locale is such that `LC_NUMERIC` uses the comma ',' as decimal
+ // separator, parsing won't work - because of unexpected number of fields
+ // and floating points format - so make sure it's set to `C`.
+ std::map<string, string> env = os::environment();
+ env["LC_ALL"] = "C";
Review comment:
So here we set `LC_ALL` to `C` to make sure the output of `perf stat` always has `.` as the decimal
separator (e.g., `100.00` instead of `100,00`) regardless user's locale, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-852388266
@asekretenko @qianzhangxa
It's really a corner case but it's causing one of the tests to fail on my box, and I'm trying to get them all to pass...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-854051032
> BTW, I see in the output of your test, the number of tokens is 7 (e.g. 788234//cycles/311478/100,00/2/GHz) which should fail in this code since there is no case 7, right?
Yes this example wouldn't work anyway but that's because it doesn't specify the cgroup, whereas the actual code does (https://github.com/apache/mesos/blob/50b729904e4b4ed7964cd5f2ac8177bca5a5a72c/src/linux/perf.cpp#L288):
```
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles -- true
1987580//cycles/3437534/100.00//
```
```
root@thinkpad:~# mkdir /sys/fs/cgroup/perf_event/test
root@thinkpad:~# perf stat --field-separator "/" --all-cpus -e cycles --cgroup test -- true
<not counted>//cycles/test/0/100.00//
root@thinkpad:~#
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853916168
> It's also possible you don't have the `fr_FR.UTF-8` locale generated, in which case it wouldn't work.
Yeah, it's because that I did not have the `fr_FR.UTF-8` locale generated, now it works as expected. Thank you!
BTW, I see in the output of your test, the number of tokens is 7 (e.g. `788234//cycles/311478/100,00/2/GHz`) which should fail in [this code](https://github.com/apache/mesos/blob/1.11.0/src/linux/perf.cpp#L364:L400) since there is no case 7, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] asfgit closed pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #391:
URL: https://github.com/apache/mesos/pull/391
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #391:
URL: https://github.com/apache/mesos/pull/391#discussion_r644189780
##########
File path: src/linux/perf.cpp
##########
@@ -125,6 +125,12 @@ class Perf : public Process<Perf>
private:
void execute()
{
+ // If the locale is such that `LC_NUMERIC` uses the comma ',' as decimal
+ // separator, parsing won't work - because of unexpected number of fields
+ // and floating points format - so make sure it's set to `C`.
+ std::map<string, string> env = os::environment();
+ env["LC_ALL"] = "C";
Review comment:
Yep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
qianzhangxa edited a comment on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853135728
> root@thinkpad:~# LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/"
> -- true
> 0,31/msec/task-clock/306721/100,00/0/CPUs utilized
> 0//context-switches/306721/100,00/0/K/sec
> 0//cpu-migrations/306721/100,00/0/K/sec
> 44//page-faults/306721/100,00/0/M/sec
> 788234//cycles/311478/100,00/2/GHz
> 538077//instructions/311478/100,00/0/insn per cycle
> 106749//branches/311478/100,00/348/M/sec
> 4556//branch-misses/311478/100,00/4/of all branches
When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like:
`0//cpu-migrations/779898/100.00/0.000/K/sec` which is different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #391: Fixed parsing of `perf` output on some locales.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #391:
URL: https://github.com/apache/mesos/pull/391#issuecomment-853254919
> When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like:
> `0//cpu-migrations/779898/100.00/0.000/K/sec` which is different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`).
Might be because `LC_ALL` is set on your machine - try running:
```
# LC_ALL=fr_FR.UTF-8 perf stat --field-separator "/" -- true
```
instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org