PnaclCoordinator: Use llc's module-splitting for pexe compilation

PnaclCoordinator: Use llc's module-splitting for pexe compilation

When there is more than 1 core in the system, request extra temp files
and use LLC's StreamInitWithSplit rpc to enable module splitting.
If the RPC fails, fall back to StreamInitWithOverrides.

Add a private Pepper interface to get the cpu count rather than
duplicating base::SysInfo in the plugin.

TEST=browser_tests --gtest_filter=NaClBrowserTstPnacl.*
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3777
R=jschuh@chromium.org, jvoung@chromium.org, teravest@chromium.org

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

[Derek Schuff, 2014-02-14 00:19:08]

jvoung: everything
bbudge: PPAPI API owners

[jvoung (off chromium), 2014-02-14 00:46:48]

otherwise LGTM, modulo the printfs

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:204:
coordinator->split_module_count_ = std::min(4, cpus);
Maybe clamp to >= 1. Looks like the unittest of implementation is supposed to
test that though.

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:227:
num_object_files_opened_(0),
Initialize split_module_count_(0) or to 1 in constructor also.

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:91: // (count
can be < the buffer size). This can be
 can be... ?

[Derek Schuff, 2014-02-14 01:01:50]

+jschuh for new IPC (we can't get the number of cores in the renderer, so we
have to ask the browser).
-bbudge(vacation), +dmichael (PPAPI API)

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:204:
coordinator->split_module_count_ = std::min(4, cpus);
On 2014/02/14 00:46:49, jvoung (cr) wrote:
> Maybe clamp to >= 1. Looks like the unittest of implementation is supposed to
> test that though.

Done.

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:227:
num_object_files_opened_(0),
On 2014/02/14 00:46:49, jvoung (cr) wrote:
> Initialize split_module_count_(0) or to 1 in constructor also.

Done.

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/165433003/diff/30001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:91: // (count
can be < the buffer size). This can be
On 2014/02/14 00:46:49, jvoung (cr) wrote:
>  can be... ?

Done.

[Derek Schuff, 2014-02-14 01:03:35]

I'll also note that we can't just let llc/ld decide how many cores to use by
calling sysconf because the plugin needs to know how many FDs to request

[jvoung (off chromium), 2014-02-14 01:43:44]

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:42: Plugin*
plugin)  {
extra space

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:176:
std::copy(split_arg.begin(), split_arg.end(), std::back_inserter(split_args));
<iterator> for back_inserter

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:334:
ld_in_files.push_back(plugin_->wrapper_factory()->MakeInvalid());
remember to delete these too -- it looks like even MakeInvalid() will call
MakeGenericCleanup() which makes a new DescWrapper()

The obj_files_'s wrappers look okay though since they have scoped_ptrs to the
wrappers.

Do the MakeInvalid() need to be unique? It looks like the underlying desc is a
singleton, but the the DescWrapper is not. If it doesn't need to be unique, can
the PnaclTranslateThread() have a single field initialized by MakeInvalid(), put
it in a scoped_ptr and reuse the references across the LLC and LD RPCs? Then
it'll clean up when the Thread dies.

[Derek Schuff, 2014-02-14 06:11:23]

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:42: Plugin*
plugin)  {
On 2014/02/14 01:43:44, jvoung (cr) wrote:
> extra space

Done.

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:176:
std::copy(split_arg.begin(), split_arg.end(), std::back_inserter(split_args));
On 2014/02/14 01:43:44, jvoung (cr) wrote:
> <iterator> for back_inserter

Done.

https://codereview.chromium.org/165433003/diff/120001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:334:
ld_in_files.push_back(plugin_->wrapper_factory()->MakeInvalid());
On 2014/02/14 01:43:44, jvoung (cr) wrote:
> remember to delete these too -- it looks like even MakeInvalid() will call
> MakeGenericCleanup() which makes a new DescWrapper()
> 
> The obj_files_'s wrappers look okay though since they have scoped_ptrs to the
> wrappers.
> 
> Do the MakeInvalid() need to be unique? It looks like the underlying desc is a
> singleton, but the the DescWrapper is not. If it doesn't need to be unique,
can
> the PnaclTranslateThread() have a single field initialized by MakeInvalid(),
put
> it in a scoped_ptr and reuse the references across the LLC and LD RPCs? Then
> it'll clean up when the Thread dies.

Looks like it doesn't need to be unique. switched to a ScopedPtr on the
coordinator.

[jschuh, 2014-02-14 15:41:32]

Lgtm on the ipc security side--it's just a benign numeric value. However, you
should ping privacy@chromium.org since this appears to be another piece of
fingerprint data that isn't otherwise available.

[dmichael (off chromium), 2014-02-14 15:47:24]

Punting this over to teravest@ for ppapi OWNERS review; He's been closer to the
NaCl integration code lately.

[Derek Schuff, 2014-02-14 16:51:46]

On 2014/02/14 15:41:32, Justin Schuh wrote:
> Lgtm on the ipc security side--it's just a benign numeric value. However, you
> should ping mailto:privacy@chromium.org since this appears to be another piece
of
> fingerprint data that isn't otherwise available.

Can do, although I should note that this information is already available to
untrusted PNaCl apps, which can call sysconf(_SC_NPROCESSORS_ONLN).

[jvoung (off chromium), 2014-02-14 17:33:53]

new pnacl_* changes lgtm

[teravest, 2014-02-14 17:36:24]

lgtm

[Derek Schuff, 2014-02-14 18:03:34]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2014-02-14 18:04:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/165433003/470001

[jschuh, 2014-02-14 18:25:11]

On 2014/02/14 16:51:46, Derek Schuff wrote:
> On 2014/02/14 15:41:32, Justin Schuh wrote:
> > Lgtm on the ipc security side--it's just a benign numeric value. However,
you
> > should ping mailto:privacy@chromium.org since this appears to be another
piece
> of
> > fingerprint data that isn't otherwise available.
> 
> Can do, although I should note that this information is already available to
> untrusted PNaCl apps, which can call sysconf(_SC_NPROCESSORS_ONLN).

Was not aware of that. But either way, Privacy Team is trying to catalog sources
of fingerprinting info on the web, so it can't hurt to give them a heads up.

[commit-bot: I haz the power, 2014-02-14 20:39:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:39:52]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Derek Schuff, 2014-02-14 22:31:16]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:34:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/165433003/470001

[Derek Schuff, 2014-02-15 00:37:09]

Committed patchset #7 manually as r251458 (presubmit successful).

[commit-bot: I haz the power, 2014-02-15 00:54:25]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...