mach vendor rust should now work reliably on windows

[Alexis Beingessner]

We just landed some new checks [0] for `mach vendor rust` that should make
it behave reliably on Windows. If you were unaware, the command had a
tendency to produce different results on Windows, up to and including
deleting random files!

The issues were upstream cargo bugs that have all been fixed in cargo
1.47.0, which is still currently in beta. If you wish to vendor on windows
in the next ~month, you may need to install and use beta or nightly. The
vendor command will enforce this to prevent further issues.

To make beta your default toolchain, you can run these commands in the top
level directory of your gecko checkout:

```
rustup toolchain install beta
rustup override set beta
./mach configure
```

(The `configure` is required to update the cached toolchain path that `mach
vendor rust` uses)

If you ever wish to revert to stable, then you just need to:

```
rustup override set stable
./mach configure
```

Have a great day!

[0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1647582]
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: mozilla::Result is about to understand move semantics

[Alexis Beingessner]

Just chiming in here with some brief extra context on the performance of
Result (which I really need to do a writeup about so that I can just link
it):

TL;DR performance of Result is usually fine, but can randomly be a huge
problem. However there's also improvements for this on the (distant)
horizon.

Rust and Swift primarily use an error handling strategy based on Result.
For the most part performance is fine, but in some situations you can get
really bad problems. LLVM is very bad at optimizing around Results, and
tends to have copy and branch heavy codegen as a result (which further
hinders other optimizations). This was enough of an issue for the binary
deserializer webrender uses for IPC (bincode) that we just landed a rewrite
to remove the Results (after several attempts by myself to fix the issues
in other ways). [0]

Meanwhile, the Swift compiler team used their expertese in llvm to add new
custom ABIs and calling conventions to handle these performance issues (the
right fix, imo). [1] I need to interview them on these details, to figure
out if we can use their work for Rust. (Since runtime performance is mostly
excellent, it's difficult to motivate working on this and diverting the
limited resources of the Rust team away from the bigger issues like compile
times and async-await.)

Also Meanwhile, the C++ standards committee is apparently[2] investigating
introducing new calling conventions for their new light-weight exceptions
proposal (which is basically adding Result to C++ properly). [3] If that
work goes forward we should be able to take advantage of it for our own C++
code, and possibly also for Rust.

Gonna work on that writeup of this issue now.


