Closed Bug 1163224 Opened 5 years ago Closed 4 years ago

Compile rust SOURCES into rlib-per-crate, compile single crate including all referenced crates into staticlib for linking

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox40 affected, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox40 --- affected
firefox48 --- fixed

People

(Reporter: rillian, Assigned: froydnj)

References

Details

Attachments

(2 files, 5 obsolete files)

Rustc supports --crate-type staticlib for building stand-alone (except for system-level dependencies) outputs. It would be nice if we could have it make object files instead, along with a list of compiler-runtime libraries we could add to the link line, so we could treat rust modules more like other source types.

This will require support from rustc devs of course.
'rustc --crate-type staticlib --emit obj' is close, but I get an unresolved symbol error for __morestack.

See https://github.com/rust-lang/rust/issues/15631 (wontfix)
From what I can tell, building rust packages works a bit different.
For example, our rust-url wrapper depends on 3 other libraries - `matches`, `rustc-serialize`, and `rust-url`
It builds them with
  rustc matches-0.1.2/lib.rs --crate-name matches --crate-type lib # extra flags
  rustc rustc-serialize-0.3.14/src/lib.rs --crate-name rustc_serialize --crate-type lib # extra flags
  rustc rust-url/src/lib.rs --crate-name url --crate-type lib -L dependency=/path/to/deps --extern rustc_serialize=path/to/deps/librustc_serialize-9ef26f158d5284e0.rlib
Then it builds the final static library for rust-url-capi with:
  rustc src/lib.rs --crate-name rust_url_capi --crate-type staticlib # extra flags and extern deps

It would be ideal to be able to configure the target library name, and the extra flags which need to be passed to the rustc command. Of course, I have no idea how to make the build system work like this :)
Hello! I've done a lot of work on the Rust compiler and I'm quite familiar with
building Rust projects, so I'm hoping I can help to answer any questions here.
I'll preface all this with saying that there haven't been a lot of efforts quite
like this to integrate possibly multiple Rust libraries into existing
applications, so this is blazing new frontiers!

As I'm sure you've discovered, Rust's unit of compilation is not per-file but
rather per-library. There are a few possible outputs of the compiler:

* Object files (`--emit=obj`). The object file emitted is independent of the
  `--crate-type` being compiled, but is missing out on all dependencies (more on
  this in a moment)
* Rust libraries (.rlib) via `--crate-type=rlib`. An rlib is just a `.a` file
  with some other goodies packed in such as metadata (e.g. a "header file") and
  native libraries that are statically linked to the Rust source (e.g. the
  contents of other `.a` files).
* Static libraries (.a) via `--crate-type=staticlib`. These libraries (as
  you've discovered) include all dependencies. The intention behind these are
  that they contain the "world of rust code", and it's not intended to link more
  than one into a final program.

Specifically on the topic of dependencies, here's a breakdown of the
dependencies that staticlib outputs include (for example), but object files do
not:

* Native libraries the Rust source depends on will be missing
* All upstream Rust dependencies are missing
* The compiler's `libmorestack.a` is missing (needed to resolve __morestack)
* The compiler's `libcompiler-rt.a` is missing (LLVM generates function calls
  into this library)

The dependency on `libmorestack.a` and `__morestack` is present to detect stack
overflow at runtime (each function has a prologue to check this), but I doubt
that Gecko wants to make use of this machinery, so you can omit the dependency
entirely with the `-C no-stack-check` flag to the compiler.

The dependency on `libcompiler-rt.a` is often satisfied by `libgcc` elsewhere
on the system, so it may not be necessary for you to explicitly include this.

---

Ok, so that's a high-level overview of the output of the compiler itself, and
I'd recommend taking one of two paths for integrating this output in to Gecko:

First, I'd recommend including all Rust code in the world for Gecko into one
crate and compile that as a staticlib, and then that one staticlib is linked
into Gecko itself. This would look something like:

1. Each Rust library included will be compiled as an `rlib`.
2. Gecko drives the build process to make sure all libraries are built in order,
   passing `-L` flags to the compiler to indicate where intermediate output is
   located.
3. A final Rust source file is generated with an `extern crate` declaration to
   the Rust crates directly needed by Gecko. For example this file would not
   contain `extern crate matches` (a dependency of `rust-url`), but it would
   contain `extern crate url`.
4. This final crate is compiled as a staticlib, which will assemble all Rust
   `rlib` files into one `.a` archive, as well as any native dependencies
   and/or compiler dependencies.
5. This one `.a` file is linked into Gecko itself, and is the entire world of
   Rust code in Gecko.

The good part about this strategy is that it's easily adaptable to Cargo in the
future, and somewhat follows the conventions of building Rust code today. The
downside is that this generated crate which contains the "world of Rust" is
somewhat unfortunate!

My second alternative would be similar to the above strategy, but instead of
building a final `.a` file you would manually link in all `.rlib` files (they're
all `.a` files under the hood anyway). You'd basically skip steps 3-5 above and
instead take on the responsibility of:

* Making sure that everything is ordered correctly on the link line (e.g. all
  Rust libraries show up and they all show up in the right order).
* Making sure all system native dependencies are on the link line in the right
  order.
* Making sure that all compiler dependencies are somewhere on the link line and
  in the right order.

---

Hope that helps clear up anything! If you have any more questions, I'm more than
willing to answer them!
:glandium, Do you have suggestions for how we should be organizing the build and final link steps for Rust code?
Flags: needinfo?(mh+mozilla)
I'm not sure what you're asking that is not essentially in comment 3.

That said, I have concerns. Specifically, what happens when we end up wanting to put rust code somewhere else than libxul.so? (a binary XPCOM component, a GMP plugin, whatever else) Do we care?
Flags: needinfo?(mh+mozilla)
The main concern here is similar to that with C++ in that the standard library should probably only show up once in a process image. This may mean dynamically linking the Rust standard library if there's a few other shared components in Gecko. 

The problems with having two Rust standard libraries aren't inherently showstoppers, however. It means the Rust code linked to may have bugs when interoperating (if at all), and then there's the code bloat issue. In general though so long as it links it'll probably work.
In meeting today acrichto suggested having a single lib.rs somewhere in the tree which imports all the rust code in gecko. We compile all that in a single monolithic crate.

pros: quick to implement, unblocks other teams.

cons: slower incremental builds, rust code not listed in its local moz.build (unless mozbuild actually generates lib.rs?)
Having mozbuild generate a lib.rs sounds reasonable, but the incremental build problem does sound kind of crappy.
I'm not sure what Gecko dev incremental build expectations are, but a no-op incremental relink of Servo (~800k lines of Rust code, plus another million or so in C++ from some staticlibs like skia/azure/SM that are just tacked on) for debug takes just over 20s on a 3 year old macbook pro.
From talking to alex today, .rlib files are ar archives with object files, and additional rust-specific metadata files. In most cases, rustc passes these directly to the platform linker and it's fine unless one does lto, -Wl,-all_load, or similar.

So, rustc's -crate-type=rlib (what cargo builds by default) would likely work with our existing linker infrastructure.
AIUI, .rlib files contain parts of the std library too, so if you have two .rlibs, you have that duplicated.
The .rlib files the compiler generates only contain the object code for the library being compiled at hand, so there's no worry about duplicating the standard library for example. Things get trickier when you generate a staticlib (libfoo.a), however, which does indeed include the standard library.

In that sense linking a bunch of rlibs manually just means that you'll have to also go hunt down libstd.rlib and all of its dependencies. If a libfoo.a is generated you'll have to be careful to only generate one (e.g. all Rust code in only one staticlib).
We talked about compiling per-crate (into rlib), and then having a single generated .rs that we would compile into a staticlib and link into the final library (like libxul). I think that covers everything said here?
Yes compiling each crate to an rlib and then compiling them all (via the generated .rs) into a staticlib would work and is likely the best option as it'll automatically pick up libstd and its dependencies.
Resummarizing: we want to:
* Compile an rlib per-crate (where "crate" is sort of nebulously defined in Gecko-land at the moment)
* Compile a staticlib from a possibly-generated rust source file that pulls in all referenced crates for the final library
* Link that staticlib into the final library
Summary: Have rustc emit object files instead of libraries → Compile rust SOURCES into rlib-per-crate, compile single crate including all referenced crates into staticlib for linking
Our current build system support for Rust compiles any Rust crate into a
so-called staticlib, which is a static library (.a file) that includes
the Rust runtime.  That staticlib is then linked into libxul.  For
supporting multiple crates, this approach breaks down, as linking
multiple copies of the Rust runtime is going to fail.

For supporting multiple crates, the approach taken here is to compile
each crate into a so-called rlib, which is essentially a staticlib
without the Rust runtime linked in.  The build system takes note of
every crate destined for linking with libxul (treating them like static
libraries generated from C/C++ files), and generates a super-crate,
whimsically named "rul", that is compiled as a staticlib (so as to
include the Rust runtime) and then linked into libxul.  Thus only one
copy of the Rust runtime is included, and the Rust compiler can take
care of any inter-crate dependencies.

This patch currently only supports Rust code in shared libraries, not in
binaries.  The handling for the rul crate is placed in the common
backend, with a special hook for derived backends to handle shared
library objects.

I know this patch isn't complete (we need some tests, which will probably wind
up being larger than the patch itself, for instance).  But it links libxul
locally with an --enable-rust build.  libxul-gtest also links successfully,
which gives me confidence that the approach isn't somehow libxul-specific.
Attachment #8716624 - Flags: feedback?(mh+mozilla)
Nice patch, Nathan. It's great that you can just call a rustc with a bunch of rlibs. I assumed we'd have to generate some awful crate.rs which re-exported all the public symbols in all the other crates.

I suggest s/RustRlibLibrary/RustLibrary/ would sufficiently clear.
This was bit-rotted by changes from bug 1245763.
Flags: needinfo?(nfroyd)
Rebased over recent changes, untested.
Attachment #8716624 - Attachment is obsolete: true
Attachment #8716624 - Flags: feedback?(mh+mozilla)
Attachment #8720323 - Flags: feedback?(mh+mozilla)
(In reply to Ralph Giles (:rillian) from comment #18)
> This was bit-rotted by changes from bug 1245763.

Rebased, and glandium gets a reset for his two-week old feedback request. ;)
Flags: needinfo?(nfroyd)
Fix up rebase from the last version. _process_sources is gone, so just work from the SourcePath list directly.


(In reply to Nathan Froyd [:froydnj] from comment #20)

> Rebased, and glandium gets a reset for his two-week old feedback request. ;)

I don't think it works that way! :)
Attachment #8720323 - Attachment is obsolete: true
Attachment #8720323 - Flags: feedback?(mh+mozilla)
Attachment #8721262 - Flags: feedback?(mh+mozilla)
This is my rebase, which actually compiles and links things for me.
Attachment #8721573 - Flags: feedback?(mh+mozilla)
Attachment #8721262 - Attachment is obsolete: true
Attachment #8721262 - Flags: feedback?(mh+mozilla)
This has bit-rotted again, perhaps by bug 1249858.

Works for me on linux. I hoped it would fix bug 1188030 as well, but it doesn't seem to. Let's get this landed.
Flags: needinfo?(nfroyd)
Comment on attachment 8721573 [details] [diff] [review]
add build system support for multiple Rust crates v4

Review of attachment 8721573 [details] [diff] [review]:
-----------------------------------------------------------------

I probably need to review this when I have fresher eyes and brain, but here is a first pass.

::: config/rules.mk
@@ +963,5 @@
> +	$(RUSTC) $(RUSTFLAGS) --crate-type rlib --emit dep-info=$(MDDEPDIR)/$(call mk_libname,$<).pp,link=$(call mk_libname,$<) $(_VPATH_SRCS)
> +
> +$(RS_STATICLIB_CRATE_OBJ):
> +	$(REPORT_BUILD)
> +	$(RUSTC) $(RUSTFLAGS) --crate-type staticlib $(RLIB_EXTERN_CRATE_OPTIONS) --emit dep-info=$(MDDEPDIR)/$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)).pp,link=$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)) $(RS_STATICLIB_CRATE_SRC)

I don't see a reason not to use $< instead of $(RS_STATICLIB_CRATE_SRC), and $(_VPATH_SRCS) for the last one, like in the $(RSOBJS) target.

::: python/mozbuild/mozbuild/backend/common.py
@@ +263,5 @@
> +
> +        elif isinstance(obj, SharedLibrary):
> +            # Enable the backend to process Shared Library more-or-less normally.
> +            if hasattr(self, '_process_shared_library'):
> +                self._process_shared_library(obj)

Mmmm, I'd rather not make this a "public" API for backends, and instead, stick all this in RecursiveMakeBackend. Making this better for other backends is bug 1224463 or some unfiled but related bug.

@@ +265,5 @@
> +            # Enable the backend to process Shared Library more-or-less normally.
> +            if hasattr(self, '_process_shared_library'):
> +                self._process_shared_library(obj)
> +            if any(isinstance(o, RustRlibLibrary) for o in obj.linked_libraries) and \
> +               hasattr(self, '_handle_linked_rust_crates'):

Yeah, the above applies to _handle_linked_rust_crates even more.

@@ +268,5 @@
> +            if any(isinstance(o, RustRlibLibrary) for o in obj.linked_libraries) and \
> +               hasattr(self, '_handle_linked_rust_crates'):
> +                # write out Rust file with extern crate declarations
> +                rust_rlibs = [o for o in obj.linked_libraries if isinstance(o, RustRlibLibrary)]
> +                extern_crate_file = obj.objdir + '/rul.rs'

mozpath.join

@@ +269,5 @@
> +               hasattr(self, '_handle_linked_rust_crates'):
> +                # write out Rust file with extern crate declarations
> +                rust_rlibs = [o for o in obj.linked_libraries if isinstance(o, RustRlibLibrary)]
> +                extern_crate_file = obj.objdir + '/rul.rs'
> +                os.path.exists(obj.objdir) or os.makedirs(obj.objdir)

ensureParentDir(extern_crate_file)

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +938,5 @@
>  
> +        rust_sources = []
> +        for obj in self._handle_linkables(context, passthru, rust_sources):
> +            if isinstance(obj, BaseSources) and obj.canonical_suffix == '.rs':
> +                rust_sources.append(obj)

The 'append an element to the argument I just passed to a generator expecting it to do something with it' thing should probably be avoided as it's confusing. Plus, it seems to me this doesn't actually do anything useful, since rust_source is set to a new list in _handle_linkables.
Attachment #8721573 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #24)
> ::: config/rules.mk
> @@ +963,5 @@
> > +	$(RUSTC) $(RUSTFLAGS) --crate-type rlib --emit dep-info=$(MDDEPDIR)/$(call mk_libname,$<).pp,link=$(call mk_libname,$<) $(_VPATH_SRCS)
> > +
> > +$(RS_STATICLIB_CRATE_OBJ):
> > +	$(REPORT_BUILD)
> > +	$(RUSTC) $(RUSTFLAGS) --crate-type staticlib $(RLIB_EXTERN_CRATE_OPTIONS) --emit dep-info=$(MDDEPDIR)/$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)).pp,link=$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)) $(RS_STATICLIB_CRATE_SRC)
> 
> I don't see a reason not to use $< instead of $(RS_STATICLIB_CRATE_SRC), and
> $(_VPATH_SRCS) for the last one, like in the $(RSOBJS) target.

