Pepper: Narrow locking at ld subprocess start.

Pepper: Narrow locking at ld subprocess start.

Currently, subprocess_mu_ is held the entire time the ld subprocess is
starting. This blocks a larger refactor that I'd like to make as part of nexe
loading.

I'd like to change ServiceRuntime::LoadModule() to be asynchronous to simplify
threading behavior on the plugin side when loading nexe modules. However, this
requires that many methods are made asynchronous, including
Plugin::LoadHelperNaClModule().

PnaclTranslateThread will need to use a pattern similar to WaitForSelLdrStart()
to resume execution on a background thread. However, I don't like the idea of
introducing another mutex and condvar to deal with while holding
subprocess_mu_.

This change narrows the time that subprocess_mu_ is held to make that refactor
possible. This follows a similar change made for the llc subprocess.

BUG=333950
R=bbudge@chromium.org

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

[bbudge, 2014-06-02 16:58:04]

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

https://codereview.chromium.org/308193004/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:345:
NaClSubprocess* ld_subprocess = plugin_->LoadHelperNaClModule(
Why not make ld_subprocess a nacl::scoped_ptr? We'll probably use scoped_ptr
when this code is moved to the renderer anyway.

Then below, you can swap with the ld_subprocess_ field.

https://codereview.chromium.org/308193004/diff/20001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:371:
ld_subprocess = NULL;
This seems unnecessary, since it's a local and we're exiting its scope.

[teravest, 2014-06-02 17:02:03]

Sounds reasonable.

On Mon, Jun 2, 2014 at 10:58 AM,  <bbudge@chromium.org> wrote:
>
>
https://codereview.chromium.org/308193004/diff/20001/ppapi/native_client/src/...
> File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
> (right):
>
>
https://codereview.chromium.org/308193004/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:345:
>
> NaClSubprocess* ld_subprocess = plugin_->LoadHelperNaClModule(
> Why not make ld_subprocess a nacl::scoped_ptr? We'll probably use
> scoped_ptr when this code is moved to the renderer anyway.
>
> Then below, you can swap with the ld_subprocess_ field.

Done.

>
>
https://codereview.chromium.org/308193004/diff/20001/ppapi/native_client/src/...
> ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:371:
> ld_subprocess = NULL;
> This seems unnecessary, since it's a local and we're exiting its scope.

I did this for readability, but the swap() is clearer.

>
> https://codereview.chromium.org/308193004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bbudge, 2014-06-03 15:57:46]

lgtm

[teravest, 2014-06-03 17:14:59]

Committed patchset #4 manually as r274557 (presubmit successful).