android: Pass CPU properties from browser to renderer process.

android: Pass CPU properties from browser to renderer process.

This is necessary to allow the renderer processes to use NEON
instructions on ARM devices that support them in libraries like
Skia.

The main issue is that on JellyBean and higher, the renderer
process runs in a sandbox that prevents it from accessing the
filesystem, including /proc/ which is the only way to query the
kernel for the features detected by the CPU.

To overcome this, send the result of the probe to each renderer
process, which will use the new android_setCpu() function
introduced in https://gerrit.chromium.org/gerrit/#/c/39370/

Note that this requires that third-party libraries use the
android_getCpuCount() and android_getCpuFeatures() function
to get this data at runtime.

BUG=164154
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175820

[digit1, 2012-12-10 13:25:16]

first try. I manually tested (by adding logs that are not in this patch) that
the values are correctly passed to the renderer processes.

On the other hand, Skia needs an update to use android_getCpuFeatures() instead
of doing its own probing.

IMPORTANT: This patch is dependent on the following gerrit one:
https://gerrit.chromium.org/gerrit/#/c/39370/1

[bulach, 2012-12-10 14:03:07]

lgtm

lgtm, just nits.
feel free to go ahead once the dependencies are in.

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc
File base/android/cpu_features.cc (right):

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc#...
base/android/cpu_features.cc:1: #include <cpu-features.h>
nit: copyright notice

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc#...
base/android/cpu_features.cc:10: return android_getCpuCount();
nit: the indentation seems off here and the two calls below.

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc#...
base/android/cpu_features.cc:17: void SetCpu(JNIEnv*, jclass, jint cpuCount,
jlong cpuFeatures) {
nit: hacker_style for cpu_count and cpu_features

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
File content/app/android/sandboxed_process_service.cc (right):

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
content/app/android/sandboxed_process_service.cc:8: #
nit: spurious #

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
content/app/android/sandboxed_process_service.cc:68: ) {
nit: move this ) { one line up

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
content/app/android/sandboxed_process_service.cc:99: ) {
nit: nit: move this ) { one line up. also, hacker_style for the two params

https://codereview.chromium.org/11503013/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SandboxedProcessConnection.java
(right):

https://codereview.chromium.org/11503013/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SandboxedProcessConnection.java:251:
Log.i(TAG, "Sending CPU features: count=" + CpuFeatures.getCount() + "
features=" + CpuFeatures.getMask());
nit: >100cols

[digit1, 2012-12-10 14:44:56]

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc
File base/android/cpu_features.cc (right):

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc#...
base/android/cpu_features.cc:1: #include <cpu-features.h>
On 2012/12/10 14:03:07, bulach wrote:
> nit: copyright notice

Done.

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc#...
base/android/cpu_features.cc:10: return android_getCpuCount();
On 2012/12/10 14:03:07, bulach wrote:
> nit: the indentation seems off here and the two calls below.

Done.

https://codereview.chromium.org/11503013/diff/1/base/android/cpu_features.cc#...
base/android/cpu_features.cc:17: void SetCpu(JNIEnv*, jclass, jint cpuCount,
jlong cpuFeatures) {
On 2012/12/10 14:03:07, bulach wrote:
> nit: hacker_style for cpu_count and cpu_features

Done.

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
File content/app/android/sandboxed_process_service.cc (right):

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
content/app/android/sandboxed_process_service.cc:8: #
On 2012/12/10 14:03:07, bulach wrote:
> nit: spurious #

Done.

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
content/app/android/sandboxed_process_service.cc:68: ) {
On 2012/12/10 14:03:07, bulach wrote:
> nit: move this ) { one line up

Done.

https://codereview.chromium.org/11503013/diff/1/content/app/android/sandboxed...
content/app/android/sandboxed_process_service.cc:99: ) {
On 2012/12/10 14:03:07, bulach wrote:
> nit: nit: move this ) { one line up. also, hacker_style for the two params

Done.

https://codereview.chromium.org/11503013/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/SandboxedProcessConnection.java
(right):

https://codereview.chromium.org/11503013/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/SandboxedProcessConnection.java:251:
Log.i(TAG, "Sending CPU features: count=" + CpuFeatures.getCount() + "
features=" + CpuFeatures.getMask());
On 2012/12/10 14:03:07, bulach wrote:
> nit: >100cols

Actually, this was a debug message that should have been removed. How do we
import the generated BuildConfig.java file from this package, it seems it's full
package name depends on the component being built.

[Torne, 2012-12-10 15:32:06]

Is this just a layer on top of the cpufeatures code we pull in from the ndk via
build/android/cpufeatures.gypi or is it going to replace that? Currently the
handling of cpufeatures across different libs in chromium is very messy..

[digit1, 2012-12-10 16:00:41]

Not a layer, it _uses_ it. Actually, it allows any caller of
android_getCpuFeatures() to get the correct values when run in a renderer
process.

Of course, it requires said libraries to use this function instead of doing it's
own /proc/cpuinfo or /proc/auxv parsing (which won't work due to the
sandboxing).

[Torne, 2012-12-10 16:46:36]

On 2012/12/10 16:00:41, digit1 wrote:
> Not a layer, it _uses_ it. Actually, it allows any caller of
> android_getCpuFeatures() to get the correct values when run in a renderer
> process.
> 
> Of course, it requires said libraries to use this function instead of doing
it's
> own /proc/cpuinfo or /proc/auxv parsing (which won't work due to the
> sandboxing).

So this can replace libraries having to depend on the cpufeatures library
themselves?

[digit1, 2012-12-10 17:08:58]

No, it is not a replacement, it is an enhancement that ensures that
android_getCpuFeatures() will work correctly in renderer processes (currently it
will fail since by default it parses /proc too).

Please read http://code.google.com/p/chromium/issues/detail?id=164154 for more
details.

The libraries will have to depend on cpufeatures, instead of the opposite for
everything to work.

[digit1, 2012-12-12 16:15:47]

Mmmm, this is a terribly funky failure, I can't reproduce it: it refuses to
build content/app/android/sandboxed_process_service.cc because android_setCpu()
is not declared.

The command-line seems to indicate the right include path to
third_party/android_tools/ndk/sources/android/cpufeatures/ through, so this is
puzzling. It looks like third_party/android_tools/ isn't up to date on the bot.
Is that likely?

[digit1, 2012-12-12 16:23:56]

Ok, the LKGR is only at 172434, and the android_tools DEPS roll happened later
in 172493, so that's why this fails. Thanks Philippe for pointing me to the
right path.

I noticed that there is an obsolete version of the header in
third_party/webrtc/system_wrappers/source/android/cpu-features.h, this is not
part of the typical build, but we should definitely fix this too.

I'll try a new git cl try with the right parameters.

[commit-bot: I haz the power, 2012-12-14 10:55:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11503013/9001

[commit-bot: I haz the power, 2012-12-14 10:55:57]

Presubmit check for 11503013-9001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    base
    content/app/android

Presubmit checks took 1.8s to calculate.

[digit1, 2012-12-19 12:10:47]

I've added Adam and Darin as reviewers to get their approval. Thanks in advance.

[agl, 2012-12-19 16:35:42]

higher-level comment to answer before I review the code:

Are you sure that /proc/cpuinfo is the only method to detect NEON support? CP15
contains metadata about the existence of other co-processors on the system like
a CPUID instruction:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0091a/i4007.html

If this doesn't apply for some reason, please let me know and I'll review the
code.

[digit1, 2012-12-19 16:52:29]

Yes, unfortunately, only a few CP15 registers are accessible from unprivilege
mode on ARM (three are writable to implement memory synchronization barriers,
one is read/write for TLS, and a final one is read-only for other TLS stuff).
CPUID stuff is strictly kernel-restricted.

For more details see the section B5.8.3 "Views of the CP15 registers" in the ARM
ARM.

[darin (slow to review), 2012-12-19 17:49:22]

LGTM for gyp{i} changes in base and content

[agl, 2012-12-19 18:26:36]

LGTM, although I don't know much about Java and less about JNI!

https://chromiumcodereview.appspot.com/11503013/diff/9001/base/android/java/s...
File base/android/java/src/org/chromium/base/CpuFeatures.java (right):

https://chromiumcodereview.appspot.com/11503013/diff/9001/base/android/java/s...
base/android/java/src/org/chromium/base/CpuFeatures.java:32: * The value is
taken directory to
Comment seems truncated?

[joth, 2012-12-19 19:09:39]

drive by nits (but lgtm; I don't need to review again)

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/CpuFeatures.java (right):

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/CpuFeatures.java:14: // For more
context, see crbug.com/164154
nit: linkify.

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/CpuFeatures.java:46: public static void
set(int coreCount, long featuresMask) {
is this needed? loos like the sandbox process is already calling straight to the
native method.

[digit1, 2013-01-09 11:26:19]

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
File base/android/java/src/org/chromium/base/CpuFeatures.java (right):

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/CpuFeatures.java:14: // For more
context, see crbug.com/164154
On 2012/12/19 19:09:40, joth wrote:
> nit: linkify.

Sure, will do.

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/CpuFeatures.java:32: * The value is
taken directory to
On 2012/12/19 18:26:36, agl wrote:
> Comment seems truncated?

Yes, I'll update it.

https://codereview.chromium.org/11503013/diff/9001/base/android/java/src/org/...
base/android/java/src/org/chromium/base/CpuFeatures.java:46: public static void
set(int coreCount, long featuresMask) {
On 2012/12/19 19:09:40, joth wrote:
> is this needed? loos like the sandbox process is already calling straight to
the
> native method.

Indeed, this is no longer needed.

[digit1, 2013-01-09 17:08:50]

Patch set 4 removed the obsolete method and updated the comment.
All tests passes. There are two totally unrelated failures. so I'll commit this
as NOTRY=true.
Thanks everyone.

[commit-bot: I haz the power, 2013-01-09 17:09:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11503013/28001

[commit-bot: I haz the power, 2013-01-09 17:09:29]

Change committed as 175820