Because $^ isn't just $(RS_STATICLIB_CRATE_SRC), for whatever reason.  I think this might have to do with it being a generated file.  These are the dependencies for $(RS_STATICLIB_CRATE_OBJ) in my build:

backend.mk Makefile ../../config/autoconf.mk /home/froydnj/src/gecko-dev.git/config/config.mk /opt/build/froydnj/build-rust-mc/toolkit/library/rul.rs

> @@ +269,5 @@
> > +               hasattr(self, '_handle_linked_rust_crates'):
> > +                # write out Rust file with extern crate declarations
> > +                rust_rlibs = [o for o in obj.linked_libraries if isinstance(o, RustRlibLibrary)]
> > +                extern_crate_file = obj.objdir + '/rul.rs'
> > +                os.path.exists(obj.objdir) or os.makedirs(obj.objdir)
> 
> ensureParentDir(extern_crate_file)

self._write_file works even better, so I'll use that instead.

> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +938,5 @@
> >  
> > +        rust_sources = []
> > +        for obj in self._handle_linkables(context, passthru, rust_sources):
> > +            if isinstance(obj, BaseSources) and obj.canonical_suffix == '.rs':
> > +                rust_sources.append(obj)
> 
> The 'append an element to the argument I just passed to a generator
> expecting it to do something with it' thing should probably be avoided as
> it's confusing. Plus, it seems to me this doesn't actually do anything
> useful, since rust_source is set to a new list in _handle_linkables.

Whoops, botched the rebase here.
Flags: needinfo?(nfroyd)
Addressed glandium's review comments.
Attachment #8724116 - Flags: feedback?(mh+mozilla)
Attachment #8721573 - Attachment is obsolete: true
(In reply to Nathan Froyd [:froydnj] from comment #25)
> (In reply to Mike Hommey [:glandium] from comment #24)
> > ::: config/rules.mk
> > @@ +963,5 @@
> > > +	$(RUSTC) $(RUSTFLAGS) --crate-type rlib --emit dep-info=$(MDDEPDIR)/$(call mk_libname,$<).pp,link=$(call mk_libname,$<) $(_VPATH_SRCS)
> > > +
> > > +$(RS_STATICLIB_CRATE_OBJ):
> > > +	$(REPORT_BUILD)
> > > +	$(RUSTC) $(RUSTFLAGS) --crate-type staticlib $(RLIB_EXTERN_CRATE_OPTIONS) --emit dep-info=$(MDDEPDIR)/$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)).pp,link=$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)) $(RS_STATICLIB_CRATE_SRC)
> > 
> > I don't see a reason not to use $< instead of $(RS_STATICLIB_CRATE_SRC), and
> > $(_VPATH_SRCS) for the last one, like in the $(RSOBJS) target.
> 
> Because $^ isn't just $(RS_STATICLIB_CRATE_SRC), for whatever reason.  I
> think this might have to do with it being a generated file.  These are the
> dependencies for $(RS_STATICLIB_CRATE_OBJ) in my build:
> 
> backend.mk Makefile ../../config/autoconf.mk
> /home/froydnj/src/gecko-dev.git/config/config.mk
> /opt/build/froydnj/build-rust-mc/toolkit/library/rul.rs

