Split mojo_system dylib into public and private

Split mojo_system dylib into public and private

This CL splits the mojo_system dylib into two pieces:
1) A dynamic library suitable for apps to link against
2) A component suitable for linking into mojo_shell

The first dylib doesn't depend on base and simply jumps through a vtable into
an implementation defined by its embedder. The second implements the vtable and
actually contains the code to implement the API.

After this CL, Mojo should basically work in the static build. This CL is a
prerequiste for fixing the Android port.

R=darin@chromium.org
BUG=326918
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239692

[darin (slow to review), 2013-12-09 18:54:56]

Trung should review this (as well).

[viettrungluu, 2013-12-09 19:25:59]

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
File mojo/public/system/core_private.cc (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
mojo/public/system/core_private.cc:13: MojoResult MojoClose(MojoHandle handle) {
Can't we just replace these with function pointers (to internal C functions),
that are initialized by Init() instead?

https://codereview.chromium.org/106173003/diff/20001/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/system/core_impl.h#...
mojo/system/core_impl.h:34: virtual MojoResult Close(MojoHandle handle)
OVERRIDE;
Imposing even more virtual dispatch seems to be bad to me. Hrm.

[abarth-chromium, 2013-12-09 21:19:33]

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
File mojo/public/system/core_private.cc (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
mojo/public/system/core_private.cc:13: MojoResult MojoClose(MojoHandle handle) {
On 2013/12/09 19:25:59, viettrungluu wrote:
> Can't we just replace these with function pointers (to internal C functions),
> that are initialized by Init() instead?

We could do that.  What would we gain by doing that?  The nice thing about using
a vtable is that we get to use |this| and OVERRIDE to make sure we've got all
the types correct.

https://codereview.chromium.org/106173003/diff/20001/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/system/core_impl.h#...
mojo/system/core_impl.h:34: virtual MojoResult Close(MojoHandle handle)
OVERRIDE;
On 2013/12/09 19:25:59, viettrungluu wrote:
> Imposing even more virtual dispatch seems to be bad to me. Hrm.

Jumping through a function pointer is the same as jumping through a virtual
function call.  If you're worried about folks who call these functions directly,
we can mark CoreImpl FINAL.

[viettrungluu, 2013-12-09 21:24:56]

On 2013/12/09 21:19:33, abarth wrote:
>
https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
> File mojo/public/system/core_private.cc (right):
> 
>
https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
> mojo/public/system/core_private.cc:13: MojoResult MojoClose(MojoHandle handle)
{
> On 2013/12/09 19:25:59, viettrungluu wrote:
> > Can't we just replace these with function pointers (to internal C
functions),
> > that are initialized by Init() instead?
> 
> We could do that.  What would we gain by doing that?  The nice thing about
using
> a vtable is that we get to use |this| and OVERRIDE to make sure we've got all
> the types correct.
> 
> https://codereview.chromium.org/106173003/diff/20001/mojo/system/core_impl.h
> File mojo/system/core_impl.h (right):
> 
>
https://codereview.chromium.org/106173003/diff/20001/mojo/system/core_impl.h#...
> mojo/system/core_impl.h:34: virtual MojoResult Close(MojoHandle handle)
> OVERRIDE;
> On 2013/12/09 19:25:59, viettrungluu wrote:
> > Imposing even more virtual dispatch seems to be bad to me. Hrm.
> 
> Jumping through a function pointer is the same as jumping through a virtual
> function call.  If you're worried about folks who call these functions
directly,
> we can mark CoreImpl FINAL.

Isn't the virtual function call one more level of indirection?

Can you run the perftests to compare? (Last I checked, the best-performing
configuration on my Z620 running Linux was the gcc with the static build; the
shared library build already imposes a nontrivial performance hit.)

[abarth-chromium, 2013-12-09 21:31:07]

On 2013/12/09 21:24:56, viettrungluu wrote:
> Isn't the virtual function call one more level of indirection?

Compared to before this CL, yes.  Compared to function pointers, it's the same.

> Can you run the perftests to compare? (Last I checked, the best-performing
> configuration on my Z620 running Linux was the gcc with the static build; the
> shared library build already imposes a nontrivial performance hit.)

Sure.

[abarth-chromium, 2013-12-09 21:40:48]

Here's some data from a z620 on Linux with gcc. Note: In the "without CL" run,
everything is statically linked, which isn't a configurations we can
realistically ship. In the "with CL" run, we're jumping both in and out of
libmojo_system.so.

== Without CL ==

abarth@abarth3:/src/cr/src$ ./out/Release/mojo_public_perftests 
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from SystemPerftest
[ RUN      ] SystemPerftest.NoOp
NoOp    3.28067e+08 iterations/second
[       OK ] SystemPerftest.NoOp (3000 ms)
[ RUN      ] SystemPerftest.MessagePipe_CreateAndClose
MessagePipe_CreateAndClose  950544  iterations/second
[       OK ] SystemPerftest.MessagePipe_CreateAndClose (3000 ms)
[ RUN      ] SystemPerftest.MessagePipe_WriteAndRead
MessagePipe_WriteAndRead_10bytes    3.75931e+06 iterations/second
MessagePipe_WriteAndRead_100bytes   3.25345e+06 iterations/second
MessagePipe_WriteAndRead_1000bytes  3.08514e+06 iterations/second
MessagePipe_WriteAndRead_10000bytes 1.54883e+06 iterations/second
[       OK ] SystemPerftest.MessagePipe_WriteAndRead (12000 ms)
[ RUN      ] SystemPerftest.MessagePipe_EmptyRead
MessagePipe_EmptyRead   1.00247e+07 iterations/second
[       OK ] SystemPerftest.MessagePipe_EmptyRead (3000 ms)
[----------] 4 tests from SystemPerftest (21000 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (21000 ms total)
[  PASSED  ] 4 tests.

== With CL ==

abarth@abarth3:/src/cr/src$ ./out/Release/mojo_public_perftests 
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from SystemPerftest
[ RUN      ] SystemPerftest.NoOp
NoOp    3.1656e+08  iterations/second
[       OK ] SystemPerftest.NoOp (3000 ms)
[ RUN      ] SystemPerftest.MessagePipe_CreateAndClose
MessagePipe_CreateAndClose  967180  iterations/second
[       OK ] SystemPerftest.MessagePipe_CreateAndClose (3000 ms)
[ RUN      ] SystemPerftest.MessagePipe_WriteAndRead
MessagePipe_WriteAndRead_10bytes    3.71253e+06 iterations/second
MessagePipe_WriteAndRead_100bytes   3.66733e+06 iterations/second
MessagePipe_WriteAndRead_1000bytes  3.04214e+06 iterations/second
MessagePipe_WriteAndRead_10000bytes 1.5052e+06  iterations/second
[       OK ] SystemPerftest.MessagePipe_WriteAndRead (12000 ms)
[ RUN      ] SystemPerftest.MessagePipe_EmptyRead
MessagePipe_EmptyRead   9.4341e+06  iterations/second
[       OK ] SystemPerftest.MessagePipe_EmptyRead (3000 ms)
[----------] 4 tests from SystemPerftest (21000 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (21001 ms total)
[  PASSED  ] 4 tests.

[viettrungluu, 2013-12-09 22:04:25]

Thanks, I'm sufficiently convinced. LGTM w/nits.

https://codereview.chromium.org/106173003/diff/20001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/mojo.gyp#newcode113
mojo/mojo.gyp:113: 'direct_dependent_settings': {
Do we still need this?

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
File mojo/public/system/core_private.cc (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
mojo/public/system/core_private.cc:9: static mojo::Core* g_core = 0;
Nit: We usually use NULL (which may necessitate including stddef.h).

https://codereview.chromium.org/106173003/diff/20001/mojo/system/memory.h
File mojo/system/memory.h (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/system/memory.h#new...
mojo/system/memory.h:20: size_t count);
Nit: Fix indentation.

[abarth-chromium, 2013-12-09 22:48:33]

https://codereview.chromium.org/106173003/diff/20001/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/mojo.gyp#newcode113
mojo/mojo.gyp:113: 'direct_dependent_settings': {
On 2013/12/09 22:04:25, viettrungluu wrote:
> Do we still need this?

Yes.  The alternative is that I could export dependent settings from
mojo_system.

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
File mojo/public/system/core_private.cc (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/public/system/core_...
mojo/public/system/core_private.cc:9: static mojo::Core* g_core = 0;
On 2013/12/09 22:04:25, viettrungluu wrote:
> Nit: We usually use NULL (which may necessitate including stddef.h).

Done.

https://codereview.chromium.org/106173003/diff/20001/mojo/system/memory.h
File mojo/system/memory.h (right):

https://codereview.chromium.org/106173003/diff/20001/mojo/system/memory.h#new...
mojo/system/memory.h:20: size_t count);
On 2013/12/09 22:04:25, viettrungluu wrote:
> Nit: Fix indentation.

Done.

[abarth-chromium, 2013-12-09 22:54:20]

On 2013/12/09 22:48:33, abarth wrote:
> https://codereview.chromium.org/106173003/diff/20001/mojo/mojo.gyp
> File mojo/mojo.gyp (right):
> 
> https://codereview.chromium.org/106173003/diff/20001/mojo/mojo.gyp#newcode113
> mojo/mojo.gyp:113: 'direct_dependent_settings': {
> On 2013/12/09 22:04:25, viettrungluu wrote:
> > Do we still need this?
> 
> Yes.  The alternative is that I could export dependent settings from
> mojo_system.

Turns out you're right.  It's not needed anymore.

[commit-bot: I haz the power, 2013-12-09 23:07:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/106173003/60001

[commit-bot: I haz the power, 2013-12-10 02:26:44]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-10 02:27:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/106173003/80001

[commit-bot: I haz the power, 2013-12-10 07:13:50]

Change committed as 239692