Open GoogleCodeExporter opened 9 years ago
GoogleCodeExporter
commented
9 years ago Not sure if bugdroid hangs around here, I committed this in webrtc to suppress
the race for now, but it'd be nice to be rid of it.
https://codereview.webrtc.org/1414093003/Original comment by pbos@google.com on 20 Oct 2015 at 12:00
GoogleCodeExporter
commented
9 years ago The global variable is basically a shadow of cpuid flags from the cpu, which
wont vary. But the implementation enables/disables the flags as it goes, so it
is unsafe.
I have a CL that fixes that, so only the final result is written to the global.
The other concern would only be for unittests, but theres a function to disable
flags. Its definitely thread unsafe in nature.
e.g.
disable cpu flags
run function1
enable cpu flags
run function2
compare result
during function1, another thread could change the flags.
If the code is working correctly, the only behavior change will be performance.
Its not necessary to use this global... I could query the cpuid directly every
time. It just came up high on profiles, whereas the current implementation
checks the flag in the calling code inline. Modern cpu detection is getting
fairly complicated, checking for OS support, so if cpuid were not shadowed, it
would get increasingly slow in future.
The cpu dispatching is done on every function call.
Most libraries set up function pointers once. That would be faster from a
dispatching point of view, but less flexible and complicates the low level
assembly. The SIMD code is simple and only able to do specific widths.. e.g.
multiple of 16. The init for those function pointers would probably have the
same issue.
If the race is coming up in practice, you could call the libyuv::InitCpuFlags()
once in your startup.Original comment by fbarch...@google.com on 20 Oct 2015 at 6:13
GoogleCodeExporter
commented
9 years ago For unittests this should be fine, since (I'd expect) you wouldn't change those
while any test is running.
For this to be totally race free we have to use something like __atomic_load_n
and __atomic_store_n for the lazy initialization I think. Or mandate that we
call InitCpuFlags() in webrtc before we use it, but I don't think that we have
a single common initialization point where we'd put this (for Chromium).Original comment by pbos@google.com on 21 Oct 2015 at 10:01
Original issue reported on code.google.com by
pbos@google.comon 19 Oct 2015 at 8:58