Figuring this was something with CommonBackend._write_file, I tried ensureParentDir + FileAvoidWrite...which results in the same thing, although rul.rs is no longer featured in $TOPOBJDIR/backend.RecursiveMakeBackend.
(In reply to Nathan Froyd [:froydnj] from comment #25)
> > I don't see a reason not to use $< instead of $(RS_STATICLIB_CRATE_SRC), and
> > $(_VPATH_SRCS) for the last one, like in the $(RSOBJS) target.
> 
> Because $^ isn't just $(RS_STATICLIB_CRATE_SRC), for whatever reason.  I
> think this might have to do with it being a generated file.  These are the
> dependencies for $(RS_STATICLIB_CRATE_OBJ) in my build:

But why $^ and not $< ? Or, said otherwise, why RS_STATICLIB_CRATE_SRC and not RS_STATICLIB_CRATE_OBJ?
Comment on attachment 8724116 [details] [diff] [review]
add build system support for multiple Rust crates, v5

Review of attachment 8724116 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +705,5 @@
> +                        # If we're building a shared library, we're going to
> +                        # auto-generate a module that includes all of the rlib
> +                        # files.  This means we can't permit Rust files as
> +                        # immediate inputs of the shared library.
> +                        shlib = context.get('SHARED_LIBRARY', None)

context.get('SHARED_LIBRARY') does the same thing... but there is no such variable in a context. You want `libname and shared_lib`.
Attachment #8724116 - Flags: feedback?(mh+mozilla) → feedback+
Newly-updated patch.
Attachment #8736005 - Flags: review?(mh+mozilla)
Attachment #8724116 - Attachment is obsolete: true
Attachment #8736005 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → nfroyd
Backed out for build bustage on Linux x64: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d158199a87cd8b0add756cfe41cd0bcc2afb496

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cb4b18566f30494ee06f9710293a086979c95c1a
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=25531665&repo=mozilla-inbound

10:38:27     INFO -  ../../../dom/media/gtest/Unified_cpp_dom_media_gtest0.o: In function `rust_CallFromCpp_Test::TestBody()':
10:38:27     INFO -  /builds/slave/m-in-l64-000000000000000000000/build/src/dom/media/gtest/TestRust.cpp:6: undefined reference to `test_rust'
10:38:27     INFO -  /builds/slave/m-in-l64-000000000000000000000/build/src/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: libxul.so: hidden symbol `test_rust' isn't defined
10:38:27     INFO -  /builds/slave/m-in-l64-000000000000000000000/build/src/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../x86_64-unknown-linux-gnu/bin/ld: final link failed: Bad value
10:38:27     INFO -  collect2: error: ld returned 1 exit status
10:38:27     INFO -  gmake[5]: *** [libxul.so] Error 1
10:38:27     INFO -  gmake[5]: Leaving directory `/builds/slave/m-in-l64-000000000000000000000/build/src/obj-firefox/toolkit/library/gtest'
10:38:27     INFO -  gmake[4]: *** [toolkit/library/gtest/target] Error 2
10:38:27     INFO -  gmake[4]: Leaving directory `/builds/slave/m-in-l64-000000000000000000000/build/src/obj-firefox'
10:38:27     INFO -  gmake[3]: *** [gtestxul] Error 2
10:38:27     INFO -  gmake[3]: Leaving directory `/builds/slave/m-in-l64-000000000000000000000/build/src/obj-firefox/toolkit/library'
10:38:27     INFO -  gmake[2]: *** [gtest] Error 2
10:38:27     INFO -  gmake[2]: Leaving directory `/builds/slave/m-in-l64-000000000000000000000/build/src/obj-firefox/testing/gtest'
10:38:27     INFO -  gmake[1]: *** [stage-gtest] Error 2
10:38:27     INFO -  gmake[1]: Leaving directory `/builds/slave/m-in-l64-000000000000000000000/build/src/obj-firefox'
10:38:27     INFO -  gmake: *** [automation/package-tests] Error 2
Flags: needinfo?(nfroyd)
This fix for the linking errors in gtestxul is almost simple enough that I'm
willing to commit as-is, but a second pair of eyes can't hurt.  We need to move
the Rust super-crate after anything else that might be using it, which means
after all of the static libraries we might be linking in.  It's not entirely
clear to me why linking libxul itself worked without this patch...

I would roll this patch up into the main patch for committing.
Attachment #8740110 - Flags: review?(mh+mozilla)
Attachment #8740110 - Flags: review?(mh+mozilla) → review+
Flags: needinfo?(nfroyd)
Lars asked me about this on IRC< and I figure it's worth posting here for anybody interested: this bug is ready to land, but it's not helpful to land it until bug 1178897 is ready to go.  Multiple crates in Gecko that don't use Gecko's allocator...that's probably not going to end well.

Bug 1178897 is blocked on custom allocators being usable in Rust stable, and the Rust RFC for that functionality is in its final comment period.  Ideally, the custom allocator stuff will land in Rust stable shortly, and then we can get this bug and bug 1178897 landed, and then the fun can (almost) start (there's probably a bit more build work to make multiple crates really build smoothly, e.g. handling interdependencies between crates).
Unfortunately custom allocators in Rust, as specified by [RFC 1183] likely won't be stable for quite some time. The "final comment period" was actually just for landing the RFC and unstable implementation, not for the stabilization itself.

If stable Rust is a **hard requirement** then we can try to stabilize some smaller pieces (without the whole surface area) ahead of time. From what I'm told the crate `alloc_system` that we ship suffices for Gecko on all platforms except Windows (as it just calls standard malloc/free). On Windows, however, it calls HeapAlloc/HeapFree. If we were to ship a crate like `alloc_malloc` which guaranteed called malloc/free on all platforms, would that suffice? That's a relatively easy extension for us to stabilize quickly if Gecko must absolutely run on stable.

[RFC 1183]: https://github.com/rust-lang/rfcs/blob/master/text/1183-swap-out-jemalloc.md
(In reply to Alex Crichton [:acrichto] from comment #37)
> Unfortunately custom allocators in Rust, as specified by [RFC 1183] likely
> won't be stable for quite some time. The "final comment period" was actually
> just for landing the RFC and unstable implementation, not for the
> stabilization itself.

Ah, sorry, I'm just completely misunderstanding the RFC process here.

> If stable Rust is a **hard requirement** then we can try to stabilize some
> smaller pieces (without the whole surface area) ahead of time. From what I'm
> told the crate `alloc_system` that we ship suffices for Gecko on all
> platforms except Windows (as it just calls standard malloc/free). On
> Windows, however, it calls HeapAlloc/HeapFree. If we were to ship a crate
> like `alloc_malloc` which guaranteed called malloc/free on all platforms,
> would that suffice? That's a relatively easy extension for us to stabilize
> quickly if Gecko must absolutely run on stable.

Depending on a non-released piece of software for part of building Firefox doesn't seem like a great idea, so I think stable Rust is a hard requirement.

Having an `alloc_malloc` crate or similar would be a welcome stopgap, though.  I think everything would Just Work if we had that, though I'd want to test it first.
It's in theory possible to test out today already by basically just writing the crate you already have, except not using jemalloc APIs on OSX (but instead using malloc/free). If you confirm that works for this use case, we can perhaps look to stabilize `alloc_malloc` or something like that in this or the next cycle!
(In reply to Nathan Froyd [:froydnj] from comment #36)

> it's not helpful to land [this bug] until bug 1178897 is ready to go.
> Multiple crates in Gecko that don't
> use Gecko's allocator...that's probably not going to end well.

Can you expand on your reasoning here? I've been anxiously awaiting this patch because doing the same thing by hand-maintained hacks is very painful, so landing this would be extremely helpful, and definitely moves the ball forward.

As far as I understand, the problem with the allocator variance is (a) crash potential if ownership is passed between rust and C++ and (b) lack of reporting. Those are both things we want to address, but in the meantime early adoptors can be careful while maintaining momentum. Blocking on those seems exactly backwards to me. The more rust code we have, the easier it is to make time to fix these things, and the sooner we'll be able to have safer code in gecko. Please land this so we can move on to the next piece.
(In reply to Ralph Giles (:rillian) from comment #40)
> (In reply to Nathan Froyd [:froydnj] from comment #36)
> > it's not helpful to land [this bug] until bug 1178897 is ready to go.
> > Multiple crates in Gecko that don't
> > use Gecko's allocator...that's probably not going to end well.
> 
> Can you expand on your reasoning here? I've been anxiously awaiting this
> patch because doing the same thing by hand-maintained hacks is very painful,
> so landing this would be extremely helpful, and definitely moves the ball
> forward.

What is "doing the same thing by hand-maintained hacks" in this context?  Are you talking about things like rust-uri or Stylo?  I would be surprised if this patch made them any easier to build without additional hacks, because this doesn't address inter-crate dependencies in any way.

I am entirely unaware of other efforts for adding Rust code to m-c, or even what the rust-uri folks/Stylo are dealing with on a day-to-day basis, but I would love to be enlightened on that front!

> As far as I understand, the problem with the allocator variance is (a) crash
> potential if ownership is passed between rust and C++ and (b) lack of
> reporting. Those are both things we want to address, but in the meantime
> early adoptors can be careful while maintaining momentum. Blocking on those
> seems exactly backwards to me. The more rust code we have, the easier it is
> to make time to fix these things, and the sooner we'll be able to have safer
> code in gecko.

Can you expand on these points?  When I think of early adopters, "careful" is not the phrase that comes to mind; I think of early adopters as "do this and we can fix it later" or "do this and don't think through the consequences".  Perhaps Gecko developers are more conservative than the early adopter picture I have in mind...but I doubt this.  And if we're not careful and we launch things before they're fully-baked, we have crashes to deal with and weird memory problems, which doesn't seem like a great way to announce our usage of this new-fangled Rust thing.

I also don't follow the line that more Rust code implies that it's easier to find time to fix things.  The only way I can get that to work is that the Rust code (presumably) takes less time to write, thereby freeing up time that would have otherwise gone toward feature development and can now be used for addressing technical debt...which does not sound like a convincing scenario.  But it's entirely possible that I am not sufficiently imaginative enough!

It also seems that if we did move forward with more Rust code, the primary blocker for fixing the memory allocator issue isn't anything in Gecko, but in Rust itself.  And that's not really under our (Platform's) control.
(In reply to Nathan Froyd [:froydnj] from comment #41)
> What is "doing the same thing by hand-maintained hacks" in this context? 
> Are you talking about things like rust-uri or Stylo?  I would be surprised
> if this patch made them any easier to build without additional hacks,
> because this doesn't address inter-crate dependencies in any way.

Rust-url and Stylo are two different beasts. vgosu is implementing rust-url by copying the sources in-tree and using the build system, and would be able to build on this work. Stylo builds a special target in Servo out of tree as a static lib, and you're right that it would not be able to use this work directly yet.

There's also interest in pulling in rust-encoding in the same manner as rust-url, now that it has addressed the design issues raised on dev-platform my hsivonen in the past.

So, I share your fear that people would move full-steam-ahead with their work, but today they're just building towers of one-off scripts in private branches or repositories rather than getting things landed behind a flag (or similar). Landing them would help us to flesh out one of the next big problems - how we do updates of the in-tree rust-url/rust-encoding bits from from the GitHub master repos.

This work would let us try out a variety of solutions to questions about where the Rust code we pull in would live (e.g., a single shared external directory, with a directory per crate?) and how any Gecko-specific m-c C++ or Rust that accesses those pieces of code would compile against it.

Once we have that problem figured out, it will be feasible to figure out how to build Stylo (which uses MANY Servo and community crates) in a more in-tree/vendored style.

> I am entirely unaware of other efforts for adding Rust code to m-c, or even
> what the rust-uri folks/Stylo are dealing with on a day-to-day basis, but I
> would love to be enlightened on that front!

The rust-url bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1151899) describes what they have to do right now, until this code lands.

There is an etherpad here that describes how to use the Stylo code:
https://public.etherpad-mozilla.org/p/stylo

Note "ac_add_options --disable-rust # Disables rust mp4 demuxer. Necessary for now to avoid linking rust libs twice."

> It also seems that if we did move forward with more Rust code, the primary
> blocker for fixing the memory allocator issue isn't anything in Gecko, but
> in Rust itself.  And that's not really under our (Platform's) control.

At the risk of speaking for acrichto here, the Rust team has been super responsive to Gecko/Platform's requests so far in this effort, and they plan to continue to be so, modulo process requirements around stability/public API changes. For example, in this case, I'd assume that we could get `alloc_malloc` in Rust Nightly almost immediately and update the required minimum Rust version if you build with multiple Rust libs after this patch.
On the stylo front: this bug itself doesn't alter the immediate dynamic for us, but it's really important to us that all of the ground work rust-in-gecko stuff be sorted out in the near term (within the next ~2 months). Sharing code with Servo adds a lot of additional complexity to the discussion, so it would be great if we at least had things relatively sorted out for the simple rust-code-in-gecko situation.

Why are we blocking on shared allocators, exactly? IIUC, that only matters if people try to allocate an object in one runtime and free it in another. This won't happen accidentally - the language barriers require this to be pretty explicit, and the obvious thing to do is to release resources with FFI calls.

Here is the code we use to map Box->UniquePtr and Arc->RefPtr in stylo:

http://searchfox.org/mozilla-central/source/layout/style/ServoBindingHelpers.h

Unless we can more-clearly articulate the scenarios that we're concerned about, I think we should move forward here.
(In reply to Nathan Froyd [:froydnj] from comment #41)

> What is "doing the same thing by hand-maintained hacks" in this context? 
> Are you talking about things like rust-uri or Stylo?  I would be surprised
> if this patch made them any easier to build without additional hacks,
> because this doesn't address inter-crate dependencies in any way.

I was referring, perhaps selfishly, to my own mp4 parser code, which is been in tree for months. It has dependent crates which I have to rewrite into modules every time I update them. But it absolutely also blocks landing *other* gecko components writen in rust, including rust-uri, stylo, and more speculative things that are still just ideas.

>                      When I think of early adopters, "careful"
> is not the phrase that comes to mind; I think of early adopters as "do this
> and we can fix it later" or "do this and don't think through the
> consequences".

Hmm. Perhaps. Matthew and I were the first people to land rust code in tree, and I feel like we've been very careful. Perhaps because we work with media data, where crashy code isn't really acceptable. There are also issues we won't discover until we start using things in distribution.

I understand your fear that this will get away from us, and quality will deteriorate. But you and I are, in fact, paying attention to these issues, as are several other experienced and careful developers. We can watch new rust code going in and offer our support to those developers.

> I also don't follow the line that more Rust code implies that it's easier to
> find time to fix things.

I meant urgency is a factor in the way we allocate developer resources. Having more people landing more rust code makes the need to address integration issues more relevant to more people. I fear if we delay rust deployment until we have more polished support, we'll have fewer people helping and it will take longer in the end.

> It also seems that if we did move forward with more Rust code, the primary
> blocker for fixing the memory allocator issue isn't anything in Gecko, but
> in Rust itself.  And that's not really under our (Platform's) control.

We can build custom toolchains. That has overhead (writing the patches, maintaining separate builds, installing them on build and developer machines) so our goal is stable, upstream rust. But it's not true that it's out of our control. More importantly, we have control of how we work with the rust team to address issues that affect rust use in Gecko. In my experience, they have been very supportive, as I think Alex's comments in this bug demonstrate.

I'd also say that people landing C++ code "we can fix later" (instead of rust) is
not going to improve our stability either. :-)

In the end I think it's important to land this now because rust is important for the future of gecko. It's been great having your help on the integration work, and patches like this are a clear improvement, a step toward where we want to go. But there's lots more to do, and having to rebase this, track its dependencies, even think about it distracts from that. Keeping work out of the tree is not the only lever we have to ensure quality. That's the responsiblity of the reviewers in each module, the object of a major platform initiative this year. I'm more concerned about our ability to move quickly to experiment in gecko, to enable more people to improve the quality of our engine. We won't get there without trusting each other more.
(In reply to Ralph Giles (:rillian) from comment #44)
> (In reply to Nathan Froyd [:froydnj] from comment #41)
> >                      When I think of early adopters, "careful"
> > is not the phrase that comes to mind; I think of early adopters as "do this
> > and we can fix it later" or "do this and don't think through the
> > consequences".
> 
> I understand your fear that this will get away from us, and quality will
> deteriorate. But you and I are, in fact, paying attention to these issues,
> as are several other experienced and careful developers. We can watch new
> rust code going in and offer our support to those developers.

As bholley said, the concerns over pointers being freed with the wrong runtime are probably not well-founded.  I think the heap fragmentation issues from using multiple heaps are a bigger problem in the short term, though we don't have a lot of Rust code currently, which reduces the fragmentation concern.  Memory visibility through reporting is also an issue, but a small one due to the amount of Rust code.

> > It also seems that if we did move forward with more Rust code, the primary
> > blocker for fixing the memory allocator issue isn't anything in Gecko, but
> > in Rust itself.  And that's not really under our (Platform's) control.
> 
> We can build custom toolchains. That has overhead (writing the patches,
> maintaining separate builds, installing them on build and developer
> machines) so our goal is stable, upstream rust. But it's not true that it's
> out of our control. More importantly, we have control of how we work with
> the rust team to address issues that affect rust use in Gecko. In my
> experience, they have been very supportive, as I think Alex's comments in
> this bug demonstrate.

For avoidance of doubt: I agree the Rust folks, and Alex in particular, have been super-responsive, and I'm grateful for that.

My comment about things not being under our control was directed at Rust features, and the delay in waiting for them to appear in stable Rust.  You can claim Rust is so safe that using unstable releases is OK, but that sounds a bit like "move fast and break things". ;)

> I'd also say that people landing C++ code "we can fix later" (instead of
> rust) is
> not going to improve our stability either. :-)

Touche!

I'll give landing this another go tomorrow; perhaps the problems are not as bad as I thought they were.
The problem with landing more rust code before the memory issue is fixed is that we're going to ship more and more rust code without the memory issue fixed. Because AIUI, rust code rode the trains and we *are* now shipping rust code, and that happened without the memory issue being fixed, despite being a blocker. Even better, I don't remember having heard something like "we're going to ship rust code, is that okay that we don't wait for the memory issue being fixed?". So while I can understand that people want convenience for their rust code, please understand my reluctance.
(In reply to Mike Hommey [:glandium] from comment #46)
> The problem with landing more rust code before the memory issue is fixed is
> that we're going to ship more and more rust code without the memory issue
> fixed. Because AIUI, rust code rode the trains and we *are* now shipping
> rust code, and that happened without the memory issue being fixed, despite
> being a blocker. Even better, I don't remember having heard something like
> "we're going to ship rust code, is that okay that we don't wait for the
> memory issue being fixed?". So while I can understand that people want
> convenience for their rust code, please understand my reluctance.

Not to Mike specifically, but prompted by Mike's comment:

Are we landing Rust code in m-c to experiment with things and figure out the best way forward prior to shipping, or are we landing Rust code in m-c with an intent to ship as soon as possible?  Because if it's the former, then sure, we can land things that make experimentation easier; we know it's not shipping, so it doesn't matter too much.  But if it's the latter, then being more cautious seems warranted.

The asks for getting this landed sound more like the first scenario to me.  But we're actually in the second, given that we have --enable-rust on our major desktop platforms.  (Not having --enable-rust on Android limits our ability to depend on Rust at the moment, but if we have Rust code going to Windows users, we are "shipping" for all intents and purposes, IMHO.)  What are we *really* trying to accomplish?

(Yes, there's a chicken-and-egg problem here, but I'm not sure "ship with known issues having unknown effects" is the right way to address that.)
If we determine that separate allocators should block us from shipping rust, then we should have someone work on that in the near term.

That said, it seems orthogonal to this bug, which has ergonomic advantages for experimenters even if we decide their code shouldn't ship.
(In reply to Bobby Holley (busy) from comment #48)
> If we determine that separate allocators should block us from shipping rust,
> then we should have someone work on that in the near term.

I think we are already "shipping rust":

https://dxr.mozilla.org/mozilla-central/search?q=mozconfig.rust&redirect=false&case=false

Every mozconfig in which we include mozconfig.rust is shipping rust; this is nightly/beta/release for all of our desktop platforms.

That being said, we should keep the amount of Rust we're using to a minimum until we have allocator support, because of the negative consequences.

> That said, it seems orthogonal to this bug, which has ergonomic advantages
> for experimenters even if we decide their code shouldn't ship.

It does have ergonomic advantages!  But since we are apparently shipping, landing this bug means that landing more Rust code can make things worse in the short term.  It sounds to me like you're envisioning that every piece of Rust code that lands should be behind a separate feature flag for experimentation purposes, though--could that be a fair construal of your above statement?  I could get behind that as a stopgap until we get the memory issues ironed out.
(In reply to Nathan Froyd [:froydnj] from comment #49)
> (In reply to Bobby Holley (busy) from comment #48)
> > If we determine that separate allocators should block us from shipping rust,
> > then we should have someone work on that in the near term.
> 
> I think we are already "shipping rust":
> 
> https://dxr.mozilla.org/mozilla-central/search?q=mozconfig.
> rust&redirect=false&case=false
> 
> Every mozconfig in which we include mozconfig.rust is shipping rust; this is
> nightly/beta/release for all of our desktop platforms.

IIUC that wasn't originally planned, but relman made the call to do it for some reason. Anyway, it's beside the point here.

> That being said, we should keep the amount of Rust we're using to a minimum
> until we have allocator support, because of the negative consequences.
> 
> > That said, it seems orthogonal to this bug, which has ergonomic advantages
> > for experimenters even if we decide their code shouldn't ship.
> 
> It does have ergonomic advantages!  But since we are apparently shipping,
> landing this bug means that landing more Rust code can make things worse in
> the short term.  It sounds to me like you're envisioning that every piece of
> Rust code that lands should be behind a separate feature flag for
> experimentation purposes, though--could that be a fair construal of your
> above statement?  I could get behind that as a stopgap until we get the
> memory issues ironed out.

Yes. My point is that the machinery in this patch is orthogonal to the policy question of how much rust we want to ship. Presumably new rust-implemented things should do a better job of "intent to ship" than the mp4 demuxer did, and I'm assuming they will going forward.

My secondary point is that this a problem we will need to have solved very soon, and it sound straightforward according to Alex. So ideally we'd just land this code now and land the allocator fix shortly thereafter.
https://hg.mozilla.org/mozilla-central/rev/841c2247f57d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.