Closed
Bug 1469027
Opened 6 years ago
Closed 6 years ago
baldrdash: running wasm with Cranelift
Categories
(Core :: JavaScript: WebAssembly, enhancement)
Core
JavaScript: WebAssembly
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: sunfish, Assigned: bbouvier)
References
(Depends on 1 open bug)
Details
Attachments
(6 files, 14 obsolete files)
10.00 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
17.73 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
104.92 KB,
patch
|
luke
:
review+
sunfish
:
review+
lth
:
feedback+
|
Details | Diff | Splinter Review |
2.67 MB,
patch
|
Details | Diff | Splinter Review | |
14.24 KB,
patch
|
mhoye
:
review+
|
Details | Diff | Splinter Review |
23.57 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Attached is the current "baldrdash" patch, which adds a nightly-only option for running wasm with Cretonne. Cretonne is in development and doesn't yet have all its planned optimization features, however it is able to pass the testsuite on x64. The patch has the following known issues: - Currently JS shell builds require the environment variable MOZ_CLANG_PATH to be set to a clang binary. This appears to be needed by the bindgen infrastructure. Is there a better way to handle this? - The JS shell builds, but full firefox builds currently fail with "mozbuild.configure.ConfigureError: Cannot include `[...]/build/moz.configure/rust.configure` because it was included already.". It appears rust.configure is being included by both toolkit/moz.configure and js/moz.configure. Is there a way to make it conditional on whether the JS shell is being built? - Cretonne depends on the failure crate, which isn't yet in firefox. Is this a problem? If so, it'd be possible to remove this dependency or make it optional. - wasmparser.rs currently includes 7.4M of test binaries that will get copied into the tree by the vendoring script. Is that too much? - Cretonne depends on target-lexicon (which I also maintain), which is licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license, however crates.io is still using an older SPDX, so target-lexicon currently uses a license-file. I added it to the whitelist in python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary workaround.
Comment 1•6 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #0) > - Cretonne depends on target-lexicon (which I also maintain), which is > licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license, > however crates.io is still using an older SPDX, so target-lexicon currently > uses a license-file. I added it to the whitelist in > python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary > workaround. Is this tracked in a GitHub issue? It seems easy enough to do to have it block this. The alternative would probably be to reach out to licensing people (i.e. mhoye), which might be more effort. (OTOH, I don't know if reaching out to the licensing people would still be needed.)
Reporter | ||
Comment 2•6 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #1) > Is this tracked in a GitHub issue? It seems easy enough to do to have it > block this. The alternative would probably be to reach out to licensing > people (i.e. mhoye), which might be more effort. (OTOH, I don't know if > reaching out to the licensing people would still be needed.) It's being tracked here: https://github.com/rust-lang-nursery/license-exprs/issues/12
Comment 3•6 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #0) > Attached is the current "baldrdash" patch, which adds a nightly-only option > for running wasm with Cretonne. A+ name
Updated•6 years ago
|
Component: JavaScript Engine: JIT → Javascript: Web Assembly
![]() |
||
Comment 4•6 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #0) > - Currently JS shell builds require the environment variable MOZ_CLANG_PATH > to be set to a clang binary. This appears to be needed by the bindgen > infrastructure. Is there a better way to handle this? We can move the logic for determining where bindgen bits live: https://searchfox.org/mozilla-central/source/toolkit/moz.configure#666-809 somewhere where JS and Gecko can see it. > - The JS shell builds, but full firefox builds currently fail with > "mozbuild.configure.ConfigureError: Cannot include > `[...]/build/moz.configure/rust.configure` because it was included > already.". It appears rust.configure is being included by both > toolkit/moz.configure and js/moz.configure. Is there a way to make it > conditional on whether the JS shell is being built? Can you have JS-only builds without building the shell? If so, we have a couple of different cases: - Gecko builds, Rust required - JS-only builds, shell built, Rust required - JS-only builds, shell not built, Rust...not required (yet?)? I think this is doable, but there will be some configure bits to sort out. I'm assuming we'd like to keep whatever Rust bits the JS engine requires Nightly-only at this point? Alternatively, do we want to take a hard dependency on Rust in the JS engine now, or wait until ESR 69/70? > - Cretonne depends on the failure crate, which isn't yet in firefox. Is > this a problem? If so, it'd be possible to remove this dependency or make it > optional. Assuming the failure crate is reasonable (no huge files, no obvious badness), I don't see why this would be a problem. > - wasmparser.rs currently includes 7.4M of test binaries that will get > copied into the tree by the vendoring script. Is that too much? I think so. Is it possible to separate those out into a test-only crate? I'm not sure if cargo-vendor will pull those in or not... > - Cretonne depends on target-lexicon (which I also maintain), which is > licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license, > however crates.io is still using an older SPDX, so target-lexicon currently > uses a license-file. I added it to the whitelist in > python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary > workaround. You can ask for license review from mhoye and we can add that as a permanent exception. I assume we'll also want some sort of configure flag --enable-cretonne or somesuch, defaulted to on in Nightly and off everywhere else, rather than the hardcoded defines the patch currently has.
Reporter | ||
Comment 5•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #4) > (In reply to Dan Gohman [:sunfish] from comment #0) > > - wasmparser.rs currently includes 7.4M of test binaries that will get > > copied into the tree by the vendoring script. Is that too much? > > I think so. Is it possible to separate those out into a test-only crate? > I'm not sure if cargo-vendor will pull those in or not... I've now filed https://github.com/yurydelendik/wasmparser.rs/issues/55 about moving the tests out. > > - Cretonne depends on target-lexicon (which I also maintain), which is > > licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license, > > however crates.io is still using an older SPDX, so target-lexicon currently > > uses a license-file. I added it to the whitelist in > > python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary > > workaround. > > You can ask for license review from mhoye and we can add that as a permanent > exception. I've now filed bug 1476388 to request a license review.
Reporter | ||
Comment 6•6 years ago
|
||
Here's an updated patch. Major changes: - Update to the latest Cretonne/Cranelift - Merge with trunk, and implement realm switching - Add the "Apache-2.0 WITH LLVM-exception" license to the whitelist, now that bug 1476388 is resolved. Also note that in this patch, as well as the previous one, I've excluded the Cargo.lock and third_part/rust changes from this patch, as they're mechanically generated from "mach vendor rust".
Attachment #8985677 -
Attachment is obsolete: true
Updated•6 years ago
|
Alias: cranelift
Summary: baldrdash: running wasm with Cretonne → baldrdash: running wasm with Cranelift
Reporter | ||
Comment 7•6 years ago
|
||
This version adds Cranelift's license to the about:license page, as requested in bug 1476388.
Attachment #8993114 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
(rebased and updated to use cranelift 0.17.0)
Attachment #8994362 -
Attachment is obsolete: true
Assignee | ||
Comment 9•6 years ago
|
||
(rebased, without Cargo.lock changes this time)
Attachment #8995990 -
Attachment is obsolete: true
Assignee | ||
Comment 10•6 years ago
|
||
More changes since the ValType refactoring.
Assignee | ||
Comment 11•6 years ago
|
||
As agreed on IRC with Dan, I'll take over this bug to get it landed. I'm currently rebasing this patch, updating Cranelift and adapting the new APIs, while ensuring tests still pass.
Assignee: sunfish → bbouvier
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•6 years ago
|
||
Rolled up wip. - updated to cranelift 0.20; - tried using bindgen for the ModuleEnvironment; failed b/o mozilla::Vector and other data structures in there (which use templates etc., which bindgen doesn't seem to support out of the box); - started to remove all the calls between Spidermonkey and Cranelift; the code and metadata is now stored in a Rust structure that's passed out to C++ afterwards, so code generation entirely happens in C++. (no more need for the Emitter structure either) Did remove a lot of bindings.
Attachment #8996306 -
Attachment is obsolete: true
Attachment #8996307 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Alias: cranelift
Assignee | ||
Comment 13•6 years ago
|
||
Rolled up patch (applies cleanly on top of mozilla-inbound 345269b39c6c), with vendoring etc. Tried to simplify a few things, attempting some different designs, etc. There are cranelift register allocation failures when trying this patch on AngryBots/Tanks, but I'd like to make sure I didn't introduce them first.
Attachment #9005714 -
Attachment is obsolete: true
Assignee | ||
Comment 14•6 years ago
|
||
We'd like to always include Cranelift in Nightly builds, or on demand for non-Nightly. This will increase the binary's size, compilation time (both Spidermonkey and full Gecko); I haven't measured by how much though. Nathan, does it make sense? Is it the right way to do this with the configure machinery?
Attachment #9006604 -
Attachment is obsolete: true
Attachment #9007273 -
Flags: review?(nfroyd)
Assignee | ||
Comment 15•6 years ago
|
||
Adds a --wasm-force-cranelift shell flag, which does nothing at the moment but to set the option in the JS options struct.
Attachment #9007274 -
Flags: review?(luke)
Assignee | ||
Comment 16•6 years ago
|
||
Introduces the Tier::Cranelift, which can be used as a second tier and binds it to the wasm-force-cranelift shell flag. With these patches, it will just compile with IonMonkey; later patches will make use of Cranelift is Tier is Tier::Cranelift.
Attachment #9007275 -
Flags: review?(luke)
Assignee | ||
Comment 17•6 years ago
|
||
This is a preparatory patch for the next one, mostly code motion. It introduces a new header called WasmBasicTypes.h, containing types that are then included in WasmTypes.h or bindgen'd for use in Baldrdash. The changes are the following: - code motion (for some types) - the FuncWithTypeId::Kind is extracted to its own enum class - a new CPU feature detection of SSE4.2 is added to the Assembler class.
Attachment #9007278 -
Flags: review?(luke)
Assignee | ||
Comment 18•6 years ago
|
||
This is part of the main patch (the rest is automatic generated output, license updates and moz.build goo). Lars, flagged you for feedback since you wrote this code in the first place. If you'd like to review it too, feel free to do so; the more eyes on this big patch, the better. This builds on all platforms, and passes tests with --wasm-force-cranelift on x64. On other platforms, since SSE2 is marked as not present (see #ifdef in CraneliftStaticEnvironment ctor), the cranelift_compiler_create function call will fail in make_isa(); so ARM / x86 / others will gracefully fail with a Rust error message ("SSE2 is not present"), reported as an OOM (better error reporting will come in subsequent bugs). My plan is to try to land this as soon as possible, then have many other bugs to make the integration better/faster, rename things, harmonize names/casing in FFIs, etc.
Attachment #9007289 -
Flags: review?(sunfish)
Attachment #9007289 -
Flags: review?(luke)
Attachment #9007289 -
Flags: feedback?(lhansen)
Assignee | ||
Comment 19•6 years ago
|
||
All the build bits, mostly copied from what was in previous patches.
Attachment #9007290 -
Flags: review?(nfroyd)
Assignee | ||
Comment 20•6 years ago
|
||
The output of ./mach vendor rust. Nathan, I assume this needs to be reviewed/approved? Is there a documented process, if approval is needed?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 21•6 years ago
|
||
Mark, this patch contains the license changes that have been discussed in bug 1476388 and bug 1476798. This will be folded with the rest of the patches numbered 5, which introduce the code the new license applies to.
Attachment #9007294 -
Flags: review?(mhoye)
![]() |
||
Comment 22•6 years ago
|
||
Comment on attachment 9007273 [details] [diff] [review] 1.cranelift-nightly.patch Review of attachment 9007273 [details] [diff] [review]: ----------------------------------------------------------------- I believe this is correct.
Attachment #9007273 -
Flags: review?(nfroyd) → review+
Comment 23•6 years ago
|
||
Comment on attachment 9007294 [details] [diff] [review] 5d-license-changes.patch R+, thank you.
Attachment #9007294 -
Flags: review?(mhoye) → review+
![]() |
||
Comment 24•6 years ago
|
||
Comment on attachment 9007290 [details] [diff] [review] 5b-build-support.patch Review of attachment 9007290 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the RUST_STATIC_LIB_FOR_SHARED_LIB change mentioned below, and assuming there's a reasonable answer to the necessity of the .hgignore change. ::: .hgignore @@ +43,5 @@ > _DBG\.OBJ/ > _OPT\.OBJ/ > ^js/src/.*-obj/ > ^js/src/obj-.*/ > +^js/src/wasm/cranelift/target Please add this to .gitignore as well. But wait, why is this necessary? Shouldn't the build be putting the output files in the objdir, not the srcdir? Or is this an attempt to ease local development for people who just want to `cargo build` cranelift and nothing else? ::: config/rules.mk @@ +552,5 @@ > # > # PROGRAM = Foo > # creates OBJS, links with LIBS to create Foo > # > +$(PROGRAM): $(PROGOBJS) $(RUST_STATIC_LIB_FOR_SHARED_LIB) $(STATIC_LIBS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) $(call mkdir_deps,$(FINAL_TARGET)) I think it would also be a good idea to rename RUST_STATIC_LIB_FOR_SHARED_LIB at this point, since we're now using Rust static libraries in other places than shared libraries. :) ::: js/src/gdb/moz.build @@ +46,5 @@ > + > +# The Rust standard library references libresolv on macOS, so we need to link > +# it as a workaround. See also bug 1367932. > +if CONFIG['OS_ARCH'] == 'Darwin': > + OS_LIBS += ['-lresolv'] I don't think you have to fix this here, but maybe we should just start linking in dependencies for Rust libraries in the build system itself, rather than scattering this stanza around in a bunch of different places. Please file a followup bug?
Attachment #9007290 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 25•6 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #20) > Created attachment 9007292 [details] [diff] [review] > 5c-mach-vendor-rust.patch > > The output of ./mach vendor rust. > > Nathan, I assume this needs to be reviewed/approved? Is there a documented > process, if approval is needed? It does need to be approved, by me or by someone who has experience with the crates being modified. AFAICT, this is just trivially updating `log` and then adding a bunch of cranelift packages. Assuming that doesn't change, r=me.
Flags: needinfo?(nfroyd)
![]() |
||
Updated•6 years ago
|
Attachment #9007274 -
Flags: review?(luke) → review+
![]() |
||
Comment 26•6 years ago
|
||
Comment on attachment 9007275 [details] [diff] [review] 3.tier::cranelift.patch Review of attachment 9007275 [details] [diff] [review]: ----------------------------------------------------------------- Question: do we want Ion and Cranelift to be separate tiers or could we instead: - rename Tier::Ion to Tier::Optimized - have a bool that indicates whether to use Cranelift for the optimized tier This would make sense under the assumption that most code that is asking about tier doesn't care about the ion vs. cranelift distinction (since they produce code with the same ABI / metadata). It seems like mostly a ModuleGenerator-specific concern, but I could be wrong. What's nice is that this isn't a long-lived distinction; once cranelift "ships", it can go away so the bool that decides which to use could be #ifdef CRANELIFT.
![]() |
||
Comment 27•6 years ago
|
||
Comment on attachment 9007278 [details] [diff] [review] 4.bindgen-d-types.patch Review of attachment 9007278 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for breaking out these patches. ::: js/src/wasm/WasmBasicTypes.h @@ +19,5 @@ > +#ifndef wasm_basic_types_h > +#define wasm_basic_types_h > + > +// This header defines basic enums and structs that don't depend on other header files being > +// included. It is included by "wasm/WasmTypes.h" AND by bindgen when making bindings for Cranelift. WasmBinaryConstants.h also doesn't #include anything, could it serve this purpose instead? It currently contains some structs that maybe trip up bindgen (OpBytes), so perhaps those could be moved to WasmTypes.h? Also, the "Binary" in "WasmBinaryConstants.h" seems superfluous/confusing, in retrospect, perhaps rename to just WasmConstants.h (which makes it's role here a bit more clear)?
Attachment #9007278 -
Flags: review?(luke) → review+
Comment 28•6 years ago
|
||
Comment on attachment 9007289 [details] [diff] [review] 5a-cranelift-integration.patch Review of attachment 9007289 [details] [diff] [review]: ----------------------------------------------------------------- Seems OK to me, for a first cut. ::: js/src/wasm/cranelift/src/lib.rs @@ +93,5 @@ > + error!("Cranelift compilation error: {}\n{}", e, compiler); > + return false; > + }; > + > + // TODO(bbouvier) if destroy is called while one of these objects is alive, you're gonna have a "going to have"
Attachment #9007289 -
Flags: feedback?(lhansen) → feedback+
Assignee | ||
Comment 29•6 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #26) > Comment on attachment 9007275 [details] [diff] [review] > 3.tier::cranelift.patch > > Review of attachment 9007275 [details] [diff] [review]: > ----------------------------------------------------------------- > > Question: do we want Ion and Cranelift to be separate tiers or could we > instead: > - rename Tier::Ion to Tier::Optimized > - have a bool that indicates whether to use Cranelift for the optimized tier > > This would make sense under the assumption that most code that is asking > about tier doesn't care about the ion vs. cranelift distinction (since they > produce code with the same ABI / metadata). It seems like mostly a > ModuleGenerator-specific concern, but I could be wrong. What's nice is that > this isn't a long-lived distinction; once cranelift "ships", it can go away > so the bool that decides which to use could be #ifdef CRANELIFT. That's how it was done in the patch previously, but there was one thing that troubled me: Tier::Serialized == Tier::Ion (and thus would be equal to Tier::Cranelift), so just having Tier::Optimized could mean that Cranelift code gets serialized/deserialized. Since Ion is still generating better code at the moment, I thought not fusing the two would prevent performance issues for people testing cranelift in a browser build. Also, we could have different heuristics for tiering baseline with cranelift, which is the only other interest. Reverting to the previous situation would be just a bit bad in my opinion, but I don't care too strongly here.
Comment 30•6 years ago
|
||
Comment on attachment 9007273 [details] [diff] [review] 1.cranelift-nightly.patch Review of attachment 9007273 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/moz.configure @@ +487,5 @@ > help='Enable Cranelift code generator for wasm') > > +@depends('--enable-cranelift', milestone) > +def enable_cranelift(value, milestone): > + return value or milestone.is_nightly This makes --disable-cranelift *not* work on central. We avoid doing things like this. If you want the default to be enabled for central, use default=milestone.is_nightly on the js_option().
Attachment #9007273 -
Flags: review+ → review-
Comment 31•6 years ago
|
||
Comment on attachment 9007290 [details] [diff] [review] 5b-build-support.patch Review of attachment 9007290 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/wasm/cranelift/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +RustLibrary('baldrdash') Adding if CONFIG['OS_ARCH'] == 'Darwin': OS_LIBS += ['-lresolv'] here should make USE_LIBS += ['baldrdash'] imply it, removing the need to it in all the other files.
Assignee | ||
Comment 32•6 years ago
|
||
Thanks! (Also incorporated the change you suggested on the moz.build files in the moz.build patch)
Attachment #9007273 -
Attachment is obsolete: true
Attachment #9007771 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 33•6 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9347f5a5365d Add a --wasm-force-cranelift flag to the shell; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/b7954f956259 Move wasm types shared with Cranelift in a WasmBinaryConstants.h; r=luke
Assignee | ||
Comment 34•6 years ago
|
||
Attachment #9007771 -
Attachment is obsolete: true
Attachment #9007771 -
Flags: review?(mh+mozilla)
Attachment #9007795 -
Flags: review?(mh+mozilla)
![]() |
||
Comment 35•6 years ago
|
||
Comment on attachment 9007275 [details] [diff] [review] 3.tier::cranelift.patch Review of attachment 9007275 [details] [diff] [review]: ----------------------------------------------------------------- Ah, Tier::Serialized is a good point. Moreover, when cranelift ships, we can delete Tier::Ion and so this is only a transitional state.
Attachment #9007275 -
Flags: review?(luke) → review+
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9347f5a5365d https://hg.mozilla.org/mozilla-central/rev/b7954f956259
![]() |
||
Updated•6 years ago
|
Attachment #9007289 -
Flags: review?(luke) → review+
Updated•6 years ago
|
Attachment #9007795 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 37•6 years ago
|
||
Comment on attachment 9007289 [details] [diff] [review] 5a-cranelift-integration.patch Review of attachment 9007289 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me :-).
Attachment #9007289 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 38•6 years ago
|
||
Thanks all for the reviews! Alright, some status update here: I'm fighting with the build gods at the moment, and have succeeded at solving a few issues. Another bunch of issues needs to be solved before landing. - build issues: to be linked with libmozjs, baldrdash must have "staticlib" (static library) as its crate-type; to be linked with libxul, it must be a "rlib" (rust object lib) due to the way we link rust code in gecko. These requirements are conflicting: in particular, rust static libs are built with LTO, while rlib can't be. The solution, as suggested by Nathan, was to split baldrdash into two libs: - baldrdash, used by Gecko (and baldrdash_lib below), which is an rlib - baldrdash_lib, used only by the shell, and defined as a static library, which just references baldrdash. Thus, baldrdash_lib needs its own Cargo.toml file, and needs to reference the source of baldrdash in its dependencies, so its root directory must be a parent of the baldrdash lib. As a matter of fact, all the code that was in /js/src/wasm/cranelift is now in /js/src/wasm/cranelift/lib; baldrdash_lib's Cargo.toml and dummy lib.rs files live by themselves in /js/src/wasm/cranelift. (Will upload a patch for all the build changes) Happy to change the `lib` name to something else, if people have better suggestions. - more build issues: the hazard analysis builds don't reference libclang, which is needed by bindgen. Nathan and Steve gave me useful help on IRC to fix this. - even more build issues: Android builds need more metadata for bindgen. Emilio on irc pointed what needed to be done for the style system, which should be helpful. - rust-bindgen doesn't support bindings generation for C++ methods attributes on windows: I'll revert to function calls that take what was the value of "this" as the first argument, on all platforms. - in Firefox, env_logger::init() was called by JS after it was already initialized in Gecko; replaced by a call to try_init() which doesn't fail if a logger has already been set.
Comment 39•6 years ago
|
||
If you create a jsrust staticlib library similar to gkrust, you can then have baldrdash as a rlib linked into either gtkrust or jsrust.
Comment 40•6 years ago
|
||
I should add that linking baldrdash into gkrust is probably not something we can do, not as long as we support building firefox with spidermonkey as a shared library.
Comment 41•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #40) > I should add that linking baldrdash into gkrust is probably not something we > can do, not as long as we support building firefox with spidermonkey as a > shared library. Why do we need to support that? That is, is there a reason why we can't aim for a setup where SpiderMonkey-relevant Rust code is built as jsrust in standalone SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part of Firefox without support for SpiderMonkey as a shared library in Firefox?
Flags: needinfo?(mh+mozilla)
Comment 42•6 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #41) > (In reply to Mike Hommey [:glandium] from comment #40) > > I should add that linking baldrdash into gkrust is probably not something we > > can do, not as long as we support building firefox with spidermonkey as a > > shared library. > > Why do we need to support that? People have been using that to make incremental builds faster. Now that there are fast linking options, this might be less necessary, but it's not a given. > That is, is there a reason why we can't aim for a setup where > SpiderMonkey-relevant Rust code is built as jsrust in standalone > SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part > of Firefox without support for SpiderMonkey as a shared library in Firefox? As mentioned in another bug, there's actually another problem, that e.g. the js shell, that is still built for Firefox, is not linking gecko for good reason, and still needs the spidermonkey-specific rust bits.
Flags: needinfo?(mh+mozilla)
Comment 43•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #42) > (In reply to Henri Sivonen (:hsivonen) from comment #41) > > (In reply to Mike Hommey [:glandium] from comment #40) > > > I should add that linking baldrdash into gkrust is probably not something we > > > can do, not as long as we support building firefox with spidermonkey as a > > > shared library. > > > > Why do we need to support that? > > People have been using that to make incremental builds faster. Now that > there are fast linking options, this might be less necessary, but it's not a > given. That doesn't seems like a great reason to prevent SpiderMonkey from reusing code that Gecko has access to. > > That is, is there a reason why we can't aim for a setup where > > SpiderMonkey-relevant Rust code is built as jsrust in standalone > > SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part > > of Firefox without support for SpiderMonkey as a shared library in Firefox? > > As mentioned in another bug, there's actually another problem, that e.g. the > js shell, that is still built for Firefox, is not linking gecko for good > reason, and still needs the spidermonkey-specific rust bits. I looked at a couple of obvious dependencies and didn't find a discussion of js shell issues. Which bug do you mean?
Flags: needinfo?(mh+mozilla)
Comment 44•6 years ago
|
||
bug 1490948, I think, but maybe the discussion was only on irc.
Flags: needinfo?(mh+mozilla)
Comment 45•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #44) > bug 1490948, I think, but maybe the discussion was only on irc. Does anyone remember the details? (Asking in order to understand the issues facing bug 1490601.)
Assignee | ||
Comment 46•6 years ago
|
||
Sure, will report there, since I've moved all the build work in this bug.
Assignee | ||
Comment 47•6 years ago
|
||
Just wanted a double-check of this patch after the serialization tests landed. Mostly the same, except we need to explicitly check that the serialized tier isn't the second tier before calling the serialization listener.
Attachment #9007275 -
Attachment is obsolete: true
Attachment #9011768 -
Flags: review?(luke)
![]() |
||
Comment 48•6 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #29) > > Question: do we want Ion and Cranelift to be separate tiers or could we > > instead: > > - rename Tier::Ion to Tier::Optimized > > - have a bool that indicates whether to use Cranelift for the optimized tier > That's how it was done in the patch previously, but there was one thing that > troubled me: Tier::Serialized == Tier::Ion (and thus would be equal to > Tier::Cranelift), so just having Tier::Optimized could mean that Cranelift > code gets serialized/deserialized. Since Ion is still generating better code > at the moment, I thought not fusing the two would prevent performance issues > for people testing cranelift in a browser build. Actually, thinking about this a bit more, it seems totally fine (and beneficial for testing purposes) to serialize cranelift code. First of all, it'll only get serialized when the force-cranelift option is set which will only be set for testing purposes. Second, even if someone sets it and caches some suboptimal cranelift code (before we enable cranelift by default), the code is build-id keyed so as soon as the browser updates we'll compile new code. So I don't think there is any reason to distinguish Tier::Ion from Tier::Cranelift for serialization purposes. > Also, we could have different heuristics for tiering baseline with > cranelift, which is the only other interest. It's probably fine to just use the Ion constants and then tweak those constants based on re-measurements before cranelift release. So based on that, how about having just Tier::Baseline and Tier::Optimized?
Assignee | ||
Comment 49•6 years ago
|
||
Here's the patch, which is a conceptual regression I think (more conditions to check to know if we're compiling for Cranelift, meaningless OptimizedBackend enum field in more cases in the CompilerEnv, etc.), but, oh well, I don't want to fight over this.
Attachment #9011768 -
Attachment is obsolete: true
Attachment #9011768 -
Flags: review?(luke)
Attachment #9012112 -
Flags: review?(luke)
![]() |
||
Comment 50•6 years ago
|
||
Comment on attachment 9012112 [details] [diff] [review] 3.new-switch.patch Review of attachment 9012112 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!
Attachment #9012112 -
Flags: review?(luke) → review+
Comment 51•6 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7df169d72b50 Introduce a new switch to determine if we should compile with Cranelift; r=luke
Comment 52•6 years ago
|
||
Backed out for assertion failures on env.optimizedBackend() == OptimizedBackend::Ion. Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=201723721&repo=mozilla-inbound&lineNumber=12313
Flags: needinfo?(bbouvier)
Comment 53•6 years ago
|
||
There were also jit failures on WasmCompile.cpp Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=201721372&repo=mozilla-inbound&lineNumber=12285
Comment 54•6 years ago
|
||
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1c85d872cc Backed out changeset 7df169d72b50 for assertion failures on env.optimizedBackend() == OptimizedBackend::Ion. CLOSED TREE
Assignee | ||
Comment 55•6 years ago
|
||
Duh, optimized build + uninitialized memory in !ENABLE_WASM_CRANELIFT build = kaboom. Thanks for the backout.
Flags: needinfo?(bbouvier)
Comment 56•6 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2456b585ef87 Introduce a new switch to determine if we should compile with Cranelift; r=luke
Assignee | ||
Comment 57•6 years ago
|
||
Comment on attachment 9007290 [details] [diff] [review] 5b-build-support.patch This work has moved to bug 1490948.
Attachment #9007290 -
Attachment is obsolete: true
Assignee | ||
Comment 58•6 years ago
|
||
Comment on attachment 9007795 [details] [diff] [review] 1.cranelift-nightly.patch This work has moved to bug 1490948.
Attachment #9007795 -
Attachment is obsolete: true
Comment 59•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2456b585ef87
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 60•6 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7864f125bbcd Integrate Cranelift in Spidermonkey; r=sunfish, r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/8a644dda1dc3 Vendor in Rust dependencies for Cranelift; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/66a1434be89e License changes; r=mhoye
Assignee | ||
Comment 61•6 years ago
|
||
Last try build went all green (except for tier2 Btup, to be fixed in bug 1494139): https://treeherder.mozilla.org/#/jobs?repo=try&revision=282c9c7435616db59cd4ad672ea876c883ea3333
Comment 62•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7864f125bbcd https://hg.mozilla.org/mozilla-central/rev/8a644dda1dc3 https://hg.mozilla.org/mozilla-central/rev/66a1434be89e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
status-firefox62:
affected → ---
Comment 63•6 years ago
|
||
Hey all, I'm trying to compile Firefox for the first time on my new MacBook, and am running into the following error: 9:45.75 Compiling baldrdash v0.1.0 (/Users/bwinton/Programming/mozilla/central/js/src/wasm/cranelift) 9:46.99 error: failed to run custom build command for `baldrdash v0.1.0 (/Users/bwinton/Programming/mozilla/central/js/src/wasm/cranelift)` 9:46.99 process didn't exit successfully: `/Users/bwinton/Programming/mozilla/central/obj-x86_64-apple-darwin18.0.0/toolkit/library/release/build/baldrdash-4511ac70b523b040/build-script-build` (exit code: 101) 9:46.99 --- stdout 9:46.99 cargo:rerun-if-changed=baldrapi.h 9:46.99 cargo:rerun-if-changed=/Users/bwinton/Programming/mozilla/central/obj-x86_64-apple-darwin18.0.0/js/src/rust/extra-bindgen-flags 9:46.99 --- stderr 9:47.00 /usr/local/Cellar/llvm/7.0.0/lib/clang/7.0.0/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found 9:47.00 /usr/local/Cellar/llvm/7.0.0/lib/clang/7.0.0/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found, err: true 9:47.00 thread 'main' panicked at 'Unable to generate baldrapi.h bindings: ()', libcore/result.rs:1009:5 9:47.00 stack backtrace: 9:47.00 0: 0x105b64e0f - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::heb6afa683932dedf 9:47.00 1: 0x105b44c7d - std::sys_common::backtrace::print::h766ca70084cc68f0 9:47.00 2: 0x105b6dae3 - std::panicking::default_hook::{{closure}}::hf3402f90591bf7bb 9:47.00 3: 0x105b6d86c - std::panicking::default_hook::hb3b7f3a59e2ecd5a 9:47.00 4: 0x105b6e217 - std::panicking::rust_panic_with_hook::h359374823d95520e 9:47.00 5: 0x105b6dd7c - std::panicking::continue_panic_fmt::hf199d5eb60536380 9:47.00 6: 0x105b6dc68 - rust_begin_unwind 9:47.00 7: 0x105bd20f1 - core::panicking::panic_fmt::he16a30f6c103fffa 9:47.00 8: 0x104ee97be - core::result::unwrap_failed::h9d03696a608dbad4 9:47.00 9: 0x104ef0fd5 - build_script_build::main::h0a86b4b58e0c8b60 9:47.00 10: 0x104ee99f5 - std::rt::lang_start::{{closure}}::he5e0228125c8f22e 9:47.00 11: 0x105b6dbe7 - std::panicking::try::do_call::h2bd103f24173fc3c 9:47.00 12: 0x105b7b5fe - __rust_maybe_catch_panic 9:47.00 13: 0x105b5192c - std::rt::lang_start_internal::h6fb67bdf1de01d4a 9:47.00 14: 0x104ee99e7 - std::rt::lang_start::h0f8cdf02aa015a6e It seems like it might be related to this bug, so I wonder if there's something in the bootstrap.py or build setup instructions that needs to be changed to get it compiling? Thanks!
Comment 64•6 years ago
|
||
(Oh, I should probably also mention that I'm on macOS Mojave, which could very well be the problem.)
Comment 65•6 years ago
|
||
Chiming in that that I'm getting identical output to Blake's after the update to macOS Mojave.
![]() |
||
Comment 66•6 years ago
|
||
You are both running into bug 1487522.
You need to log in
before you can comment on or make changes to this bug.
Description
•