[0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1550640
[1]: https://lists.llvm.org/pipermail/llvm-dev/2016-March/096250.html
[2]:
https://botondballo.wordpress.com/2019/07/26/trip-report-c-standards-meeting-in-cologne-july-2019/
[3]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0709r3.pdf

On Tue, Aug 13, 2019 at 1:01 PM Kris Maglione  wrote:

> On Mon, Aug 12, 2019 at 10:14:19PM -0700, Bryce Seager van Dyk wrote:
> >>But either way, that's going to result in a copy when the
> >>Result is constructed (unless the compiler is really clever).
> >
> >Is it the data being moved into the Result which is incurring
> >the copy here, or the actual Result that's being returned?
>
> The former.
>
> >I would have thought that the data is moved into the Result
> >avoids a copy, then the Result itself would be moved or RVOed
> >(either way avoiding a copy).
>
> The move into the result only means that we invoke move rather
> than copy constructors when initializing the value stored in the
> result. That's more efficient for a lot of things, but still
> requires copying data from the old struct to the new one.
>
> The return value Result is guaranteed to be optimized, though,
> so you only wind up with a single copy rather than two.
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Fission Newsletter #2

[Alexis Beingessner]

Is dogfoodability at all platform-specific for fission? i.e. is windows the
only platform that is really actively developed/maintained? (as would make
sense at this stage)

More concretely, I was under the impression that fission had webrender as a
dependency, is that mandatory? Is it actually enforced by the fission pref?
(webrender's support of different platforms has varying levels of quality,
although it is generally dogfoodable on all major platforms)

On Fri, Aug 9, 2019 at 1:43 PM Nika Layzell  wrote:

> Looks like gmail chewed up the formatting :-S
>
> Published gdocs link:
>
> https://docs.google.com/document/d/e/2PACX-1vTuGpZNthNxk0OYRyBjiHpaKnyKdmb9AompceuncvFmjeXB0bfk-L_LSlQmRaqiqx8vKif-LzdnE2F8/pub
>
>
> On Fri, Aug 9, 2019 at 1:33 PM Nika Layzell  wrote:
>
> > Hey all!
> >
> > It's been a while (7 months!) since the first Fission newsletter, but
> > we've made some exciting progress we'd love to tell you about!
> >
> > Enabling Fission on Nightly
> >
> > It's now possible to turn on Fission in nightly builds of Firefox by
> > setting fission.autostart pref to true. Fission can also be enabled for
> > running tests using mach test … --enable-fission.
> >
> > When Fission is enabled, each cross-site iframe is loaded in a different
> > content process, meaning lots of different processes participate in
> drawing
> > a single tab. The hover tooltip for a Fission-enabled tab is annotated
> with
> > a "[F …]" containing a series of process IDs, as shown in the image
> below,
> > serving as a visual verification of an active Fission-enabled session.
> >
> >
> > We currently do not recommend trying to use Fission for day-to-day
> > browsing, as there are still known stability issues. However, if you do
> try
> > it out, please file bugs/issues blocking fission-dogfooding
> > .
> >
> > Fission Mochitests on mozilla-central
> >
> > Fission Mochitests were recently enabled as tier-2 jobs on
> mozilla-central.
> > This will allow us to run tests with Fission enabled on infrastructure,
> and
> > prevent landing new features or code which don't support Fission. Tests
> > which do not currently successfully pass are marked as fail-if = Fission
> > or skip-if = Fission.
> >
> > We'd love your help migrating tests to run with Fission enabled! Here are
> > a couple of handy tips for making your test Fission-compatible:
> >
> >1.
> >
> >Use SpecialPowers.spawn(target, [args…], async (args…) => { … }), to
> >run code in potentially cross-origin iframes, as they may be in a
> different
> >process. This API is similar to the ContentTask.spawn API used by
> >browser-chrome mochitests.
> >2.
> >
> >Wait for document loads to complete before trying to run code inside
> >the target window, as a process switch may occur after the frame or
> browser
> >is created. For frames in content, this usually means waiting for the
> >load event.
> >
> >
> > These tests may also be run on the tryserver, however they are currently
> > excluded from the default set. They are called M-fis, and can be found
> in ./mach
> > try fuzzy --full.
> >
> > Fixing these Mochitests is a goal of our next major milestone, M4!
> There's
> > a ton of awesome stuff happening in M4, which you can read about on the
> > wiki (https://wiki.mozilla.org/Project_Fission#M4_goals).
> >
> > Fission Talk and Demo
> >
> > At the 2019 Whistler All-Hands, Nika gave a talk & demo about the Fission
> > architecture. This talk is publicly available on Air Mozilla.
> >
> > You can watch the talk here:
> >
> https://onlinexperiences.com/Launch/Event.htm?ShowKey=44908=E334923
> >
> > The slides are here:
> >
> https://docs.google.com/presentation/d/1equyaJTujM4xF-ucoMZiLE-lo0lbHKFMliUfPE4_1B8/edit?usp=sharing
> >
> > And… So Much More!
> >
> > A ton has happened in Fission over those 7 months, and it would be
> > impossible to cover all of the awesome work everyone has been
> contributing
> > to make Fission happen. We just want to say a massive thank you to
> > everyone who has helped with Fission - writing patches, doing reviews,
> > planning, and more! We hope to do brief update newsletters like this one
> > with a better cadence, so hopefully there'll be another one of these in
> > your inbox soon.
> >
> > Let's keep fission-on!
> >
> > - The Fission Team.
> >
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Enabling (many) assertions in opt builds locally and eventually Nightly

[Alexis Beingessner]

With webrender we've had pretty good results from always defaulting to
MOZ_RELEASE_ASSERT, as users are often mildly decent at producing an STR.
But perhaps we're luckier in that everyone dogfooding webrender was more
willing to figure things out than a nightly user?

On Mon, Sep 17, 2018 at 3:31 PM Botond Ballo  wrote:

> One potential issue here is that an assertion failure in Nightly
> builds might be simultaneously very annoying (because it crashes the
> browser of Nightly users every time it occurs) and not very actionable
> (because no reliable STR can be found for it). That's the situation
> I'm in with bug 1457603, for example, where I may have to end up
> _downgrading_ a diagnostic assert that I added, to a regular assert.
>
> On Mon, Sep 17, 2018 at 3:25 PM, Jeff Gilbert 
> wrote:
> > I would be excited for something between MOZ_ASSERT and MOZ_CRASH, but
> > I think raising MOZ_ASSERTs to MOZ_ASSERT_NIGHTLY should be done by
> > hand and reviewed.
> >
> > On Mon, Sep 17, 2018 at 11:46 AM, Kris Maglione 
> wrote:
> >> There are some non-trivial snags for this proposal. A lot of assertions
> rely
> >> on other code gated on #ifdef DEBUG or DebugOnly<...> to avoid defining
> data
> >> members or storing values in non-debug builds. We could replace those
> with
> >> more fine-grained macros, of course, but particularly in the case of
> data
> >> members, we'd probably also take a significant memory usage regression.
> >>
> >> There's also the snag of NS_ASSERTION macros which are... weird.
> >>
> >> This is nothing we can't deal with, of course, but it will probably
> require
> >> a lot of manual work if we want to do it correctly.
> >>
> >>
> >> On Mon, Sep 17, 2018 at 02:38:14PM -0400, Randell Jesup wrote:
> >>>
> >>> There are quite a few things that may be caught by assertions by
> >>> developers before committing, especially sec issues - but most
> >>> developers don't run debug builds as their main local test builds, or
> >>> for local browsing use, because they are almost unusably slow.  And
> >>> turning on compiler optimization doesn't actually help much here - the
> >>> problem is mostly debug-only assertions and code that's debug only,
> such
> >>> as bug 1441052 ("Don't do full grey-node checking in local debug
> >>> builds").
> >>>
> >>> These added checks may be useful for CI tests, but they make the builds
> >>> unusable, so the vast majority of assertions don't run in the builds
> our
> >>> developers are using.  So enabling most of the existing MOZ_ASSERTs for
> >>> local opt builds (minus the worst performance-limiting ones) would help
> >>> developers catch bugs before landing them (and perhaps shipping
> >>> them). It's a lot like using ASAN builds to browse - but with much less
> >>> perf degradation on hopes.
> >>>
> >>> Even better, if we can make the overall hit to perf low enough (1%?),
> we
> >>> could enable it for some/all Nightly users/builds.  Or make "early
> >>> nightly" (and developer builds) enable them, and late nightly and beta
> >>> not.
> >>>
> >>> If we moved some of the most expensive checks to an alternative macro
> >>> (MOZ_ASSERT_DEBUG()), we could (selectively?) enable MOZ_ASSERT checks
> >>> in some opt builds.  Alternatively we could promote some MOZ_ASSERTs to
> >>> MOZ_ASSERT_NIGHTLY() (or MOZ_DIAGNOSTIC_ASSERT), which would assert in
> >>> (all) debug builds, and in nightly opt builds - and maybe promote some
> >>> to MOZ_ASSERT_RELEASE if we can take the perf hit, and they're in an
> >>> important spot.
> >>>
> >>> And as a stepping-stone to the above, having local opt builds enable
> >>> (most) MOZ_ASSERTs (excluding really expensive ones, like bug 1441052)
> >>> even if the hit is larger (5, 10%) would also increase the likelihood
> >>> that we'll catch things before we land them, or before they get to
> beta.
> >>> (Note: --enable-release would turn this local-build behavior off - and
> >>> anyone doing speed tests/profiling/etc needs that already, and probably
> >>> we should have a specific enable/disable like
> >>> --disable-extra-assertions).  This wouldn't be all that hard - most of
> >>> the work would be finding "expensive" assertions like bug 1441052.
> >>>
> >>> (and perhaps we should not continue to overload --enable-release for "I
> >>> want to benchmark/profile this build")
> >>>
> >>> Enabling most MOZ_ASSERTs in automation tests on opt builds would also
> >>> slightly increase our odds of finding problems - though this may not
> >>> help much, as the same tests are being run in debug builds.  The only
> >>> advantage would be races may be more likely in the faster opt builds.
> >>> It might be worth trying once we have this for a few weeks, and see if
> >>> it helps or not.
> >>>
> >>> Note: I've discussed this concept with various people including bz in
> >>> the past, and mostly have had agreement that this would be useful -
> thus
> >>> my filing of bug 1441052.  If we agree this is worth doing, we should
> >>> file bugs on it and 

Re: Tagged pointers

[Alexis Beingessner]

On Thu, Jul 12, 2018 at 11:03 PM, Robert O'Callahan 
wrote:

> On Fri, Jul 13, 2018 at 11:40 AM, Steve Fink  wrote:
>
> > On 07/12/2018 04:27 PM, Cameron McCormack wrote:
> >
> >> On Fri, Jul 13, 2018, at 6:51 AM, Kris Maglione wrote:
> >>
> >>> I actually have a patch sitting around with helpers to make it super
> >>> easy to
> >>> use smart pointers as tagged pointers :) I never wound up putting it up
> >>> for
> >>> review, since my original use case went away, but it you can think of
> any
> >>> specific cases where it would be useful, I'd be happy to try and get it
> >>> landed.
> >>>
> >> Speaking of tagged pointers, I've used lower one or two bits for tagging
> >> a number of times, but I've never tried packing things into the high
> bits
> >> of a 64 bit pointer.  Is that inadvisable for any reason?  How many bits
> >> can I use, given the 64 bit platforms we need to support?
> >>
> >
> > JS::Value makes use of this. We preserve the bottom 47 bits, but that's
> > starting to be problematic as some systems want 48. So, stashing stuff
> into
> > the high 16 bits is pretty safe!
> >
>
> 57-bit address space support is coming for x86-64.
>
> Rob
>

Last I heard the 48-bit assumption had become so pervasive (and useful)
that
OS devs were planning to only expose this to processes that explicitly
opted into it.

In the case of linux, you would have to explicitly request high-address
pages to
start receiving them: https://lwn.net/Articles/717293/

I always assumed firefox simply wouldn't ever opt into high-addresses,
unless
fission mem-shrink doesn't work out and firefox suddenly needs 300TB of RAM
;p
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: incremental compilation for opt Rust builds

[Alexis Beingessner]

Oh and here's the one documented in the nightly docs, just for completeness

# The benchmarking profile, used for `cargo bench` and `cargo test
--release`.[profile.bench]opt-level = 3debug = falserpath = falselto =
falsedebug-assertions = falsecodegen-units = 1panic =
'unwind'incremental = falseoverflow-checks = false



On Tue, Mar 13, 2018 at 9:24 AM, Alexis Beingessner <a.beingess...@gmail.com
> wrote:

> The defaults of the various cargo profiles are documented here:
> https://doc.rust-lang.org/cargo/reference/manifest.html
>
> The relevant entry is:
>
> # The benchmarking profile, used for `cargo bench` and `cargo test 
> --release`.[profile.bench]opt-level = 3debug = falserpath = falselto = 
> falsedebug-assertions = falsecodegen-units = 1panic = 'unwind'
>
> Note that we always build with panic=abort which improves codegen; not
> sure about which conditions we use lto for rust code off the top of my head.
>
>
> On Tue, Mar 13, 2018 at 3:10 AM, Henri Sivonen <hsivo...@hsivonen.fi>
> wrote:
>
>> On Tue, Mar 13, 2018 at 2:56 AM, Nathan Froyd <nfr...@mozilla.com> wrote:
>> > (Our release builds use -O2 for Rust code.)
>>
>> What does cargo bench use by default?
>> (https://internals.rust-lang.org/t/default-opt-level-for-rel
>> ease-builds/4581
>> suggests -O3.)
>>
>> That is, is cargo bench for a crate that's vendored into m-c
>> reflective of that crate's performance when included in a Firefox
>> release?
>>
>> --
>> Henri Sivonen
>> hsivo...@hsivonen.fi
>> https://hsivonen.fi/
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: incremental compilation for opt Rust builds

[Alexis Beingessner]

The defaults of the various cargo profiles are documented here:
https://doc.rust-lang.org/cargo/reference/manifest.html

The relevant entry is:

# The benchmarking profile, used for `cargo bench` and `cargo test
--release`.[profile.bench]opt-level = 3debug = falserpath = falselto =
falsedebug-assertions = falsecodegen-units = 1panic = 'unwind'

Note that we always build with panic=abort which improves codegen; not sure
about which conditions we use lto for rust code off the top of my head.


On Tue, Mar 13, 2018 at 3:10 AM, Henri Sivonen  wrote:

> On Tue, Mar 13, 2018 at 2:56 AM, Nathan Froyd  wrote:
> > (Our release builds use -O2 for Rust code.)
>
> What does cargo bench use by default?
> (https://internals.rust-lang.org/t/default-opt-level-for-
> release-builds/4581
> suggests -O3.)
>
> That is, is cargo bench for a crate that's vendored into m-c
> reflective of that crate's performance when included in a Firefox
> release?
>
> --
> Henri Sivonen
> hsivo...@hsivonen.fi
> https://hsivonen.fi/
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Actually-Infallible Fallible Allocations

[Alexis Beingessner]

TL;DR: we're using fallible allocations without checking the result in
cases where we're certain there is enough space already reserved. This is
confusing and potentially dangerous for refactoring. Should we stop doing
this?

-

I was recently searching through our codebase to look at all the ways we
use fallible allocations, and was startled when I came across several lines
that looked like this:

dom::SocketElement  = *sockets.AppendElement(fallible);

For those who aren't familiar with how our allocating APIs work:

* by default we hard abort on allocation failure
* but if you add `fallible` (or use the Fallible template), you will get
back nullptr on allocation failure

So in isolation this code is saying "I want to handle allocation failure"
and then immediately not doing that and just dereferencing the result. This
turns allocation failure into Undefined Behaviour, rather than a process
abort.

Thankfully, all the cases where I found this were preceded by something
like the following:

uint32_t length = socketData->mData.Length();if
(!sockets.SetCapacity(length, fallible)) {   JS_ReportOutOfMemory(cx);
  return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
socketData->mData.Length(); i++) {dom::SocketElement  =
*sockets.AppendElement(fallible);



So really, the fallible AppendElement *is* infallible, but we're just
saying "fallible" anyway. I find this pattern concerning for two reasons:

* It's confusing to anyone who encounters this for the first time.
* It's a time bomb for any bad refactoring which makes the allocations
*actually* fallible

I can however think of two reasons why this might be desirable

* It encourages the optimizer to understand that the allocations can't
fail, removing branches (dubious, these branches would predict perfectly if
they exist at all)

* It's part of a guideline/lint that mandates only fallible allocations be
used in this part of the code.

If the latter is a concern, I would much rather we use infallible
allocations with a required comment, or some other kind of decoration to
state "this is fine". In the absence of a checker that can statically prove
the the AppendElement calls will never fail (which in the given example
would in fact warn that we don't use `length` in the loop condition), I
would rather mistakes lead to us crashing more, rather than Undefined
Behaviour.

Thoughts?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: More Rust code

[Alexis Beingessner]

I'm currently trying to improve the C++ <-> Rust FFI story a bit. I just
posted a draft proposal to add a mode to rustc that has it output the ABI
details of all public types:
https://internals.rust-lang.org/t/stabilizing-a-machine-readable-zprint-type-sizes/5525

This would theoretically reduce our maintenance/conversion burden at the
FFI boundary.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform