PSA: Please clear weak reference to cycle collected objects on unlink

[Kris Maglione]

Accessing an object after it has been unlinked by the cycle collector is a 
certain way to cause trouble. Usually, that trouble is just in the form of 
null pointer crashes, but it can potentially be much worse. And the most 
common cause of those types of issues comes from accessing a weak pointer for 
an object after it has been unlinked, but before it has been finalized. The 
solution to that entire class of problem is to always clear weak references to 
an object when it is unlinked.


Bug 1535617 adds some helper macros for this (NS_IMPL_CYCLE_COLLECTION_UNLINK_WEAK_REFERENCE 
and NS_IMPL_CYCLE_COLLECTION_UNLINK_WEAK_PTR, along with some other 
convenience macros along the lines of NS_IMPL_CYCLE_COLLECTION(...)), and fixes 
most existing classes which implement both cycle collection and weak 
references. Any new such classes will need to do the same.


Thanks,
Kris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Intent to Unship: in content

[Kris Maglione]

 in non-chrome documents has been unsupported 
in production environments for some time. Maintaining support 
for it in content processes has become increasingly difficult in 
the light of Fission, to the point that we've decided it is time 
to fully remove support. Since support is disabled by default, 
and there is no user-visible way to enable it, there should be 
no users in the wild. The only in-tree users are tests, which 
will need to be updated by their owners or disabled.


Bug 1614462 will track this work.


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

Re: How best to do async functions and XPCOM?

[Kris Maglione]

On Thu, Dec 12, 2019 at 09:30:06AM -0500, Boris Zbarsky wrote:

On 12/10/19 3:31 PM, Kris Maglione wrote:

In what way is dom::Promise annoying to use from C++?


The one thing I know about that's pretty annoying is if you receive 
the promise from someone else and want to add reactions to it. 
PromiseNativeHandler kinda works, but then you get JS::Values and have 
to extract the things you care about from them manually, which is a 
bit of a pain.


Note that you can also use `ThenWithCycleCollectedArgs` and 
`ThenWithoutCycleCollection` to add reactions from native code, which is 
a lot easier than `PromiseNativeHandler`. You do still need to manually 
deal with JS::Values, though.

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

Re: How best to do async functions and XPCOM?

[Kris Maglione]

On Sun, Dec 08, 2019 at 11:40:52PM -0500, Joshua Cranmer  wrote:
The problem with MozPromise is that it doesn't integrate well if you 
use XPIDL interfaces, so you have this annoying issue that if you want 
to use XPIDL integration, you have to use mozilla::dom::Promise, which 
is annoying to use from C++.


In what way is dom::Promise annoying to use from C++? The entire purpose of 
dom::Promise is to make JS promises easy to use from C++.

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

Re: How best to do async functions and XPCOM?

[Kris Maglione]

On Fri, Dec 06, 2019 at 11:20:12AM +1300, Geoff Lankow wrote:

Hi all

I'm redesigning a bunch of Thunderbird things to be asynchronous. I'd 
like to use Promises but a lot of the time I'll be far from a JS 
context so that doesn't really seem like an option. The best 
alternative I've come up with is to create some sort of listener 
object and pass it to the async function:


interface nsIFooOperationListener : nsISupports {
 void onOperationComplete(
   in nsresult status,
   [optional] in string errorMessage
 );
};

...

void fooFunction(..., in nsIFooOperationListener listener);

This works fine but I wonder if there's a better way, or if there's 
some established prior art I can use/borrow rather than find out the 
pitfalls myself.


In general, if you're thinking about using an XPIDL interface for something, 
you're probably going down the wrong track. If you're only going to be 
dealing with C++, then you should generally use a pure C++ interface 
(probably just MozPromise in this case). If you're going to need to interact 
with JS at any point, you should probably just use dom::Promise. You can 
always use AutoJSAPI to get a JSContext, and create the Promise in the 
shared JSM global.

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

Re: Consider avoiding allSettled in tests (was: Re: Intent to ship: Promise.allSettled)

[Kris Maglione]

On Thu, Oct 31, 2019, 06:02 Jason Orendorff  wrote:

> Ignoring the awaited value here is like using `catch {}` to squelch all
> exceptions, or ignoring the return value of an async function or method, or
> any other expression that produces a Promise. Do we have lints for those
> pitfalls? I'm kind of surprised that there doesn't seem to be a built-in
> ESLint lint for `catch {}`.
>

Some directories have an ESLint rule to disallow empty blocks, which
requires that any empty catch blocks at least contain a comment to justify
them.

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

Fission Mochitests moving to tier 1 and must all† pass by October 31st

[Kris Maglione]

Fission Milestone 4 is focused on getting all mochitests running and passing 
with Fission enabled. The deadline for the end of this milestone is October 
31st, approximately one month from today, and we still have a significant 
amount of work to do, with about 350 mochitests skipped and about 100 running 
but failing under Fission.


This is a high priority goal for the entire organization, which means that 
everyone is responsible for getting the tests that they maintain to pass. The 
DOM Fission team is available to help and to answer questions (you can find us 
in #domfission on IRC and #fission on Slack), but ultimately every team is 
responsible for its own tests.


You can check how many tests from your components are currently skipped or 
failing in the Are We Fission Yet dashboard: https://arewefissionyet.com/m4/


And you can get a more detailed lists of specific tests in our tracking 
spreadsheet‡: 
https://docs.google.com/spreadsheets/d/1kjp32JTuB4axM3wKx0iIYL2Ado-HcyeBhoY-siGxYAs/edit


There are also some basic tips for making tests Fission-compatible on the 
wiki: https://wiki.mozilla.org/Project_Fission/Enabling_Tests_with_Fission and 
in-tree docs with more details about working with Fission in non-test code: 
https://firefox-source-docs.mozilla.org/dom/dom/Fission.html For any questions 
not answered there, please reach out to the Fission team, and we will answer 
them as best we can.



Fission mochitests have been running on tier 2 for about a month now. We 
believe at this point that they are stable enough to run on every check-in, so 
in the month leading up to our deadline, they are moving to tier 1. Any 
check-in which causes a new M-fis failure will immediately be backed out, as 
will any patch which adds a new `skip-if` or `fail-if` annotation for Fission 
mode without approval from a member of the Fission team.



I also want to particularly thank Brian Grinstead for putting together the Are 
We Fission Yet dashboards, and Geoff Brown and Andrew Halberstadt for helping 
us get the data that we need to generate the dashboards and spreadsheets from 
treeherder.



† With some exceptions for tests of features which still require major 
platform work to become Fission compatible, and are in the M5 milestone. 
Moving tests to a later milestone requires approval from Project Fission 
management, and exceedingly strong justification.


‡ Only accessible to Mozilla staff. A read-only version is viewable at
https://docs.google.com/spreadsheets/d/e/2PACX-1vRRmnRUOy-KDDScK8o8Z6aKRaEtXKXb39Yn2OOPXoMgZwcMC3Oce3jgSjI5-jRK0jLS73gQYLkfSTJ_/pubhtml?gid=156071=true
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to remove: Fennec

[Kris Maglione]

On Tue, Sep 24, 2019 at 10:07:28AM -0700, mark.erik...@gmail.com wrote:
- I am very happy with the current Fennec app and its UI, and 
don't understand why Mozilla feels a need to drop that product 
and create a new one from scratch.


We're not creating a new one from scratch. Many of the component 
parts of Fenix already existed, and the core is basically the 
same as Fennec. That said, there are many reasons:


1) GeckoView, which is a more or less drop-in replacement for 
Android's WebView, is designed to be used by any application 
which needs to embed web content on Android, including 
potentially other web browsers. It's an important part of our 
core mission to make sure the web remains a viable living 
standard, with multiple competing implementations, to avoid the 
sort of stagnation and vendor lock-in we saw when IE6 
essentially ruled the world.


2) We also already needed to maintain WebView-based browser 
front-ends for configurations where shipping our entire 
rendering back-end was not viable or practical. Given that we 
already need to maintain these separate, already-compatible 
front-end and back-end implementations, having to maintain an 
entirely separate browser with a completely different front-end, 
and a lot of different back-end glue, is just not a good use of 
resources.


3) GeckoView and the Fenix front-end are much more modern 
frameworks than Fennec. They were architected from the ground up 
using all of the knowledge and experience that we've gained 
developing Fennec and a number of other experimental browsers 
over the years. The result is that they are not only much easier 
to develop and maintain, but also much faster and more resource 
efficient.


4) The current Fennec browser, unlike Fenix and our desktop 
browsers, is very much single-process by design. Web content 
runs in the same process as the browser UI. The native Java UI 
allows us to use threading to work around some of the 
performance problems inherent in this sort of design, but it 
doesn't give us any of the security properties of process 
isolation, which are becoming increasingly important in the age 
of Spectre attacks. If we wanted to continue maintaining Fennec 
apart from Fenix, we would need to drastically rearchitect it to 
support process isolation for web content. And, given that 
Fenix and GeckoView were designed to handle this from the start, 
again, that would just not be a good use of resources.


--
Kris Maglione

UNIX is simple.  It just takes a genius to understand its simplicity.
--Dennis Ritchie

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

Re: Nika Layzell and Kris Maglione are now XPCOM peers

[Kris Maglione]

On Wed, Sep 4, 2019, 11:33 Nathan Froyd  Kris noticed that Nika was going to become an XPCOM peer and, not
> wanting to be left out, volunteered (yes, really).  Kris has worked on
> modernizing the component manager and various thread-related
> improvements.
>

To be clear, I volunteered because erahm said "we really need more XPCOM
peers" :p

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

Re: Must we rebuild all our rust code constantly?

[Kris Maglione]

On Tue, Aug 20, 2019 at 02:23:06PM +0900, ISHIKAWA,chiaki wrote:

On 2019/08/20 9:11, Dave Townsend wrote:

Thanks to a tip I've tracked this down. This seems to only be the case when
I have sccache enabled. Disabling it gives me nice quick incremental builds
again. Of course that isn't an ideal solution but it will do for now.

On Mon, Aug 19, 2019 at 1:55 PM Dave Townsend  wrote:


For a couple of weeks now I've seen that any attempt to build Firefox,
even incremental builds seem to rebuild an awful lot of rust code. I found
this in the source which seems to suggest why:
https://searchfox.org/mozilla-central/source/config/makefiles/rust.mk#238.
But, this means that now an incremental build with a couple of code change
that have no impact on rust is taking upwards of 4 minutes to complete in
comparison to around 40 seconds, and the log file is full of cargo output.
I've heard similar comments from other developers.

This is a pretty big increase in the time to compile and test and is
really slowing down my work. Is there any way we can avoid this?






I am using linux for local development and noticed something similar.

So I should disable sccache (!?).


For what it's worth, Rust is now configured to use incremental 
compilation, which has its own cache which isn't cleared after 
clobber, so sccache isn't actually needed anymore. Ordinary 
ccache should be sufficient.

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

Re: Must we rebuild all our rust code constantly?

[Kris Maglione]

This is apparently a known bug that no-one seems to be able to 
track down the cause of. It suddenly started happening to me one 
night for every build, even if I changed nothing. Then, just as 
suddenly, stopped happening after a couple of hours.


On Mon, Aug 19, 2019 at 05:11:19PM -0700, Dave Townsend wrote:

Thanks to a tip I've tracked this down. This seems to only be the case when
I have sccache enabled. Disabling it gives me nice quick incremental builds
again. Of course that isn't an ideal solution but it will do for now.

On Mon, Aug 19, 2019 at 1:55 PM Dave Townsend  wrote:


For a couple of weeks now I've seen that any attempt to build Firefox,
even incremental builds seem to rebuild an awful lot of rust code. I found
this in the source which seems to suggest why:
https://searchfox.org/mozilla-central/source/config/makefiles/rust.mk#238.
But, this means that now an incremental build with a couple of code change
that have no impact on rust is taking upwards of 4 minutes to complete in
comparison to around 40 seconds, and the log file is full of cargo output.
I've heard similar comments from other developers.

This is a pretty big increase in the time to compile and test and is
really slowing down my work. Is there any way we can avoid this?

___
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

[Kris Maglione]

Just for some additional extra context...

The C++ mozilla::Result implementation is pretty heavily optimized for 
common use cases to try to pack its representation into a single machine 
word when possible. For instance, if you return a `Result`, 
you're really just returning an `nsresult`. If you return a 
`Result`, you're really just returning a `T*`. If you return a 
`Result`, you're really just returning a `void*`. For the 
first two, there isn't any extra overhead compared to not returning a 
`Result`. For the third, there are a couple of extra bitwise  which might 
get optimized out anyway.


It's also fairly easy to add new optimized packing strategies for other 
common patterns as they come up.


For more complex types, we currently return a struct. Those calls compile as 
the caller allocating space on the stack for the result type and passing in 
a pointer (though this varies slightly depending on OS ABI; sometimes larger 
types are returned in multiple registers, or large SIMD registers), and the 
compiler is generally required to perform copy elision on the return value 
if you return a temporary or a local of the same type, i.e., when you write:


 Thing foo() {
   Thing bar;
   bar.mThing = 1;
   return bar;
 }

`bar` essentially just refers to the pointer that the caller passed in.

The real problem comes when storing large values in, or extracting large 
values from, the `Result` itself. In general, if you construct the `Result` 
with a named value of any sort, even with `std::move`, it will have to be 
copied (unless the compiler can guarantee that there will be no observable 
side-effects of omitting the copy, and it's willing to try that hard to 
prove that there aren't).


If the `Result` is constructed with a temporary, e.g.,

 Result foo() {
   return Thing { ... };
 }

Then the copy of the temporary `Thing` to the `Result` *may* be omitted, and 
is realistically quite likely to be. But if you're doing something like this 
in particularly hot code, I'd at least check machine code clang generates 
before going very far with it.


-Kris

On Tue, Aug 13, 2019 at 01:37:49PM -0400, Alexis Beingessner wrote:

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 ret

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

[Kris Maglione]

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

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

[Kris Maglione]

On Mon, Aug 12, 2019 at 11:16:17PM +0200, Emilio Cobos Álvarez wrote:


On 8/12/19 10:38 PM, Bryce Seager van Dyk wrote:

On Monday, August 12, 2019 at 8:41:26 AM UTC-7, Emilio Cobos Álvarez wrote:

Neat! Thanks for doing this. It should allow for broader use of Result in media 
code.

Are there any footguns to watch out for when moving into the Result? Looking at the 
changes it appears that if my return type is Result then I can just 
return a BigStruct? I.e. I don't need to do explicitly invoke anything else to avoid 
unintended copies.


I think you need to use `return std::move(bigStruct);`, since 
otherwise you peek the `const T&` constructor rather than the `T&&` 
constructor. Before my patch, you always get a copy when returning it 
(because Result only took const references).


With my patch, if you `return bigStruct;`, your code will compile and 
do the same copy, but you'll get a warning from the compiler with 
-Wreturn-std-move, such as:


Note that if you construct the returned struct in the return 
statement with something like:


 return BigStruct {
   ...,
 };

it should still invoke the Result<> constructor with move 
semantics. But either way, that's going to result in a copy when 
the Result is constructed (unless the compiler is really clever).


The only way to avoid the copy altogether would be to create the 
result with a default initialized value, fill it in, and then 
return it without an std::move. Which is easier said than done. 
It might make sense to add a `.emplace()` method to make that 
easier...

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

Re: Fission Newsletter #2

[Kris Maglione]

On Fri, Aug 09, 2019 at 01:33:31PM -0400, Nika Layzell wrote:
Tests which do not currently successfully pass are marked as 
fail-if = Fission or skip-if = Fission.


To be slightly pedantic, they're marked as `fail-if = fission` or 
`skip-if = fission`. mozinfo expression matching should be case 
sensitive, so if you try to use `Fission` instead, it won't work.

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

Re: Non-header-only headers shared between SpiderMonkey and the rest of Gecko

[Kris Maglione]

On Tue, Aug 06, 2019 at 10:56:55AM +0300, Henri Sivonen wrote:
Do we have some #ifdef for excluding parts of mfbt/ when mfbt/ is being used 
in a non-SpiderMonkey/Gecko context?


#ifdef MOZ_HAS_MOZGLUE
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Native actor refcounting support in IPDL

[Kris Maglione]

\o/

This is going to get rid of so much (often buggy, potentially leaky, and 
slightly varying) boilerplate from most of our IPC code.


Thanks!

On Thu, Aug 01, 2019 at 03:11:00PM -0400, Nika Layzell wrote:

Bug 1550560 (https://bugzilla.mozilla.org/show_bug.cgi?id=1550560) landed
recently, adding native support for declaring actors as *refcounted*. This
change improves the ergonomics for declaring and using IPDL actors, and
opens avenues for future improvements to the IPC core. *refcounted* is the
recommended way to define the lifecycle for an actor, and may become the
default in the future. The syntax currently looks as-follows:

async *refcounted* protocol PBrowserBridge {
https://searchfox.org/mozilla-central/rev/b38e3beb658b80e1ed03e0fdf64d225bd4a40327/dom/ipc/PBrowserBridge.ipdl#33

Adding this attribute has the following impacts:

  1. Generated actor classes have pure-virtual *AddRef* and *Release*
  methods to be overridden by the implementation.
  2. *AllocPFoo* methods return *already_AddRefed* instead of *PFoo**
  .
  3. No *DeallocPFoo* methods will be generated or called on the manager
  class.
  4. IPDL will automatically obtain a single reference to the actor when
  its underlying connection is established, and will release it when the
  actor is no longer in use by IPC.
  5. The "helper constructor" is no longer generated, so *AllocPFoo*
  methods on the constructor sender side, which are often implemented as
  *MOZ_ASSERT_UNREACHABLE()*, may be removed. This may require consumers
  of the helper constructor to slightly modify their code.

I have converted 16 actors to use this new annotation in bug 1570369 (
https://bugzilla.mozilla.org/show_bug.cgi?id=1570369). Patches on this bug,
such as https://phabricator.services.mozilla.com/D40263, can be used as a
reference when converting your actor.

Please reach out to me (:nika) on IRC or Slack if you have questions or
encounter bugs when converting your actor.

Thanks!
Nika

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

PSA: Fission mochitests will soon run on integration branches

[Kris Maglione]

Within the next few days, mochitests will run with Fission enabled (in the 
M-fis job) on every check-in. The tests will run as Tier 2, so new failures 
will not cause immediate backout, but will still need to be fixed.


In order to get tests running as soon as possible, so that we can spot new 
regressions easily, a large number of existing tests have been skipped or 
marked expected-fails. We'll be working on decreasing this number over the 
coming weeks (and will likely be reaching out to owners of failing tests for 
assistance when possible). Any expected failures which are fixed will need to 
have the annotation removed, and continue to pass from that point on.


Please do not add new fail-if or skip-if annotations for Fission tests without 
first speaking to a member of the Fission team (preferably me or Andrew 
McCreight). Shipping Fission is critical to the success of the organization, 
so any changes that set it back may be backed out.


Thanks,
Kris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Resolving URLs in c++

[Kris Maglione]

On Thu, Jul 11, 2019 at 10:57:05PM -0700, mcace...@mozilla.com wrote:

Was looking at how WebKit implements the WebShare API, and they have this nice 
method `completeURL(str)` [1] that resolved URLs relative to, I guess, the 
`Document` object (or whatever context is). So they can basically do this c++:

...

Right now, in Gecko, I'm having to do this:


Most of the overhead in this has nothing to do with resolving 
the URIs, so much as error checking. That said, there are a few 
ways to make it simpler. If you really need a UTF-16 string, you 
can do something like:


 nsAutoCString utf8URI;
 MOZ_TRY(doc->GetDocumentURI()->Resolve(aData.mUrl.Value()));

 NS_ConvertUTF8toUTF16 uri(utf8URI);

Though the general preference should be to keep URIs stored as 
nsIURIs rather than strings wherever possible, which means just:


 nsCOMPtr uri;
 MOZ_TRY(NS_NewURI(getter_AddRefs(uri), aData.mUrl.Value(), 
   doc->GetDocumentURI()));


There really isn't a need to pass a reference to the IO service 
to NS_NewURI anymore. Even if you happened to have a handy copy, 
the argument isn't even used anymore.

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

Re: Running C++ early in shutdown without an observer

[Kris Maglione]

On Fri, Jun 07, 2019 at 11:40:05AM -0700, Chris Peterson wrote:
Note that on Android, you may never get an opportunity for a clean 
shutdown because the OS can kill your app at any time.


I don't know what is the recommendation for shutdown activities on 
Android. The GeckoView team has had some recent bugs caused by 
shutdown tasks not running (e.g. committing cached font files or 
deleting temp files). I think these tasks were moved to startup or 
scheduled to run periodically.


A lot of things really need to have flush timeouts with shutdown 
blockers for their finalizers. In JS land, we have helpers like 
DeferredTask and JSONFile for that. I don't think we really have 
anything comparable for C++.

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

Re: Running C++ early in shutdown without an observer

[Kris Maglione]

On Fri, Jun 07, 2019 at 09:18:38AM +0300, Henri Sivonen wrote:

For late shutdown cleanup, we have nsLayoutStatics::Shutdown(). Do we
have a similar method for running things as soon as we've decided that
the application is going to shut down?

(I know there are observer topics, but I'm trying to avoid having to
create an observer object and to make sure that _it_ gets cleaned up
properly.)


Observers are automatically cleaned up at XPCOM shutdown, so you 
generally don't need to worry too much about them. That said, 
nsIAsyncShutdown is really the way to go when possible. But it 
currently requires an unfortunate amount of boilerplate.

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

Re: Change to preferences via StaticPrefs, tremoval of gfxPrefs

[Kris Maglione]

On Wed, May 15, 2019 at 07:02:58PM -0400, Botond Ballo wrote:

On Wed, May 15, 2019 at 6:54 PM Jean-Yves Avenard  wrote:

It is then up to each process to handle preference modifications and
pass that change. There's no automatic or universal method on how this
is done unfortunately.


Will SpecialPowers.pushPrefEnv(), which currently does propagate the
prefs at least between the content and parent processes, continue to
work? A lot of tests rely on this.


Preference changes in the parent process are automatically 
propagated to any child processes that handle preference 
changes. Currently, I believe that does not include GPU and VR 
processes. It does include web content processes and the necko 
process.


But for processes that do propagate preference changes, it 
doesn't matter whether they use StaticPrefs or another 
preference access API.

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

Re: nsresult code lookup via IRC (mrgiggles bot)

[Kris Maglione]

If you're not in an IRC channel with mrgiggles, and happen to 
have a Firefox or xpcshell instance, you can also just evaluate  
`Components.Exception("", 0x80630001)`, which will generally 
work with anything other than NSS error codes (which are 
special, and have their own special lookup mechanism).


On Fri, Mar 29, 2019 at 04:41:53PM -0700, Steve Fink wrote:
If you're in an IRC channel with mrgiggles, the query "what is 
0x806e000b?" will return the corresponding nsresult code(s), together 
with any associated comments heuristically parsed from the 
ErrorList.py source. The comments are sometimes helpful:


 what is 0x804B0046?
 0x804b0046 as nsresult is NS_ERROR_DOCUMENT_NOT_CACHED 
from the NETWORK module, commented:
    Error passed through onStopRequest if the document could not be 
fetched from the cache.


and sometimes not so much:

 what is 0x80630001?

 0x80630001 as nsresult is NS_ERROR_STORAGE_BUSY from the 
"STORAGE" module, commented:


    To add additional errors to Storage, please append entries to the 
bottom of the list in the following format:

    NS_ERROR_STORAGE_YOUR_ERR, FAILURE(n)
    where n is the next unique positive integer. You must also add an 
entry to js/xpconnect/src/xpc.msg under the code block beginning with 
the comment 'storage related codes (from mozStorage.h)', in the 
following format: 'XPC_MSG_DEF(NS_ERROR_STORAGE_YOUR_ERR, "brief 
description of your error")'


If mrgiggles does not subscribe to your favored channel, or you're the 
quiet unassuming sort, `/msg mrgiggles what is 0x80630001?` anywhere 
on moznet will also work.


Reverse lookup also works:

 what is NS_RDF_ASSERTION_REJECTED?
 NS_RDF_ASSERTION_REJECTED is 0x4f0003 from the RDF module, 
commented as:
Returned from nsIRDFDataSource::Assert() and Unassert() if the 
assertion not willing to record the statement.


I'm not entirely sure why that last one is a success code rather than 
an error code, but thankfully I do not need to know. mrgiggles will 
update every couple of hours or so, except when I break it and don't 
notice.


Web-based nsresult lookup services include Mossop's 
https://www.oxymoronical.com/nsresult/ and the older 
https://james-ross.co.uk/mozilla/misc/nserror


Note that this output is only for numbers that happen to be nsresult 
codes; mrgiggles has other numeric lookup tricks too:


 what is 0xfffe6b887ea990a0?
 sfink: (as PUNBOX64) ObjectValue(0x6b887ea990a0)
 what is 0xe5e5e5f7?
 e5e5e5f7 is 18 bytes after the magic value e5e5e5e5
    0xe5 is jemalloc freed memory
    if you're seeing a crash with this pattern, you may have a 
use-after-free on your hands
    and you'd better fix it. They tend to be security bugs! (Or we 
might just have not initialized that memory after allocating it.)


I don't normally post about new mrgiggles tricks, but the nsresult 
stuff seems generally useful and undiscoverable.



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


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

It is the mark of an educated mind to be able to entertain a thought
without accepting it.
--Aristotle

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

Re: Searchfox now indexing m-beta, m-release, m-esr60

[Kris Maglione]

Huzzah!

On Tue, Mar 12, 2019 at 02:15:16PM -0400, Kartikaya Gupta wrote:

Due to popular demand, searchfox now also has
mozilla-{beta,release,esr60} repos. Text search only for now; blame
should appear within a couple of days or so. Follow along on bug
1282123 or just mash F5 periodically to find out when exactly.
Rust/C++ indexing for beta and release will happen sometime after bug
1533801 arrives on those branches via the code train. Choo-choo!

Cheers,
kats
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

For every complex problem there is an answer that is clear, simple,
and wrong.
--H. L. Mencken

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

Re: What is dom/browser-element/ used for?

[Kris Maglione]

On Thu, Feb 28, 2019 at 11:45:29AM +0200, Henri Sivonen wrote:

It appears dom/browser-element/ was created for Gaia. Is it used for
something still? WebExtensions perhaps?


We've talked about giving access to WebExtensions (bug 1318532; 
partly because Google provides a similar  element, and 
partly because it would solve a lot of other problems), but as 
of now they can't use it.

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

Re: PSA: New method for registering XPCOM components

[Kris Maglione]

On Wed, Feb 06, 2019 at 11:31:20AM +0100, Michael de Boer wrote:
That looks really neat and a nice step forward! I’m a fan of 
all the things you’re doing, btw ;-)


Cheers.

* Perhaps a link (or multiple links) to MDN docs we already 
have on XPCOM components - which may provide an introduction as 
to what they are, when and why they’re used, etc.


I'm not sure this is a good idea. Docs about Gecko internals on 
MDN are generally deprecated, and the ones about XPCOM are 
obsolete to the point of being useless.


I'm also a bit leery about mentioning old coding styles in docs 
like this, since those kinds of mentions tend to stick around 
long after everyone has forgotten that style ever really 
existed...


* An example of ‘old style’ vs. ’new style’ xpcom definition, 
 perhaps even a ‘good’ vs. ‘bad’ - but there’s a negative 
 connotation there, which may not be preferred.
* A full example (prolly on a second page would be best), which 
 showcases the discrete steps to get from zero to hero. Err, I 
 mean new component.


I’m suggesting all this extra work, because I think it will 
actually save you a lot in the future; rtfm is a very simple, 
yet powerful response to all the queries you’re gonna get.


Again, I'm not sure the linked doc is really the best place for 
such things, since the doc is meant to be permanent, and it 
would get dated fast.


I can give examples in this thread, if you think it would be 
useful. But given that I've already converted (or have pending 
patches to convert) pretty much all of our old-style 
registrations, it wasn't clear to me that it would.



On 5 Feb 2019, at 22:12, Kris Maglione  wrote:

As of bug 1478124, the new preferred method for registering XPCOM components is 
via static manifest files, as documented here:

https://firefox-source-docs.mozilla.org/build/buildsystem/defining-xpcom-components.html

And, as of bug 1524688, it will be the preferred method of defining JavaScript 
components as well as native ones.

The primary motivation for this change is to decrease the amount of memory 
(and, to a lesser extent, startup performance) overhead that component 
registrations consume in content processes, which would not have been 
acceptable in the post-Fission world. It has the side-benefit, though, of 
making most registrations much more straightforward, requiring only a single 
entry, in a single place, for each component.


Thank you to all of the reviewers who had to review a lot of very large patches 
to make this possible, particularly Nathan Froyd, Eric Rahm, and Mike Conley, 
on whom I dumped most of the biggest chunks.


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

All great truths begin as blasphemies.
--George Bernard Shaw

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

PSA: New method for registering XPCOM components

[Kris Maglione]

As of bug 1478124, the new preferred method for registering XPCOM components 
is via static manifest files, as documented here:


https://firefox-source-docs.mozilla.org/build/buildsystem/defining-xpcom-components.html

And, as of bug 1524688, it will be the preferred method of defining 
JavaScript components as well as native ones.


The primary motivation for this change is to decrease the amount of memory 
(and, to a lesser extent, startup performance) overhead that component 
registrations consume in content processes, which would not have been 
acceptable in the post-Fission world. It has the side-benefit, though, of 
making most registrations much more straightforward, requiring only a single 
entry, in a single place, for each component.



Thank you to all of the reviewers who had to review a lot of very large 
patches to make this possible, particularly Nathan Froyd, Eric Rahm, and 
Mike Conley, on whom I dumped most of the biggest chunks.

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

Re: Intent to ship: Devirtualizing IPC method calls (bug 1512990)

[Kris Maglione]

On Mon, Feb 04, 2019 at 04:29:54PM -0500, Alex Gaynor wrote:

b) Memory -- we no longer need to emit vtables for all of these methods
(~20 KB per process)


\o/


c) Ergonomics -- implementing classes will now have control over whether
they take arguments by value or reference


\o/

Thank you :)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Does mozilla allow modification of Strings

[Kris Maglione]

On Tue, Jan 29, 2019 at 02:08:59AM -0800, Nanday Dan wrote:

On Tuesday, January 29, 2019 at 12:22:08 AM UTC+5:30, Kris Maglione wrote:

>Is it possible to add an  extra variable to mozilla string(nsTStringRepr).
>
>
>I added a bool variable to nsTStringRepr class in  Xpcom/Strings/

As something of a side note, the general way to do this is to
add a new data or class flag, and store it in one of the
existing fields:



I think storing in existing fields will break the actual usage/functioning of 
those fields and it will make  more complex.  So I felt like adding a new flag 
will not affect normal functioning of it.


You have this entirely backward. The existing flags fields are 
meant to store arbitrary boolean flags that describe a string's 
contents. Existing code knows about them, and is prepared to 
deal with them. Adding a new field to the string representation, 
on the other hand, is a drastic change that existing code is 
*not* prepared to deal with.

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

Re: PSA: Major ChromeUtils.import() API change

[Kris Maglione]

On Mon, Jan 28, 2019 at 04:50:28PM -0800, Kris Maglione wrote:
Whatever else this whole process accomplishes, it will have the major 
side-effect of making our system JS code much more JIT-friendly, and 
in many cases, some orders of magnitude faster.


Since there's been some question about my "orders of magnitude" comment, I 
may as well explain:


To begin with, the weirdness of our JSM scopes makes a fair amount of our 
system code un-JITtable, which should be enough on its own. But even for 
code that we can JIT (which is still a lot of it), there's another problem: 
In order to make code fast, the JIT needs to be certain that it knows the 
location and type of every variable that it touches, and that those values 
can't change out-from-under it.


For code that deals with global variables and has weird scope chains, 
though, that's not really possible. There are scopes in-between the 
executing scope and the global scope that the JIT doesn't really understand, 
and can't really trust. So, in an ideal world, the following, for instance:


 for (let i = 0; i < typedArrayA.length; i++) {
   typedArrayC[i] = Math.floor(typedArrayA[i] / typedArrayB[i]);
 }

The Math.floor call can be inlined into pure assembly operations that take a 
few nanoseconds. The whole loop becomes pretty similar to the equivalent 
construct in C.


If we're in a JSM environment, though, the JIT can't trust the value of 
`Math` to stay the same from one iteration to the other. It has to walk the 
scope chain for every iteration, and do a hash lookup for the name `Math` in 
each scope, until it finds a match. Hash lookups are expensive. The same 
operation which took nanoseconds in an ideal world takes microseconds in our 
current JSM world. That's three orders of magnitude slower before we even 
take into account the memory we touch that's now taking up valuable L1 cache 
or register space, or the likelihood that we have to do a real JS function 
call rather than do some simple inline arithmetic.



This is probably more than most of you want to know, and less than the rest 
of you want to know, but it's been a long week for it being only a Monday 
night, so there you are.


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

PSA: Major ChromeUtils.import() API change

[Kris Maglione]

As of bug 1514594, the behavior ChromeUtils.import() API has changed in 
order to avoid touching the caller's global scope, or returning the imported 
module's global scope. In short, where you previously wrote:


 ChromeUtils.import("resource://gre/modules/Services.jsm");

You should now write something like:

 const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

We've made this change for a lot of reasons. The obvious ones include making 
it obvious to readers exactly what symbols are imported, and making it 
easier for tooling to analyze such imports.


The less obvious, and arguably more important reason, though, is that we 
would like to start loading JSMs as ES6 modules. The major stumbling block 
to that effort is that our JSM import and export APIs rely on modules being 
loaded into global-like objects that we can return, and our caller APIs 
likewise rely on callers being loaded into objects where we can define 
properties. Neither of these assumptions are compatible with ES6 module 
scripts, which have lexical scopes, but no global or pseudo-global objects 
of their own.


Changing the ChromeUtils.import() API to stop relying on these assumptions 
is the first step down this road. In the near future, we're going to need to 
make similar changes to our lazy getter and lazy import APIs to stop 
defining properties on the `this` object (which will be going away), and to 
a large number of unit tests which rely on mangling module globals.


Once all of this is done, we'll change our JSM loading infrastructure to use 
the `export` keyword rather than the `EXPORTED_SYMBOLS` array, and rewrite 
our existing code to comply with the new behavior. Whatever else this whole 
process accomplishes, it will have the major side-effect of making our 
system JS code much more JIT-friendly, and in many cases, some orders of 
magnitude faster.


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

Re: Does mozilla allow modification of Strings

[Kris Maglione]

On Mon, Jan 28, 2019 at 04:51:12AM -0800, edusubscr...@gmail.com wrote:

Is it possible to add an  extra variable to mozilla string(nsTStringRepr).


I added a bool variable to nsTStringRepr class in  Xpcom/Strings/


As something of a side note, the general way to do this is to 
add a new data or class flag, and store it in one of the 
existing fields:


https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/xpcom/string/nsTStringRepr.h#300-301

Changing the size or layout of the string class is more or less 
guaranteed to cause problems. Aside from affecting the size and 
padding of thousands of classes, the strings API is some of the 
oldest code in Gecko, and there is almost certainly some subtle 
code which makes assumptions about its layout, and will break if 
it changes. The Rust bindings are one such example (as you saw), 
but there are almost certainly others as well.

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

Re: Does mozilla allow modification of Strings

[Kris Maglione]

On Mon, Jan 28, 2019 at 02:05:18PM +0100, Emilio Cobos Álvarez wrote:

There are probably two different issues.

On 1/28/19 1:51 PM, edusubscr...@gmail.com wrote:



Is it possible to add an  extra variable to mozilla string(nsTStringRepr).


I added a bool variable to nsTStringRepr class in  Xpcom/Strings/

While building the build  I got static Assertions like  below

 /User/Desktop/MOZB/mozilla-central/xpcom/base/nsCycleCollector.cpp:1954:3: error: 
static_assert failed "Don't create too large CCGraphBuilder objects"
 0:34.99   static_assert(sizeof(CCGraphBuilder) <= 4096,


If you're just experimenting this assertion can probably just be ignored
/ commented out, seems it's just to prevent accidentally wasting a lot
of memory.


Indeed. If the size of that object grows one byte above 4096, 
each instance takes 8K of (mostly unused) memory rather than 4K. 
That probably doesn't matter for debugging, but for real world 
usage, it matters a lot.

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

Re: Proposal to adjust our clang-format rules to include spaces after the hash for nested preprocessor directives

[Kris Maglione]

On Thu, Jan 10, 2019 at 08:01:52PM -0500, Ehsan Akhgari wrote:

The common way to deal with this problem is to indent nested preprocessor
directives, very much similarly to how we indent normal code, for example:

#if foo
#  if bar
#define x 1
#  else
#define x 2
#  endif
#endif


+1

This has been my preferred style for a long time, but lately 
clang-format just throws it away whenever I try to do it.

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

Re: Pointer to the stack limit

[Kris Maglione]

On Wed, Dec 19, 2018 at 07:36:52AM -0500, David Major wrote:

You'll need platform-specific code, but on Windows there's 
https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/js/xpconnect/src/XPCJSContext.cpp#986.

And, to get a sense of caution, have a look at the ifdef madness surrounding 
the caller -- 
https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/js/xpconnect/src/XPCJSContext.cpp#1125
 -- to see the number of hoops we have to jump through to accommodate various 
build configs.


For glibc Linux, there's also:

https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/xpcom/threads/nsThread.cpp#506-557

Other Unix platforms require similarly super-specific code.


On Wed, Dec 19, 2018, at 7:12 AM, Henri Sivonen wrote:

Is it possible to dynamically at run-time obtain a pointer to call
stack limit? I mean the address that is the lowest address that the
run-time stack can grow into without the process getting terminated
with a stack overflow.

I'm particularly interested in a solution that'd work on 32-bit
Windows and on Dalvik. (On ART, desktop Linux, and 64-bit platforms we
can make the stack "large enough" anyway.)

Use case: Implementing a dynamic recursion limit.

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

Re: PSA: Landing Rust code that warns is no longer ok

[Kris Maglione]

On Sat, Dec 15, 2018 at 04:06:05AM +0100, Emilio Cobos Álvarez wrote:
In https://bugzilla.mozilla.org/show_bug.cgi?id=1513009 I just landed 
a patch so that warnings for non-third-party Rust code get reported as 
errors in automation, and thus fail to build (also if you build 
locally with --enable-warnings-as-errors).


\o/

Rust warning spew has been driving me crazy...
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Dropping support for compiling with MSVC in the near future

[Kris Maglione]

On Fri, Dec 07, 2018 at 10:57:41AM -0500, Gabriele Svelto wrote:

On 06/12/18 15:39, Kris Maglione wrote:

As it stands, we need to remain compatible with at least GCC and Clang,
because some of our static analysis code still depends on GCC plugins.


Some Linux distros will keep building Firefox with GCC so there's going
to be at least some external users of that particular combination. MSVC
is a different story since nobody else would be using it.


I'm not sure how long that's going to be viable, at least for branded 
builds. GCC's optimizers already generate significantly slower code than 
clang's, and the disadvantage is going to get much worse when we have 
cross-language inlining in our clang builds, which GCC will never be 
able to support.


If they want to continue shipping GCC builds with their own branding, I 
suppose that's fine, but I don't think that would be a good reason for 
us to continue investing effort into supporting it.

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

Re: Dropping support for compiling with MSVC in the near future

[Kris Maglione]

On Thu, Dec 06, 2018 at 03:34:59PM -0500, Jonathan Kew wrote:
While I sympathize with the concern that "supporting more than one 
compiler is a maintenance burden", this still leaves me feeling a 
little uneasy. Ensuring that our code builds successfully with 
multiple compilers is a useful way to keep us from becoming dependent 
on quirks of a particular tool, and different compilers may provide 
different (valid, useful) diagnostics that we should not simply 
ignore.


Our C/C++ code should (IMO) be standard and portable enough to build 
with any modern, mainstream C++ compiler; writing code for a clang 
monoculture feels wrong, a bit like building websites for a Blink 
monoculture...


As it stands, we need to remain compatible with at least GCC and 
Clang, because some of our static analysis code still depends on 
GCC plugins. But supporting MSVC is a much larger burden. It 
tends to be the slowest compiler to adopt major new features, 
and the fact that it's closed source means that we're completely 
dependent on Microsoft to fix bugs which are trivial for us to 
fix in our open source tool chains.


I'm all for not chaining ourselves so tightly to Clang, though.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Dropping support for compiling with MSVC in the near future

[Kris Maglione]

\o/

On Thu, Dec 06, 2018 at 03:00:12PM -0500, Ted Mielczarek wrote:

Hello,

In light of the fact that we've switched to clang-cl for our Windows builds[1], 
we are planning to drop support for compiling Firefox with MSVC in the near 
future[2]. Our estimate is that this will happen sometime in Q1. Supporting 
more than one compiler is a maintenance burden and we've already seen 
developers spend considerable time getting their patches that work with 
clang-cl to build with MSVC. We are currently blocked by the fact that our 
aarch64-windows builds are still using MSVC and we are waiting on upstream 
clang-cl work to switch those builds to clang-cl. Once that takes place we no 
longer have a compelling reason to continue supporting MSVC.

To preempt the question--when this happens we intend to make MSVC error in configure, and 
not just move MSVC to Tier 3 "patches welcome" status. Our reasoning is that 
Tier 3 configurations still create work: developers spend time building in those 
configurations, and lack of CI coverage means that when they inevitably break they waste 
time trying to fix things. Bugs get filed, and other developers waste time trying to help 
or reviewing patches to fix things. Explicitly unsupporting MSVC is the best way for us 
to convey the fact that developers should not be using it and we will not accept patches 
to fix issues that only affect MSVC.

If you have specific reasons for continuing to use MSVC please let us know. If 
there are deficiencies in clang-cl compared to MSVC we should get them filed 
and fixed.

Thanks,
-Ted

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

Re: Performance profiling improvements #3

[Kris Maglione]

On Thu, Nov 01, 2018 at 02:38:46PM +0900, Mike Hommey wrote:

On Thu, Nov 01, 2018 at 01:07:54AM -0400, Randell Jesup wrote:

>I think sudo will let you have symbolicated kernel stacks which can be handy.

$ sudo perf record ./firefox  has a problem:
"Running Nightly as root in a regular user's session is not supported."


Try sudo -H.


Regardless of whether it works, running Firefox as root is generally a Very 
Bad Idea. Dropping root privileges is definitely the better option.

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

Re: assignment between related nsCOMPtrs<> now works

[Kris Maglione]

On Tue, Oct 02, 2018 at 09:29:51AM -0700, Andrew McCreight wrote:

This is a step towards bug 1493226, which is going to statically ban
trivial do_QueryInterface calls like this:


\o/
___
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

[Kris Maglione]

On Mon, Sep 24, 2018 at 04:05:22PM -0400, Randell Jesup wrote:

On 9/20/18 5:59 PM, Andrew McCreight wrote:

On Wed, Sep 19, 2018 at 5:44 PM Kris Maglione  wrote:


On Wed, Sep 19, 2018 at 05:37:46PM -0700, Bobby Holley wrote:

So, I don't think we need to do anything fancy with forking - we'd just
need to capture stacks and send them via telemetry rather than as a crash
report. This was the idea behind bug 1209131, which got pretty far along
but never made it to the finish line.


This would actually potentially even give us better information
than fork-and-crash, since we should be able to include JS
stacks in that setup too. We've never been able to do that in
ordinary crash reports, since breakpad doesn't know how to
unwind JS stacks, and we can't safely ask the JS runtime to do
it while we're crashing.



Though keep in mind that any stack that includes content JS is going to
likely count as PII, so it would have to be hidden by default on Soccorro.



Please note that it would be illegal to collect such data
without asking for explicit user consent first.
The GDPR requires a "positive opt-in" mechanism:
https://ico.org.uk/for-organisations/guide-to-the-general-data-protection-regulation-gdpr/lawful-basis-for-processing/consent/
Our current Telemetry permission is an opt-out mechanism.


Right - this would need to be handled in a similar way to real crashes -
pop a crashreporter dialog to let the user submit it.  We just wouldn't
kill the browser (and probably disable future semi-assertions until
restart once we hit and report one to avoid bugging the user too much).


For telemetry assertion reporting, I think BHR is a closer 
analog. That already collects JS stacks.

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

Re: Browser Architecture Newsletter #7 (S02E02)

[Kris Maglione]

On Thu, Sep 20, 2018 at 09:02:09AM -0700, Nicholas Alexander wrote:

On Thu, Sep 20, 2018 at 7:25 AM smaug  wrote:

> I'm reminded of https://bugzilla.mozilla.org/show_bug.cgi?id=618912 but
> IIRC there were similar experiments back then on desktop, and basic html
> chrome was significantly faster than basic xul chrome.
That bug seems to be more about the layout.


https://screenshotscdn.firefoxusercontent.com/images/d1753829-3ebd-4c42-a757-14757051accf.png
is
the latest numbers I've seen. That isn't pgo, so may or many not be very
accurate, but the regression is
very significant.



I'm not expert in these areas, so I hope the experts chime in, but I think
there are lots of trade offs here.  I believe that you are correct: the XUL
prototype cache and similar mechanisms significantly impact browser startup
and related metrics.  But there is a general belief, which I do not have
references for, that the HTML widget set is either faster than or will be
faster than the XUL widget set.  Certainly folks believe that effort should
be put into optimizing core Web Platform technologies (rather than
Mozilla-specific extensions).


We can't afford a startup or window opening performance regression. If 
switching to HTML gives us other performance improvements, that's great, but 
it can't come at the cost of performance in other areas that we've worked so 
hard to get into reasonable shape.

___
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

[Kris Maglione]

On Wed, Sep 19, 2018 at 05:37:46PM -0700, Bobby Holley wrote:

So, I don't think we need to do anything fancy with forking - we'd just
need to capture stacks and send them via telemetry rather than as a crash
report. This was the idea behind bug 1209131, which got pretty far along
but never made it to the finish line.


This would actually potentially even give us better information 
than fork-and-crash, since we should be able to include JS 
stacks in that setup too. We've never been able to do that in 
ordinary crash reports, since breakpad doesn't know how to 
unwind JS stacks, and we can't safely ask the JS runtime to do 
it while we're crashing.



On Wed, Sep 19, 2018 at 5:05 PM Randell Jesup  wrote:


>On Thu, Sep 20, 2018 at 09:22:12AM +1000, Cameron McCormack wrote:
>> On Thu, Sep 20, 2018, at 1:52 AM, Ehsan Akhgari wrote:
>> > While it may be the case that we may need to be more stable for
>> > MOZ_RELEASE_ASSERTs, I very much doubt that every single MOZ_ASSERT
in our
>> > codebase is actually a guaranteed to never fail, so promoting them
all to
>> > be enabled in something like Nightly is extremely dangerous in terms
of the
>> > stability of Nightly for users who are trying to use the browser to
get
>> > their normal work done.
>>
>> If it's truly useful to know whether Nightly users are failing these
>> MOZ_ASSERT assertions, we could use Telemetry to report their failure
>> instead of crashing.>
>> (I wonder if we could collect all the same data, and use the same crash
reporting infrastructure, for non-crashing crash reports like this.)
>
>On Linux and Mac, we could make MOZ_ASSERT fork and crash in the child,
>and continue in the parent process.

Oooh, tricky.  (though threads aren't preserved in forks, but the
current thread's stack is, so if we care about all threads, we might
need to grab pointers to their stacks/etc and store pointers to the
stacks before fork()).  But even without that it would be good.

And yes, we don't have to have it crash the browser (though if it later
crashes we'd want to know about assertions we'd hit).  Telemetry isn't
quite appropriate for reporting it; it could however save a crash report
at ASSERT(), then (probably) disable MOZ_ASSERT reports and let the
browser continue (and it may well now really crash).  This disabling
would avoid the cascade effect, but still get us crashreports.  This
would be a bunch(?) of work in crashreporter - ted?  It might be easy
invoke a crashreport sampling, and continue, but I suspect it's not, and
might be quite hard.

--
Randell Jesup, Mozilla Corp
remove "news" for personal email
___
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


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

Common sense is the collection of prejudices acquired by age 18.
--Albert Einstein

___
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

[Kris Maglione]

On Wed, Sep 19, 2018 at 11:52:07AM -0400, Ehsan Akhgari wrote:

FWIW, I think your original proposal, having a way to opt into assertions
without a slow build locally, is extremely valuable.  I could see myself
using that option intentionally even with the pain points I described
above, but for dogfooding rather than while working on a patch and such.


I agree. A separate build flag to enable most assertions without 
doing a full debug build would be extremely useful. I would 
probably leave that enabled most of the time, even though I 
rarely do debug builds in normal development.

___
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

[Kris Maglione]

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 see what the effort to get this would be, and if we
could ship some of this to users.  Much like nightly-asan, this would
shake out bugs we'll never find from crash-stats reports, and which can
lead to critical sec bugs, and turn them into frequently-actionable
bugs.  If needed this can be enabled incrementally once the build/config
infra and macros are in place; there are several options for that.

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

Re: MinGW Target Re-Enabled on TaskCluster

[Kris Maglione]

Thank you. This will make my development flow much easier.

On Wed, Sep 12, 2018 at 12:09:36AM -0500, Tom Ritter wrote:

Previous Thread:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/r3mYWbb42pM

As of a few hours ago, there is a new Tier 2 MinGW build on TaskCluster.
It's in the 'Windows MinGW all' line, with the group WMC64 for 'Windows
MinGW Clang x64'.


The MinGW builds are part of the Tor Uplift project, where we work closely
with Tor to upstream their patches and in general move Tor Browser closer
and closer to 'Firefox with Addons and Pref Changes' - instead of a fork of
Firefox with a bunch of custom patches.

Tor is currently using ESR releases: the bump to ESR60 (which occurred last
week! It's a huge release for them with a lot of UX improvements:
https://blog.torproject.org/new-release-tor-browser-80 ) was a lot smoother
- build-wise - than other bumps because we had the MinGW build running in
TC. Without keeping the MinGW build working, every ESR they have to go
through a potentially colossal amount of work on a short timescale to get
the build working under MinGW again. By minimizing their effort in fixing
the build, rebasing patches and the like - we can free up their limited
resources to continue to research and experiment on privacy technology like
First Party Isolation and Fingerprinting Resistance.



Now, you might be wondering "I thought we had a MinGW build?" We did. But
we had to disable it. Shortly after 60 went to beta, we removed support for
--disable-stylo. Stylo requires libclang to build; and getting that working
with MinGW was very complicated (see
https://bugzilla.mozilla.org/show_bug.cgi?id=1390583) so MinGW fell behind
and had to be disabled.

However, thanks (again) to the efforts of all the reviewers, build peers,
and especially Jacek Caban - we've been able to re-enable a MinGW build.
We are now building with clang using the MinGW headers. (Previously it was
gcc.) I believe we're the first 'real' project that is building with
MinGW-clang, and Tor will be the first major project to ship it (but I
could be wrong there.)

In configure and moz.build files, CC_TYPE will be 'clang-cl' for our normal
Windows builds (which build on Windows) and will be 'clang' for the
MinGW-clang builds (which build on Linux).

There are still some outstanding issues: I hope to land a x86 build, we
need to remove some of the --disable-foo flags, and once ESR60 gets a NSS
uplift I intend the backport the jobs there also. We hope to get pdbs
generating so we can debug easier - major appreciation to David Major and
Bob Owen who both have debugged pretty ugly crashes without symbols.
Eventually, I'd like to enable a limited set of tests to catch browser
crashes.  Because there is no path forward for getting the MinGW gcc builds
re-enabled (nor anyone who wants to work on it...) I intend to remove the
(disabled) build jobs from the tree also. And finally I need to document
how to get a local build environment for it.


MinGW is Tier 2, and sometimes breaking it is unavoidable because the fix
needs to happen upstream in MinGW. Other times breaking it is avoidable,
and one just needs to special-case it. The Tor Uplift team and Tor
themselves greatly appreciate all of your efforts to keep the build green.

As always, if you have a MinGW question or a build error you can't quite
understand - feel free to reach out to me via irc or email.


--

It is practically impossible to teach good programming style to
students that have had prior exposure to Basic; as potential
programmers they are mentally mutilated beyond hope of regeneration.
--Edsger W. Dijkstra

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

Re: Plan for Sunsetting MozReview

[Kris Maglione]

On Tue, Sep 04, 2018 at 07:37:28PM +0200, Dão Gottwald wrote:

This may have been discussed before since it's kind of an obvious question:

Was there a conscious decision not to post phabricator review comments to
bugzilla? It's a somewhat significant change from how we've used bugzilla.
I can see a potential upside of separating review comments from other
planning and chatter. Then again, the line isn't always drawn easily. Plus,
it makes it harder to migrate away from phabricator should we want that at
some unknown point; that MozReview posted comments to bugzilla turns out to
be quite valuable now.

I'd prefer if review comments stayed in bugzilla with an option to hide
them.


Concur. Aside from future-proofing things, reading comments in phabricator 
is pretty painful, especially for bugs with multiple patches. With the old 
flow, I could look at all of them in one place. Now, I have to open a half 
dozen separate pages, and then scroll through the entire patch squinting for 
comments, since none of the comments at the top of the review page provide 
even the slightest bit of context.



2018-07-26 20:37 GMT+02:00 Mark Côté :


To follow up on some previous threads, here is the plan for deprecating,
archiving, and decommissioning MozReview.

The MozReview shutdown deadline is approaching. Although enhanced
commit-series support is still in progress (see update below), MozReview
users should start familiarizing themselves with Phabricator now. We have a
guide specifically for MozReview users to ease the transition (which will
be updated when the commit-series work is finalized): https://moz-conduit.
readthedocs.io/en/latest/mozreview-migration-guide.html

From August 6 to August 20, use of MozReview will be restricted to updates
to existing reviews. In other words, review cycles in progress will be
given until August 20 to be completed, but no new commit series will be
permitted.

On August 20, we will remove public access to MozReview and archive
patches. Every landed, in-progress, and abandoned patch will be downloaded
from MozReview and stored in an S3 bucket. The “stub attachments” in
Bugzilla that currently redirect to MozReview will be updated to link to
the appropriate S3 bucket. Review flags and bug comments will be preserved.

After archiving is complete and verified, the MozReview servers will be
decommissioned.

We will also be writing a simple redirection service to map specific
MozReview reviews to associated BMO comments, review requests to associated
bugs, and review-request diffs to the appropriate S3 buckets. This service
will be up shortly after MozReview is decommissioned.

We realize and apologize that this is a fairly short timeline; however,
the current location of the MozReview servers is in the process of being
shut down, and thus it is urgent that we decommission this service soon to
allow an orderly exit.

As for enhanced support for series of commits in Phabricator, the new
command-line interface to submit, update, and apply series (bug 1471678) is
currently in review. The first release will support Mercurial only, but
git-cinnabar support will follow shortly (the code is designed with it in
mind). Work on series support in Lando (bug 1457525) is progressing; we
will be posting screenshots of the new UI shortly. It should be ready in
2-3 weeks.

Please note that we eventually plan to decommission Splinter as well;
however, we know we need some time to work out the kinks in Phabricator.
Splinter will remain operational near-term, but we ask everybody to
familiarize themselves with Phabricator and file bugs when things don't
work. *Please do not wait for Splinter EOL to try Phabricator; we need your
feedback before then in order to act on it.*

Mark


___
firefox-dev mailing list
firefox-...@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev





___
firefox-dev mailing list
firefox-...@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev



--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

Memory is like an orgasm.  It's a lot better if you don't have to fake
it.
--Seymour Cray

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

Re: PSA: nsISimpleEnumerator now works as a JS iterator

[Kris Maglione]

Oh, and from C++, there's also now a range iterator wrapper with 
similar semantics:


 for (auto& docShell : SimpleEnumerator(docShellEnum)) {
   ...
 }

On Thu, Aug 23, 2018 at 03:19:55PM -0700, Kris Maglione wrote:
As of bug 1484496, any C++-implemented nsISimpleEnumertor instance can 
be used as a JS iterator. And, when used this way, the iterators now 
have intrinsic type information, and therefore do not require QIing 
their elements.


Which is to say, now you can simply do:

for (let window of Services.wm.getEnumerator("navigator:browser")) {
  ...;
}

for (let docShell of docShellEnum) {
  ...;
}

rather than:

let winEnum = Services.wm.getEnumerator("navigator:browser");
while (winEnum.hasMoreElements()) {
  let window = winEnum.getNext();
  ...
}

while (docShellEnum.hasMoreElements()) {
  let docShell = winEnum.getNext().QueryInterface(Ci.nsIDocShell);
  ...
}

If you happen to be using an nsIArray from directly from JavaScript, 
you unfortunately still need to specify the expected types, since 
nsIArray has no idea what types it can contain:


for (let thing of array.enumerate(Ci.nsIThing)) {
  ...
}

Aside from being easier to maintain, these forms should also be 
somewhat faster than the old protocol, since they only require one 
XPConnect call per iteration rather than 3(+).


-Kris


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

There is no greater mistake than the hasty conclusion that opinions
are worthless because they are badly argued.
--Thomas Huxley

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

Re: Proposal: Require dangling commas for multiline objects/arrays on all Javascript mozilla-central code

[Kris Maglione]

On Wed, Aug 22, 2018 at 09:29:26PM +0100, Mark Banner wrote:
I would like to propose that we require dangling commas for multi-line 
object/arrays in Javascript code for mozilla-central.


+1

I've lost count of the number of times I've hit an obscure 
failure because I added a new element to the end of an array or 
object without realizing the last element didn't have a comma. 
Most of the areas I'm responsible for already require this rule, 
and it's saved me a lot of headaches.


--
Kris Maglione

The use of COBOL cripples the mind; its teaching should therefore be
regarded as a criminal offense.
--Edsger W. Dijkstra

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

Re: mozilla::Hash{Map,Set}

[Kris Maglione]

On Thu, Aug 16, 2018 at 09:07:57AM -0400, Alex Gaynor wrote:

Similarly, would it make sense to consolidate the APIs, as much as
possible, if the primary differences are around implementation details like
"how much is templated/inlined"?


It would probably worth converging the APIs, if only because the 
nsTHashtable APIs are arcane and antiquated. But it's easier 
said than done. There are a lot of consumers of all of the 
hashtable APIs, and the code is... not very easy to work with.



On Wed, Aug 15, 2018 at 5:39 AM Nicholas Nethercote 
wrote:


Hi,

I recently moved Spidermonkey's js::Hash{Map,Set} classes from
js/public/HashTable.h into mfbt/HashTable.h and renamed them as
mozilla::Hash{Map,Set}. They can now be used throughout Gecko.

(js/public/HashTable.h still exists in order to provide renamings of
mozilla::Hash{Map,Set} as js::Hash{Map,Set}.)

Why might you use mozilla::Hash{Map,Set} instead of PLDHashTable (or
nsTHashtable and other descendants)?

- Better API: these types provide proper HashMap and HashSet instances, and
(in my opinion) are easier to use.

- Speed: the microbenchmark in xpcom/rust/gtest/bench-collections/Bench.cpp
shows that mozilla::HashSet is 2x or more faster than PLDHashTable. E.g.
compare "MozHash" against "PLDHash" in this graph:


https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800=mozilla-central,1730159,1,6=mozilla-central,1730162,1,6=mozilla-central,1730164,1,6=mozilla-central,1732092,1,6=mozilla-central,1730163,1,6=mozilla-central,1730160,1,6

Bug 1477627 converted a hot hash table from PLDHashTable to
mozilla::HashSet and appears to have sped up cycle collection in some cases
by 7%. If you know of another PLDHashTable that is hot, it might be worth
converting it to mozilla::HashTable.

Both mozilla::Hash{Map,Set} and PLDHashTable use the same double-hashing
algorithm; the speed difference is due to mozilla::HashSet's extensive use
of templating and inlining. The downside of this is that mozilla::HashSet
will increase binary size more than PLDHashTable.

There are overview comments at the top of mfbt/HashTable.h, and the classes
themselves have more detailed comments about every method.

Nick
___
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


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

He who joyfully marches to music in rank and file has already earned
my contempt.  He has been given a large brain by mistake, since for
him the spinal cord would suffice.
--Albert Einstein

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

Re: mozilla::Hash{Map,Set}

[Kris Maglione]

On Wed, Aug 15, 2018 at 07:28:26PM -0400, Ehsan Akhgari wrote:

On Wed, Aug 15, 2018 at 5:39 AM Nicholas Nethercote 
wrote:

Bug 1477627 converted a hot hash table from PLDHashTable to
mozilla::HashSet and appears to have sped up cycle collection in some cases
by 7%. If you know of another PLDHashTable that is hot, it might be worth
converting it to mozilla::HashTable.



Do you have any good suggestions of how to find such candidates?  One thing
that came to my mind was that the BHR data may be a useful source of
insight for this...  <
https://arewesmoothyet.com/?category=all=512_2048=explore=c8e925752fc94f78af9349665aad14c7=PLDHashTable%3A%3ASearchTable=0>
(for content processes, for example) suggests a few hashtables based on a
very quick skimming which aren't surprising to me (things like
nsHTMLDocument::mIdentifierMap, some XPCOM component manager hashtables,
memory reporter hashtables, some font hashtables, the preferences
hashtable, sEventListenerManagersHash, mJSHolderMap, etc.


Note I'm planning to replace most of the component manager hashtables with 
static hashtables (bug 1478124), so those probably aren't great candidates 
just yet.

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

Re: mozilla::Hash{Map,Set}

[Kris Maglione]

On Wed, Aug 15, 2018 at 01:18:23PM -0700, Jeff Gilbert wrote:

What are the pros/cons to mozilla::Hash{Map,Set} vs
std::unordered_{map,set}? I'm assuming perf is the main draw? Do we
have a data on this too?


According to our benchmarks, mozilla::HashMap is much faster 
than std::unordered_map. But that's not the reason you should 
use it instead.


The main reason you shouldn't use std::unordered_map is that 
it's not approved for use in mozilla-central (though it is on 
the whitelist, and marked as unreviewed). Beyond that, we have 
no control over the memory usage or performance characteristics 
of unordered_map or unordered_set, and no way to add memory 
reporters for them. Their performance and memory usage are likely 
to vary from system to system, and from STL version to STL 
version, without giving us any indication, unless we're lucky 
and spot a talos or AWSY regression that we don't have any easy 
way to track down.



On Mon, Aug 13, 2018 at 10:44 PM, Nicholas Nethercote
 wrote:

Hi,

I recently moved Spidermonkey's js::Hash{Map,Set} classes from
js/public/HashTable.h into mfbt/HashTable.h and renamed them as
mozilla::Hash{Map,Set}. They can now be used throughout Gecko.

(js/public/HashTable.h still exists in order to provide renamings of
mozilla::Hash{Map,Set} as js::Hash{Map,Set}.)

Why might you use mozilla::Hash{Map,Set} instead of PLDHashTable (or
nsTHashtable and other descendants)?

- Better API: these types provide proper HashMap and HashSet instances, and
(in my opinion) are easier to use.

- Speed: the microbenchmark in xpcom/rust/gtest/bench-collections/Bench.cpp
shows that mozilla::HashSet is 2x or more faster than PLDHashTable. E.g.
compare "MozHash" against "PLDHash" in this graph:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800=mozilla-central,1730159,1,6=mozilla-central,1730162,1,6=mozilla-central,1730164,1,6=mozilla-central,1732092,1,6=mozilla-central,1730163,1,6=mozilla-central,1730160,1,6

Bug 1477627 converted a hot hash table from PLDHashTable to
mozilla::HashSet and appears to have sped up cycle collection in some cases
by 7%. If you know of another PLDHashTable that is hot, it might be worth
converting it to mozilla::HashTable.

Both mozilla::Hash{Map,Set} and PLDHashTable use the same double-hashing
algorithm; the speed difference is due to mozilla::HashSet's extensive use
of templating and inlining. The downside of this is that mozilla::HashSet
will increase binary size more than PLDHashTable.

There are overview comments at the top of mfbt/HashTable.h, and the classes
themselves have more detailed comments about every method.

Nick
___
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


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

First, solve the problem.  Then, write the code.
--John Johnson

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

Re: PSA: xptcall on Tier-3 platforms

[Kris Maglione]

This is fantastic news.

XPIDL has been stagnant for far too long, given how core a part 
of our platform it is. As anyone who's had to work with it can 
attest, it's basically a relic of the coding styles of the late 
'90s, and doesn't translate well to the practices of our current 
codebase. And the frozen state of xptcall has long been part of 
the problem.


I'm happy to see these things being modernized, with particular 
thanks to Nika Layzell who's done the lion's share of that work 
lately.


-Kris

On Tue, Jul 31, 2018 at 09:14:01AM -0700, Bobby Holley wrote:

XPConnect requires some platform-specific code to do its magic [1]. There
are around ~28 different copies of this code in xpcom/reflect/xptcall,
which makes it very difficult to maintain.

Historically, we've handled this by treating xptcall as fixed and working
around its deficiencies. Given the need to evolve our codebase, I don't
believe this is the right trade-off for us today.

Here's the plan going forward:
* Developers may make breaking changes to xptcall, provided they keep the
Tier-1 platforms on Treeherder green.
* Such changes should be marked as blocking bug 1479807 (xptcall-changes),
so that maintainers of Tier-3 platforms can easily follow along.
* Any xptcall implementation that didn't build and run at the time of the
most recent ESR release may be removed [2].

Please get in touch if you have any questions.

Cheers,
bholley

[1] Specifically, to dynamically invoke C++ virtual methods from JS, and to
allow JS objects to impersonate arbitrary XPCOM C++ objects.
[2] The source is, of course still available via version history if
somebody decides to support the platform again.

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

Re: Stopgap for Commit Series in Phabricator

[Kris Maglione]

Here's an approximate equivalent for hg which doesn't require 
Arcanist:


https://bitbucket.org/kmaglione/hgext/src/default/phabricator.py

It's a slightly modified version of stock hg Phabricator plugin 
(which we apparently have gps to thank for inspiring) which 
handles parsing bug IDs and reviewers from commit messages.


You just need to add something like this to your .hgrc:

[phabricator]
url = https://phabricator.services.mozilla.com/
callsign = MOZILLACENTRAL

[auth]
mozilla.schemes = https
mozilla.prefix = phabricator.services.mozilla.com
mozilla.phabtoken = cli-...

and then use `hg phabsend` to push a commit series (or `hg phabread` 
to import one).


On Wed, Jul 25, 2018 at 04:31:51PM -0400, Nika Layzell wrote:

While our services team is working on making a reliable & maintained tool
for handling commit series with Phabricator, I threw together something
small to use as a stop-gap for pushing large commit series to Phabricator
and updating them.

It currently works as a wrapper around Arcanist, and *only supports git*
(as I don't know how hg works enough to get it to work reliably), but
should allow pushing a range of commits as revisions without touching the
working tree, automatic dependency relationships, bug number filling, and
reviewer field population.

I called it 'phlay' (splinter => flay; flay + phabricator => phlay).

GitHub: https://github.com/mystor/phlay
Tweet w/ short demo clip:
https://twitter.com/kneecaw/status/1021434807325163523

I've used it to push pretty-much all of my recent patch series to
Phabricator, and it's saved me a good amount of time, so I figured I'd let
people know. Feel free to poke me on IRC if you have questions.

- nika


--
Kris Maglione

[T]he people can always be brought to the bidding of the leaders.
That is easy.  All you have to do is tell them they are being attacked
and denounce the pacifists for lack of patriotism and exposing the
country to danger.  It works the same way in any country.
--Herman Göring

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

Re: PSA: Major preference service architecture changes inbound

[Kris Maglione]

On Fri, Jul 20, 2018 at 11:53:22AM -0400, Ehsan Akhgari wrote:

On Fri, Jul 20, 2018 at 5:00 AM, Jonathan Kew  wrote:

+1 to that. Our need for OMT access to prefs is only likely to grow, IMO,
and we should just fix it once, so that any code (regardless of which
thread(s) it may eventually run on) can trivially read prefs.

Even if that means we can't adopt Robin Hood hashing, I think the
trade-off would be well worthwhile.



While it's true that the need for reading more prefs from workers will
continue to grow, given the number of prefs we have I think it's safe to
say that the set of prefs that we need to access from threads beside the
main thread will be a minority of the entire set of prefs.  So one way to
have our cake and eat it too would be to separate out the prefs that are
meant to be accessible through a worker thread and expose them through an
alternate thread-safe API which may be a bit more costly to call on the
main thread, and keep the rest of the min-thread only prefs on the existing
non-thread-safe APIs.  This won't be as elegant as having one set of APIs
to work with, of course.


This is what we have atomic var caches are for. They can't currently be used 
for string preferences, but that's a problem that could be solved with an 
rwlock. They're also a bit difficult to use for preferences which aren't known 
at compile time, but we've generally been trying to move away from using the 
preference service for such things.


For the sorts of preferences that are generally needed by Worker threads, 
though, they should mostly just work as-is.

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

Re: PSA: Major preference service architecture changes inbound

[Kris Maglione]

On Fri, Jul 20, 2018 at 10:00:22AM +0100, Jonathan Kew wrote:
+1 to that. Our need for OMT access to prefs is only likely to grow, 
IMO, and we should just fix it once, so that any code (regardless of 
which thread(s) it may eventually run on) can trivially read prefs.


Even if that means we can't adopt Robin Hood hashing, I think the 
trade-off would be well worthwhile.


This is exactly the kind of performance footgun I'm talking about. The marginal 
cost of making something threadsafe may be low, but those costs pile up. The 
cost of locking in hot code often adds up enough on its own. Throwing away an 
important optimization on top of that adds up even faster. Getting into the 
habit, and then continuing the pattern elsewhere starts adding up exponentially.


Threads are great. Threadsafe code is useful. But data that's owned by a single 
thread is still the best when you can manage it, and it should be the default 
option whenever we can reasonably manage it. We already pay the price of being 
overzealous about thread safety in other areas (for instance, the fact that all 
of our strings require atomic refcounting, even though DOM strings are generally 
only used by a single thread). I think the trend needs to be in the opposite 
direction.


Rust and JS make this much easier than C++ does, unfortunately. But we're 
getting better at it in C++, and moving more and more code to Rust.



On Thu, Jul 19, 2018 at 2:19 PM, Kris Maglione  wrote:

On Tue, Jul 17, 2018 at 03:49:41PM -0700, Jeff Gilbert wrote:


We should totally be able to afford the very low cost of a
rarely-contended lock. What's going on that causes uncached pref reads
to show up so hot in profiles? Do we have a list of problematic pref
keys?



So, at the moment, we read about 10,000 preferences at startup in debug
builds. That number is probably slightly lower in non-debug builds, bug we
don't collect stats there. We're working on reducing that number (which is
why we collect statistics in the first place), but for now, it's still quite
high.


As for the cost of locks... On my machine, in a tight loop, the cost of a
entering and exiting MutexAutoLock is about 37ns. This is pretty close to
ideal circumstances, on a single core of a very fast CPU, with very fast
RAM, everything cached, and no contention. If we could extrapolate that to
normal usage, it would be about a third of a ms of additional overhead for
startup. I've fought hard enough for 1ms startup time improvements, but
*shrug*, if it were that simple, it might be acceptable.

But I have no reason to think the lock would be rarely contended. We read
preferences *a lot*, and if we allowed access from background threads, I
have no doubt that we would start reading them a lot from background threads
in addition to reading them a lot from the main thread.

And that would mean, in addition to lock contention, cache contention and
potentially even NUMA issues. Those last two apply to atomic var caches too,
but at least they generally apply only to the specific var caches being
accessed off-thread, rather than pref look-ups in general.


Maybe we could get away with it at first, as long as off-thread usage
remains low. But long term, I think it would be a performance foot-gun. And,
paradoxically, the less foot-gunny it is, the less useful it probably is,
too. If we're only using it off-thread in a few places, and don't have to
worry about contention, why are we bothering with locking and off-thread
access in the first place?

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

PSA: Default thread stack size decreased to 256K

[Kris Maglione]

tl;dr: Bug 1476828 significantly decreased the default thread stack size. If 
you notice any thread abort issues, please file bugs blocking that bug.


For some time, our default stack size for thread pools has been 256K on most 
platforms, but the stack size for other threads has remained set to the 
platform default. On Windows and Android, that's about 1MB. On desktop Linux, 
it defaults to 2MB, unless overridden by a ulimit.


One wouldn't generally expect this to cause problems, since thread stacks are 
generally lazily committed as they grow. On Linux, however, the 2MB default 
causes a specific problem: it matches the size of VM huge pages, which causes 
the kernel to sometimes allocate an entire 2MB region for them, in a single 
huge page, the first time they're touched.


Decreasing the number to anything lower than 2MB solves this problem, but 256K 
is much closer to what we actually expect them to reasonably use, and matches 
the defaults we generally use elsewhere, so that's the number we chose. It's 
possible that certain specific threads may need more, however, so if you 
notice any thread crashes, please report them.


Thanks.

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

Re: PSA: Major preference service architecture changes inbound

[Kris Maglione]

On Thu, Jul 19, 2018 at 02:37:07PM -0700, Justin Dolske wrote:

I know we've had code that, instead of reading a pref directly, checks the
pref once in an init() and uses pref observers to watch for any changes to
it. (i.e., basically mirrors the pref into some module-local variable, at
which point you can roll your own locking or whatever to make it
threadsafe). Is that a pattern that would work here, if people really want
OMT access but we're not ready to bake support for that into the pref
service? [Perhaps with some simple helper glue / boilerplate to make it
easier.]


We already have helper glue for this. For C++, we have VarCache prefs, and for 
JS, we have XPCOMUtils.defineLazyPreferenceGetter. In general, it's probably 
better to use those rather than hand-rolled observers when possible, since I 
have optimizations planned for both.



On Thu, Jul 19, 2018 at 2:19 PM, Kris Maglione 
wrote:


On Tue, Jul 17, 2018 at 03:49:41PM -0700, Jeff Gilbert wrote:


We should totally be able to afford the very low cost of a
rarely-contended lock. What's going on that causes uncached pref reads
to show up so hot in profiles? Do we have a list of problematic pref
keys?



So, at the moment, we read about 10,000 preferences at startup in debug
builds. That number is probably slightly lower in non-debug builds, bug we
don't collect stats there. We're working on reducing that number (which is
why we collect statistics in the first place), but for now, it's still
quite high.


As for the cost of locks... On my machine, in a tight loop, the cost of a
entering and exiting MutexAutoLock is about 37ns. This is pretty close to
ideal circumstances, on a single core of a very fast CPU, with very fast
RAM, everything cached, and no contention. If we could extrapolate that to
normal usage, it would be about a third of a ms of additional overhead for
startup. I've fought hard enough for 1ms startup time improvements, but
*shrug*, if it were that simple, it might be acceptable.

But I have no reason to think the lock would be rarely contended. We read
preferences *a lot*, and if we allowed access from background threads, I
have no doubt that we would start reading them a lot from background
threads in addition to reading them a lot from the main thread.

And that would mean, in addition to lock contention, cache contention and
potentially even NUMA issues. Those last two apply to atomic var caches
too, but at least they generally apply only to the specific var caches
being accessed off-thread, rather than pref look-ups in general.


Maybe we could get away with it at first, as long as off-thread usage
remains low. But long term, I think it would be a performance foot-gun.
And, paradoxically, the less foot-gunny it is, the less useful it probably
is, too. If we're only using it off-thread in a few places, and don't have
to worry about contention, why are we bothering with locking and off-thread
access in the first place?


On Tue, Jul 17, 2018 at 8:57 AM, Kris Maglione 

wrote:


On Tue, Jul 17, 2018 at 02:06:48PM +0100, Jonathan Kew wrote:



On 13/07/2018 21:37, Kris Maglione wrote:



tl;dr: A major change to the architecture preference service has just
landed, so please be on the lookout for regressions.

We've been working for the last few weeks on rearchitecting the
preference service to work better in our current and future
multi-process
configurations, and those changes have just landed in bug 1471025.




Looks like a great step forward!

While we're thinking about the prefs service, is there any possibility
we
could enable off-main-thread access to preferences?




I think the chances of that are pretty close to 0, but I'll defer to
Nick.

We definitely can't afford the locking overhead—preference look-ups
already
show up in profiles without it. And even the current limited exception
that
we grant Stylo while it has the main thread blocked causes problems (bug
1474789), since it makes it impossible to update statistics for those
reads,
or switch to Robin Hood hashing (which would make our hash tables much
smaller and more efficient, but requires read operations to be able to
move
entries).

I am aware that in simple cases, this can be achieved via the

StaticPrefsList; by defining a VARCACHE_PREF there, I can read its value
from other threads. But this doesn't help in my use case, where I need
another thread to be able to query an extensible set of pref names that
are
not fully known at compile time.

Currently, it looks like to do this, I'll have to iterate over the
relevant prefs branch(es) ahead of time (on the main thread) and copy
all
the entries to some other place that is then available to my worker
threads.
For my use case, at least, the other threads only need read access;
modifying prefs could still be limited to the main thread.




That's probably your best option, yeah. Although I will say that those
kinds
of extensible preference sets aren't great for performance or memory
usage,
so switching to some

Re: PSA: Major preference service architecture changes inbound

[Kris Maglione]

On Tue, Jul 17, 2018 at 03:49:41PM -0700, Jeff Gilbert wrote:

We should totally be able to afford the very low cost of a
rarely-contended lock. What's going on that causes uncached pref reads
to show up so hot in profiles? Do we have a list of problematic pref
keys?


So, at the moment, we read about 10,000 preferences at startup in debug 
builds. That number is probably slightly lower in non-debug builds, bug we 
don't collect stats there. We're working on reducing that number (which is why 
we collect statistics in the first place), but for now, it's still quite high.



As for the cost of locks... On my machine, in a tight loop, the cost of a 
entering and exiting MutexAutoLock is about 37ns. This is pretty close to 
ideal circumstances, on a single core of a very fast CPU, with very fast RAM, 
everything cached, and no contention. If we could extrapolate that to normal 
usage, it would be about a third of a ms of additional overhead for startup. 
I've fought hard enough for 1ms startup time improvements, but *shrug*, if it 
were that simple, it might be acceptable.


But I have no reason to think the lock would be rarely contended. We read 
preferences *a lot*, and if we allowed access from background threads, I have 
no doubt that we would start reading them a lot from background threads in 
addition to reading them a lot from the main thread.


And that would mean, in addition to lock contention, cache contention and 
potentially even NUMA issues. Those last two apply to atomic var caches too, 
but at least they generally apply only to the specific var caches being 
accessed off-thread, rather than pref look-ups in general.



Maybe we could get away with it at first, as long as off-thread usage remains 
low. But long term, I think it would be a performance foot-gun. And, 
paradoxically, the less foot-gunny it is, the less useful it probably is, too. 
If we're only using it off-thread in a few places, and don't have to worry 
about contention, why are we bothering with locking and off-thread access in 
the first place?



On Tue, Jul 17, 2018 at 8:57 AM, Kris Maglione  wrote:

On Tue, Jul 17, 2018 at 02:06:48PM +0100, Jonathan Kew wrote:


On 13/07/2018 21:37, Kris Maglione wrote:


tl;dr: A major change to the architecture preference service has just
landed, so please be on the lookout for regressions.

We've been working for the last few weeks on rearchitecting the
preference service to work better in our current and future multi-process
configurations, and those changes have just landed in bug 1471025.



Looks like a great step forward!

While we're thinking about the prefs service, is there any possibility we
could enable off-main-thread access to preferences?



I think the chances of that are pretty close to 0, but I'll defer to Nick.

We definitely can't afford the locking overhead—preference look-ups already
show up in profiles without it. And even the current limited exception that
we grant Stylo while it has the main thread blocked causes problems (bug
1474789), since it makes it impossible to update statistics for those reads,
or switch to Robin Hood hashing (which would make our hash tables much
smaller and more efficient, but requires read operations to be able to move
entries).


I am aware that in simple cases, this can be achieved via the
StaticPrefsList; by defining a VARCACHE_PREF there, I can read its value
from other threads. But this doesn't help in my use case, where I need
another thread to be able to query an extensible set of pref names that are
not fully known at compile time.

Currently, it looks like to do this, I'll have to iterate over the
relevant prefs branch(es) ahead of time (on the main thread) and copy all
the entries to some other place that is then available to my worker threads.
For my use case, at least, the other threads only need read access;
modifying prefs could still be limited to the main thread.



That's probably your best option, yeah. Although I will say that those kinds
of extensible preference sets aren't great for performance or memory usage,
so switching to some other model might be better.


Possible? Or would the overhead of locking be too crippling?



The latter, I'm afraid.

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


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

On two occasions I have been asked, "Pray, Mr. Babbage, if you put
into the machine wrong figures, will the right answers come out?" I am
not able rightly to apprehend the kind of confusion of ideas that
could provoke such a question.
--Charles Babbage

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

Re: PSA: Major preference service architecture changes inbound

[Kris Maglione]

On Tue, Jul 17, 2018 at 02:06:48PM +0100, Jonathan Kew wrote:

On 13/07/2018 21:37, Kris Maglione wrote:
tl;dr: A major change to the architecture preference service has 
just landed, so please be on the lookout for regressions.


We've been working for the last few weeks on rearchitecting the 
preference service to work better in our current and future 
multi-process configurations, and those changes have just landed in 
bug 1471025.


Looks like a great step forward!

While we're thinking about the prefs service, is there any possibility 
we could enable off-main-thread access to preferences?


I think the chances of that are pretty close to 0, but I'll 
defer to Nick.


We definitely can't afford the locking overhead—preference 
look-ups already show up in profiles without it. And even the 
current limited exception that we grant Stylo while it has the 
main thread blocked causes problems (bug 1474789), since it 
makes it impossible to update statistics for those reads, or 
switch to Robin Hood hashing (which would make our hash tables 
much smaller and more efficient, but requires read operations to 
be able to move entries).


I am aware that in simple cases, this can be achieved via the 
StaticPrefsList; by defining a VARCACHE_PREF there, I can read its 
value from other threads. But this doesn't help in my use case, where 
I need another thread to be able to query an extensible set of pref 
names that are not fully known at compile time.


Currently, it looks like to do this, I'll have to iterate over the 
relevant prefs branch(es) ahead of time (on the main thread) and copy 
all the entries to some other place that is then available to my 
worker threads. For my use case, at least, the other threads only need 
read access; modifying prefs could still be limited to the main 
thread.


That's probably your best option, yeah. Although I will say that 
those kinds of extensible preference sets aren't great for 
performance or memory usage, so switching to some other model 
might be better.



Possible? Or would the overhead of locking be too crippling?


The latter, I'm afraid.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: XUL Overlays Removed

[Kris Maglione]

On Fri, Jul 13, 2018 at 02:10:54PM -0700, Brendan Dahl wrote:

This is hopefully the last thing you’ll ever hear about XUL overlays as
they have now been completely removed from Firefox[1].


\o/ Thank you to everyone who was involved in this!


Removing overlays cut around 3.5K lines of code from Firefox and in my
opinion made understanding which resources get loaded into which documents
easier to reason about


I completely agree. I can't count the number of times I've made 
a change in one place that seemed obviously sound, only to have 
it break on try because some completely unrelated piece of code 
loads a special overlay into the window only on OS-X.

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

PSA: Major preference service architecture changes inbound

[Kris Maglione]

tl;dr: A major change to the architecture preference service has just landed, 
so please be on the lookout for regressions.


We've been working for the last few weeks on rearchitecting the preference 
service to work better in our current and future multi-process configurations, 
and those changes have just landed in bug 1471025.


Our preference database tends to be very large, even without any user values. 
It also needs to be available in every process. Until now, that's meant 
complete separate copies of the hash table, name strings, and value strings in 
each process, along with separate initialization in each content process, and 
a lot of IPC overhead to keep the databases in sync.


After bug 1471025, the database is split into two sections: a snapshot of the 
initial state of the database, which is stored in a read-only shared memory 
region and shared by all processes, and a dynamic hash table of changes on top 
of that snapshot, of which each process has its own. This approach 
significantly decreases memory, IPC, and content process initialization 
overhead. It also decreases the complexity of certain cross-process 
synchronization logic.


But it adds complexity in other areas, and represents one of the largest 
changes to the workings of the preference service since its creation. So 
please be on the lookout for regressions that look related to preference 
handling. If you spot any, please file bugs blocking https://bugzil.la/1471025.


Thanks,
Kris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Fri, Jul 13, 2018 at 11:14:24AM -0400, Randell Jesup wrote:

Hash tables are a big issue.  There are a lot of 64K/128K/256K
allocations at the moment for hashtables.  When we started looking at
this in bug 1436250, we had a 256K, ~4 128K, and a whole bunch of 64K
hashtable allocs (on linux).  Some may be smaller or gone now, but it's
still big.

I wonder if it's worth the perf hit to realloc to exact size hash tables
that are build-once - probably.  hashtable->Finalize()?  (I wonder if
that would let us make any other memory/speed optimizations if we know
the table is now static.)


I think, as much as possible, we really want static or mostly-static 
hash tables to be shared between processes. I've already been working on this 
in a few areas, e.g., bug 1470365 for string bundles, which are completely 
static, and bug 1471025 for preferences, which are mostly static.


And those patches add helpers which should make it pretty easy to do the same 
for more things in the future, so that should probably be our go-to strategy 
for reducing per-process overhead, when possible.

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

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Thu, Jul 12, 2018 at 10:27:13PM +0200, Gabriele Svelto wrote:

On 12/07/2018 22:19, Kris Maglione wrote:

I've actually been thinking on filing a bug to do something similar, to
measure cumulative effects of excess padding in certain types since I
began looking into bug 1460674, and Sylvestre mentioned that
clang-analyzer can generate reports on excess padding.


I've encountered at least one structure where a boolean flag is 64-bits
in size on 64-bit builds. If we really want to go to the last mile we
might want to also evaluate things like tagged pointers; there's
probably some KiB's to be saved there too.


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.

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

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Thu, Jul 12, 2018 at 04:08:49PM -0400, Randell Jesup wrote:

I do hope that the 100 process figures scenario that was given is a worse case 
scenario though...


It's not.  Worst case is a LOT worse.

Shutting down threads/threadpools when not needed or off an idle timer
is a Good thing.  There may be some perf hit since it may mean starting
a thread instead of just sending a message at times; this may require
some tuning in specific cases, or leaving 1 thread or more running
anyways.

Stylo will be an interesting case here.

We may need to trade first-load time against memory use by lazy-initing
more things than now, though we did quite a bit on that already for
reducing startup time.


This is a really important point: Memory usage and performance deeply 
intertwined.


There are hard limits on the amount of memory we can use, and the more 
of it we waste needlessly, the less we have available for performance 
optimizations that need it. In the worst (performance) case, we wind up 
swapping, at which point performance may as well not exist.


We're going to have to make hard decisions about when/how often/how 
aggressively we flush caches, spin down threads, unload tabs, ... The 
more unnecessary overhead we save, the less extreme we're going to have 
to be about this. And the better we get at spinning down unused threads 
and evicting low impact cache entries, the less aggressive we're going 
to have to be about the high impact ones. Throwing those things away 
will have a performance impact, but not throwing them away will, in the 
end, have a bigger one.

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

Re: Intent to remove: the 'Memory usage of Subprocesses' table from about:performance

[Kris Maglione]

+1 for adding it back in the future.

Even if memory usage isn't as directly related to performance as 
CPU usage is, it has a *huge* effect on performance on memory 
constrained systems, if it causes them to have to swap.


Also, in my experience, the overlap between poorly-performing 
code and leaky code tends to be high, so it would really be nice 
to keep these numbers in one place.


On Thu, Jul 12, 2018 at 10:25:31AM -0700, Eric Rahm wrote:

Thanks Florian, considering it's roughly unmaintained right now, leaking,
and showing up in perf profiles it sounds reasonable to remove the memory
section. I've filed bug 1475301 [1] to allow us to measure USS off main
thread; we can deal with adding that back in the future if it makes sense.

-e

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1475301

On Thu, Jul 12, 2018 at 12:41 AM, Florian Quèze  wrote:


On Thu, Jul 12, 2018 at 1:18 AM, Eric Rahm  wrote:

> What performance issues are you seeing? RSS and USS should be relatively
> lightweight and the polling frequency isn't very high.

It seems ResidentUniqueDistinguishedAmount does blocking system calls,
resulting in blocking the main thread for several seconds in the worst
case. Here's a screenshot of a profile showing it:
https://i.imgur.com/DjRMQtY.png (unfortunately that profile is too big
and fails to upload with the 'share' feature).

There's also a memory leak in the implementation, after leaving
about:performance open for a couple hours, there was more than 300MB
of JS "Function" and "Call" objects (about:memory screenshot:
https://i.imgur.com/21YNDru.png ) and the devtools' Memory tool shows
that this is coming from the code queuing updates to that subprocess
memory table: https://i.imgur.com/04M71hg.png

Florian

--
Florian Quèze


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


--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

If we wish to count lines of code, we should not regard them as lines
produced but as lines spent.
--Edsger W. Dijkstra

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

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Thu, Jul 12, 2018 at 08:56:28AM -0700, Andrew McCreight wrote:

On Thu, Jul 12, 2018 at 3:57 AM, Emilio Cobos Álvarez 
wrote:


Thanks for doing this!

Just curious, is there a bug on file to measure excess capacity on
nsTArrays and hash tables?


njn looked at that kind of issue at some point (he changed how arrays grow,
for instance, to reduce overhead), but it has probably been around 5 years,
so there may be room for improvement for things added in the meanwhile.
However, our focus here is really on reducing per-process memory overhead,
rather than generic memory improvements, because we've had a lot of focus
on the latter as part of MemShrink, but not the former, so there's likely
easier improvements to be had.


I kind of suspect that improving the storage efficiency of hashtables (and 
probably nsTArrays too) will have an out-sized effect on per-process memory. 
Just at startup, for a mostly empty process, we have a huge amount of memory 
devoted to hashtables that would otherwise be shared across a bunch of 
origins—enough that removing just 4 bytes of padding per entry would save 87K 
per process. And that number tends to grow as we populate caches that we need 
for things like layout and atoms.


As much as I'd like to be able to share many of those caches between 
processes, there are always going to need process-specific hashtables on top 
of the shared ones for things that can't be/shouldn't be/aren't yet shared. 
And that extra overhead tends to grow proportionally to the number of 
processes we have.



On 07/10/2018 08:19 PM, Kris Maglione wrote:


Welcome to the first edition of the Fission MemShrink newsletter.[1]

In this edition, I'll sum up what the project is, and why it matters to
you. In subsequent editions, I'll give updates on progress that we've made,
and areas that we'll need to focus on next.[2]


The Fission MemShrink project is one of the most easily overlooked
aspects of Project Fission (also known as Site Isolation), but is
absolutely critical to its success. And will require a company- and
community-wide effort effort to meet its goals.

The problem is thus: In order for site isolation to work, we need to be
able to run *at least* 100 content processes in an average Firefox session.
Each of those processes has its own base memory overhead—memory we use just
for creating the process, regardless of what's running in it. In the
post-Fission world, that overhead needs to be less than 10MB per process in
order to keep the extra overhead from Fission below 1GB. Right now, on our
best-cast platform, Windows 10, is somewhere between 17 and 21MB. Linux and
OS-X hover between 25 and 35MB. In other words, between 2 and 3.5GB for an
ordinary session.

That means that, in the best case, we need to reduce the memory we use in
content processes by *at least* 7MB. The problem, of course, is that there
are only so many places we can cut memory without losing functionality, and
even fewer places where we can make big wins. But, there are lots of places
we can make small and medium-sized wins.

So, to put the task into perspective, of all of the places we can cut a
certain amount of overhead, here are the number of each that we need to fix
in order to reach 1MB:

250KB:   4
100KB:  10
75KB:   13
50KB:   20
20KB:   50
10KB:  100
5KB:   200

Now remember: we need to do *all* of these in order to reach our goal.
It's not a matter of one 250KB improvement or 50 5KB improvements. It's 4
250KB *and* 200 5KB improvements. There just aren't enough places we can
cut 250KB. If we fall short in any of those areas, Project Fission will
fail, and Firefox will be the only major browser without site isolation.

But it won't fail, because all of you are awesome, and this is a totally
achievable goal if we all throw our effort behind it.

Essentially what this means, though, is that if we identify an area of
overhead that's 50KB[3] or larger that can be eliminated, it *has* to be
eliminated. There just aren't that many large chunks to remove. They all
need to go. And if an area of code has a dozen 5KB chunks that can be
eliminated, maybe they don't all have to go, but at least half of them do.
The more the better.


To help us triage these issues, we have a tracking bug (
https://bugzil.la/memshrink-content), and a per-bug whiteboard tag
([overhead:...]) which gives an estimate of how much per-process overhead
we believe fixing that bug would eliminate. Please feel free to add
blockers to the tracking bug if you think they're relevant, and to add or
update [overhead] tags if you have reasonable estimates.


With all of that said, here's a brief update of the progress we've made
so far:

In the past month, unique memory per process[4] has dropped 3-4MB[5], and
JS memory usage in particular has dropped 1.1-1.9MB.

Particular credit goes to:

* Eric Rahm added an AWSY test suite to track base content process memory
   (https://bugzil.la/1442361). Results:

Resident unique: https

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Thu, Jul 12, 2018 at 12:57:35PM +0200, Emilio Cobos Álvarez wrote:

Thanks for doing this!

Just curious, is there a bug on file to measure excess capacity on 
nsTArrays and hash tables?


I don't think so, but it's a good idea.

I've actually been thinking on filing a bug to do something similar, to 
measure cumulative effects of excess padding in certain types since I 
began looking into bug 1460674, and Sylvestre mentioned that 
clang-analyzer can generate reports on excess padding.


It would probably be a good idea to try to roll this into the same 
project.


One nice change coming up on this front is that bug 1402910 will probably 
allow us to increase the load factors of most of our hashtables without 
losing performance. Having up-to-date numbers for these things would 
probably help decide how to prioritize those sorts of bugs.



On 07/10/2018 08:19 PM, Kris Maglione wrote:

Welcome to the first edition of the Fission MemShrink newsletter.[1]

In this edition, I'll sum up what the project is, and why it matters 
to you. In subsequent editions, I'll give updates on progress that 
we've made, and areas that we'll need to focus on next.[2]



The Fission MemShrink project is one of the most easily overlooked 
aspects of Project Fission (also known as Site Isolation), but is 
absolutely critical to its success. And will require a company- and 
community-wide effort effort to meet its goals.


The problem is thus: In order for site isolation to work, we need to 
be able to run *at least* 100 content processes in an average 
Firefox session. Each of those processes has its own base memory 
overhead—memory we use just for creating the process, regardless of 
what's running in it. In the post-Fission world, that overhead needs 
to be less than 10MB per process in order to keep the extra overhead 
from Fission below 1GB. Right now, on our best-cast platform, 
Windows 10, is somewhere between 17 and 21MB. Linux and OS-X hover 
between 25 and 35MB. In other words, between 2 and 3.5GB for an 
ordinary session.


That means that, in the best case, we need to reduce the memory we 
use in content processes by *at least* 7MB. The problem, of course, 
is that there are only so many places we can cut memory without 
losing functionality, and even fewer places where we can make big 
wins. But, there are lots of places we can make small and 
medium-sized wins.


So, to put the task into perspective, of all of the places we can 
cut a certain amount of overhead, here are the number of each that 
we need to fix in order to reach 1MB:


250KB:   4
100KB:  10
75KB:   13
50KB:   20
20KB:   50
10KB:  100
5KB:   200

Now remember: we need to do *all* of these in order to reach our 
goal. It's not a matter of one 250KB improvement or 50 5KB 
improvements. It's 4 250KB *and* 200 5KB improvements. There just 
aren't enough places we can cut 250KB. If we fall short in any of 
those areas, Project Fission will fail, and Firefox will be the only 
major browser without site isolation.


But it won't fail, because all of you are awesome, and this is a 
totally achievable goal if we all throw our effort behind it.


Essentially what this means, though, is that if we identify an area 
of overhead that's 50KB[3] or larger that can be eliminated, it 
*has* to be eliminated. There just aren't that many large chunks to 
remove. They all need to go. And if an area of code has a dozen 5KB 
chunks that can be eliminated, maybe they don't all have to go, but 
at least half of them do. The more the better.



To help us triage these issues, we have a tracking bug 
(https://bugzil.la/memshrink-content), and a per-bug whiteboard tag 
([overhead:...]) which gives an estimate of how much per-process 
overhead we believe fixing that bug would eliminate. Please feel 
free to add blockers to the tracking bug if you think they're 
relevant, and to add or update [overhead] tags if you have 
reasonable estimates.



With all of that said, here's a brief update of the progress we've 
made so far:


In the past month, unique memory per process[4] has dropped 
3-4MB[5], and JS memory usage in particular has dropped 1.1-1.9MB.


Particular credit goes to:

* Eric Rahm added an AWSY test suite to track base content process memory
  (https://bugzil.la/1442361). Results:

   Resident unique: 
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1684862,1,4=mozilla-central,1684846,1,4=mozilla-central,1685133,1,4=mozilla-central,1685127,1,4

   Explicit allocations: 
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound,1706218,1,4=mozilla-inbound,1706220,1,4=mozilla-inbound,1706216,1,4

   JS: 
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1684866,1,4=mozilla-central,1685137,1,4=mozilla-central,1685131,1,4


* Andrew McCreight created a tool for tracking JS memory usage, and 
figuring

  out which scripts and objects are responsible for how much of it
  (https://bugzil.la/1463569).

* Andrew

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Wed, Jul 11, 2018 at 11:42:01PM +0200, Jean-Yves Avenard wrote:

On 11 Jul 2018, at 10:10 pm, Kris Maglione  wrote:
It looks like it will be helpful, but unfortunately won't give us the 2MB 
simple arithmetic would suggest. On Windows, at least, (and probably 
elsewhere, but need to confirm) thread stacks are lazily committed, so as long 
as the decoders aren't used in a process, the overhead is probably closer to 
25KB per thread.


I haven’t looked much in details, not being an expert on this and having just 
finished watching the world cup…


A quick glance at the code gives me:

On mac/linux using pthread:
when a thread is created, the stack size is set using pthread_attr_setstacksize
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptthread.c#355

On Linux, the man page is clear:
"The stack size attribute determines the minimum size (in bytes) that will be 
allocated for threads created using the thread attributes object attr.”


Right, but allocation size doesn't imply that the memory is committed, just that 
it's mapped. In general, anonymous mapped memory isn't actually committed (and 
therefore doesn't become part of the process's USS) until it's touched.



On Windows:
https://searchfox.org/mozilla-central/source/nsprpub/pr/src/md/windows/w95thred.c#151

the thread is created with STACK_SIZE_PARAM_IS_A_RESERVATION flag set. This 
will allocate the memory immediately.


Allocate, yes, but not commit. That flag is actually what ensures that our 
Windows thread stacks don't consume system memory until they're actually 
touched.


The saving I was mentioning earlier isn’t just due to media decoder threadpool 
thread stack no longer needing to be that big, but that all other threadpools 
can be reduced too. Threadpools aren’t used only when playing a video/audio 
file.


Reducing thread pool sizes would certainly be helpful. One unfortunate 
side-effect of large thread pools is that, even with lazy commit thread stacks, 
the more threads you run code on, the more stacks wind up with committed pages.

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

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Wed, Jul 11, 2018 at 01:49:04PM +0200, Jean-Yves Avenard wrote:

There’s one place where we could gain heaps is in the media stack.
Currently, each content process allocate a thread-pool with at least 8 
threads for use with the media decoders, each threads a default stack size of 
256kB.

(https://searchfox.org/mozilla-central/source/xpcom/threads/nsIThreadManager.idl#53)

That stack size has been increased over the years due to the growing use of 
either system frameworks (in particular the mac CoreVideo framework that use 
over 200kB alone), and right now 256kB itself isn’t enough for the new AV1 
decoder from libaom.


One of the work the media team has started, is to have all those decoders run 
in a dedicated process: the reason for this work was mostly done for security 
reasons, but there will be side gains memory-wise.


This work is tracked in bug 1471535 
(https://bugzilla.mozilla.org/show_bug.cgi?id=1471535)


Once this is done, and we no longer calls decoders in the content process, 
the decoder process could use an increase stack size, while reducing the 
content process default stack size to 128kB (and maybe even 64kB)


That alone may be sufficient to achieve your mentioned goals.


Thanks. Boris added this as a blocker.

It looks like it will be helpful, but unfortunately won't give us the 2MB 
simple arithmetic would suggest. On Windows, at least, (and probably 
elsewhere, but need to confirm) thread stacks are lazily committed, so as long 
as the decoders aren't used in a process, the overhead is probably closer to 
25KB per thread.


Shrinking the size of the thread pool and lazily spinning up threads when 
they're first needed would probably save us 200KB per process, though...



An immediate intermediary step could be to use two different stack sizes as we 
pretty much know which one needs more over others.

JY



On 10 Jul 2018, at 8:19 pm, Kris Maglione  wrote:

Welcome to the first edition of the Fission MemShrink newsletter.[1]

In this edition, I'll sum up what the project is, and why it matters to you. In 
subsequent editions, I'll give updates on progress that we've made, and areas 
that we'll need to focus on next.[2]


The Fission MemShrink project is one of the most easily overlooked aspects of 
Project Fission (also known as Site Isolation), but is absolutely critical to 
its success. And will require a company- and community-wide effort effort to 
meet its goals.

The problem is thus: In order for site isolation to work, we need to be able to 
run *at least* 100 content processes in an average Firefox session. Each of 
those processes has its own base memory overhead—memory we use just for 
creating the process, regardless of what's running in it. In the post-Fission 
world, that overhead needs to be less than 10MB per process in order to keep 
the extra overhead from Fission below 1GB. Right now, on our best-cast 
platform, Windows 10, is somewhere between 17 and 21MB. Linux and OS-X hover 
between 25 and 35MB. In other words, between 2 and 3.5GB for an ordinary 
session.

That means that, in the best case, we need to reduce the memory we use in 
content processes by *at least* 7MB. The problem, of course, is that there are 
only so many places we can cut memory without losing functionality, and even 
fewer places where we can make big wins. But, there are lots of places we can 
make small and medium-sized wins.

So, to put the task into perspective, of all of the places we can cut a certain 
amount of overhead, here are the number of each that we need to fix in order to 
reach 1MB:

250KB:   4
100KB:  10
75KB:   13
50KB:   20
20KB:   50
10KB:  100
5KB:   200

Now remember: we need to do *all* of these in order to reach our goal. It's not 
a matter of one 250KB improvement or 50 5KB improvements. It's 4 250KB *and* 
200 5KB improvements. There just aren't enough places we can cut 250KB. If we 
fall short in any of those areas, Project Fission will fail, and Firefox will 
be the only major browser without site isolation.

But it won't fail, because all of you are awesome, and this is a totally 
achievable goal if we all throw our effort behind it.

Essentially what this means, though, is that if we identify an area of overhead 
that's 50KB[3] or larger that can be eliminated, it *has* to be eliminated. 
There just aren't that many large chunks to remove. They all need to go. And if 
an area of code has a dozen 5KB chunks that can be eliminated, maybe they don't 
all have to go, but at least half of them do. The more the better.


To help us triage these issues, we have a tracking bug 
(https://bugzil.la/memshrink-content), and a per-bug whiteboard tag 
([overhead:...]) which gives an estimate of how much per-process overhead we 
believe fixing that bug would eliminate. Please feel free to add blockers to 
the tracking bug if you think they're relevant, and to add or update [overhead] 
tags if you have reasonable estimates.


With all of that said, here's a brief

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

On Wed, Jul 11, 2018 at 02:42:11PM +0200, David Bruant wrote:

2018-07-10 20:19 GMT+02:00 Kris Maglione :


The problem is thus: In order for site isolation to work, we need to be
able to run *at least* 100 content processes in an average Firefox session


I've seen this information of 100 content processes in a couple places but
i haven't been able to find the rationale for it. How was the 100 number
picked?


So, the basic problem here is that we don't get to choose the number of 
content processes we'll have. It will depend entirely on the number of 
origins that we load documents from at any given time. In practice, the 
biggest contributing factor to that number tends to be iframes (mostly 
for things like ads and social widgets).


The "100 processes" number was initially chosen based on experimentation 
(basically, counting the number of origins loaded by typical pages on 
certain popular sites) and our knowledge of typical usage patterns. It's 
meant to be a conservative estimate of the number of processes typical 
users are likely to hit on a regular basis, though hopefully not all the 
time.


For heavy users, we expect the number to be much higher[1]. And while those 
users typically have more RAM to spare, they also tend not to be happy 
when we waste it.


We also need to add to that number the Activity Stream process that 
hosts things like about:newtab and about:home, the system extension 
process, processes for any other extensions the user has installed 
(which will each likely need their own processes for the same reasons 
each content origin will), and the pre-loaded web content process[4].



We've been working on improving our estimates by collecting telemetry on 
the number of document groups[2] per tab group[3]:


https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=1_date=2018-06-30=__none__!__none__!__none___channel_version=nightly%252F63=TOTAL_HTTP_DOCGROUPS_PER_TABGROUP_channel_version=null=*=Firefox=0_keys=submissions_date=2018-06-25=0=1_submission_date=0

But we don't have enough data to draw conclusions yet.


Would 90 prevent a release of project fission?


This isn't really something we get to choose. The closest I can come is 
something like "would an overhead of 1.1GB prevent a release of project 
Fission". And, while the answer may turn out to be "no", I'd prefer not 
to speculate, because that's a decision we'd wind up paying for with 
user dissatisfaction.


There are some other hacks that we can use to decrease the overall 
overhead, like aggressively unloading background tabs, and flushing 
their resources. We're almost certainly going to wind up having to do 
some of that regardless, but it comes at a performance cost. The more 
aggressive we have to be about it, the less responsive the browser is 
going to wind up being. So, again, the shorter we fall on our memory 
reduction efforts, the more we're going to pay in terms of user 
satisfaction.



How will the rollout happen?
  Will the rollout happen progressively (like 2 content processes soon, 4
soon after, 10 some time after, etc.) or does it have to be 1 (current
situation IIUC) then 100?


* Andrew McCreight created a tool for tracking JS memory usage, and figuring

  out which scripts and objects are responsible for how much of it
  (https://bugzil.la/1463569).


How often is this code run? Is there a place to find the daily output of
this tool applied to a nightly build for instance?


For the moment, it requires a patched build of Firefox, so we've been 
running it locally as we try to track down and fix memory issues, and 
Andrew has been periodically updating the numbers in the bug.


I believe Andrew has been working on updating the patch to a land-able 
state (which is non-trivial), after which we'll hopefully be able to get 
up-to-date numbers from automation.



[1]: Particularly readers of TechCrunch, which regularly loads 30 
origins on a single page.

[2]: Essentially documents of different origin.
[3]: Essentially sets of tabs that are tied together because they were 
opened by things like window.open() calls or link clicks from other 
tabs.
[4]: Which currently have only one of, but may need more of in the 
future in order to support loading several iframes in a given page 
without noticeable lag or jank.

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

Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[Kris Maglione]

Welcome to the first edition of the Fission MemShrink newsletter.[1]

In this edition, I'll sum up what the project is, and why it matters to you. 
In subsequent editions, I'll give updates on progress that we've made, and 
areas that we'll need to focus on next.[2]



The Fission MemShrink project is one of the most easily overlooked aspects of 
Project Fission (also known as Site Isolation), but is absolutely critical to 
its success. And will require a company- and community-wide effort effort to 
meet its goals.


The problem is thus: In order for site isolation to work, we need to be able 
to run *at least* 100 content processes in an average Firefox session. Each of 
those processes has its own base memory overhead—memory we use just for 
creating the process, regardless of what's running in it. In the post-Fission 
world, that overhead needs to be less than 10MB per process in order to keep the 
extra overhead from Fission below 1GB. Right now, on our best-cast platform, 
Windows 10, is somewhere between 17 and 21MB. Linux and OS-X hover between 25 
and 35MB. In other words, between 2 and 3.5GB for an ordinary session.


That means that, in the best case, we need to reduce the memory we use in 
content processes by *at least* 7MB. The problem, of course, is that there are 
only so many places we can cut memory without losing functionality, and even 
fewer places where we can make big wins. But, there are lots of places we can 
make small and medium-sized wins.


So, to put the task into perspective, of all of the places we can cut a 
certain amount of overhead, here are the number of each that we need to fix in 
order to reach 1MB:


250KB:   4
100KB:  10
75KB:   13
50KB:   20
20KB:   50
10KB:  100
5KB:   200

Now remember: we need to do *all* of these in order to reach our goal. It's 
not a matter of one 250KB improvement or 50 5KB improvements. It's 4 250KB *and* 
200 5KB improvements. There just aren't enough places we can cut 250KB. If we 
fall short in any of those areas, Project Fission will fail, and Firefox will be 
the only major browser without site isolation.


But it won't fail, because all of you are awesome, and this is a totally 
achievable goal if we all throw our effort behind it.


Essentially what this means, though, is that if we identify an area of 
overhead that's 50KB[3] or larger that can be eliminated, it *has* to be 
eliminated. There just aren't that many large chunks to remove. They all need 
to go. And if an area of code has a dozen 5KB chunks that can be eliminated, 
maybe they don't all have to go, but at least half of them do. The more the 
better.



To help us triage these issues, we have a tracking bug (https://bugzil.la/memshrink-content), 
and a per-bug whiteboard tag ([overhead:...]) which gives an estimate of how 
much per-process overhead we believe fixing that bug would eliminate. Please 
feel free to add blockers to the tracking bug if you think they're relevant, and 
to add or update [overhead] tags if you have reasonable estimates.



With all of that said, here's a brief update of the progress we've made so far:

In the past month, unique memory per process[4] has dropped 3-4MB[5], and JS 
memory usage in particular has dropped 1.1-1.9MB.


Particular credit goes to:

* Eric Rahm added an AWSY test suite to track base content process memory
  (https://bugzil.la/1442361). Results:

   Resident unique: 
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1684862,1,4=mozilla-central,1684846,1,4=mozilla-central,1685133,1,4=mozilla-central,1685127,1,4
   Explicit allocations: 
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-inbound,1706218,1,4=mozilla-inbound,1706220,1,4=mozilla-inbound,1706216,1,4
   JS: 
https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1684866,1,4=mozilla-central,1685137,1,4=mozilla-central,1685131,1,4

* Andrew McCreight created a tool for tracking JS memory usage, and figuring
  out which scripts and objects are responsible for how much of it
  (https://bugzil.la/1463569).

* Andrew and Nika Layzell also completely rewrote the way we handle XPIDL type
  info so that it's statically compiled into the executable and shared between
  all processes (https://bugzil.la/1438688, https://bugzil.la/1444745).

* Felipe Gomes split a bunch of code out of frame scripts so that it could be
  lazily loaded only when needed (https://bugzil.la/1467278, ...) and added a
  whitelist of JSMs that are allowed to be loaded at content process startup
  (https://bugzil.la/1471066)

* I did a bit of this too, and also prevented us from loading some other JSMs
  before we need them (https://bugzil.la/1470333, https://bugzil.la/1469719,
  ...)

* Nick Nethercote made dynamic nsAtoms allocate their string storage inline
  rather than use a refcounted StringBuffer (https://bugzil.la/1447951)

* Emilio Álvarez reduced the amount of memory the Gecko Profiler uses in
  content processes.

* Nathan Froyd fixed 

Re: CPOWs are now almost completely disabled

[Kris Maglione]

\o/

On Thu, Jun 28, 2018 at 12:52:51AM +0200, Tom Schuster wrote:

Since landing bug 1465911 [1], CPOWs [2] are only functional on our testing
infrastructure. In normal builds that we ship to users CPOWs can be
created, but no operations like property lookup can be performed on them.

CPOWs continue to exist, because a lot of tests still depend on them. We
can't disable CPOW creation in user builds, because the context menu passes
them from the child to the parent and back like a token.

This is a significant IPC attack surface reduction.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1465911
[2]
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers

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

Re: mozilla-inbound backout policy subject to change (become similar to autoland)

[Kris Maglione]

Just to add one other point to this discussion:

I *hate* backouts. I don't just mean of my patches. I mean in 
general. Whenever I come across a backout in a blame walk or a 
bisect, it makes my life much more difficult. And I know I'm not 
alone in this.


I thought the policy for autoland was pretty reasonable when it 
was meant to use history rewriting to actually remove bad 
changesets from change history, but I haven't been a fan of it 
since that plan was abandoned.


If this new policy gives us open trees a slightly higher 
percentage of the time at the expense of winding up with more 
backouts for trivial ESLint bustages or one-line test fixes, I 
don't think it's worth it. Having to wait for trees to re-open 
is not a huge inconvenience to me. Dealing with mangled blame 
is.


On Sun, Jun 24, 2018 at 03:28:45PM -0400, Boris Zbarsky wrote:

On 6/19/18 9:04 AM, Sebastian Hengst wrote:
TL;DR: We would like to change the mozilla-inbound backout policy to 
be like autoland’s.


This seems like a pretty reasonable change in general.

Is there a well-documented try syntax string which corresponds to 
"these are the things that need to be green to avoid being backed 
out"? Presumably that string is not "-p all -u all" because it should 
exclude tier2 and lower things, but it's not entirely clear to me what 
the autoland backout criteria are.  I would assume we want developers 
to do a try run with that syntax before pushing to inbound as needed.


-Boris

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

Re: XPIDL trajectory

[Kris Maglione]

On Mon, Jun 25, 2018 at 09:45:22PM -0700, L. David Baron wrote:

On Tuesday 2018-06-26 14:29 +1000, Nicholas Nethercote wrote:

The trend is clearly down, except for the large increase in .xpt size for
the most recent measurement -- note the extra digit! It appears that .xpt
files used to be binary, and now they are JSON. This might be related to
mccr8's recent XPT overhaul (bug 1438688)?


What's the relative value of making something not use xpidl anymore
vs. marking an xpidl interface as no longer [scriptable]?

(I hope that marking it not [scriptable] would mean we don't
generate .xpt data for it... although I haven't checked.  I *think*
mccr8's XPT work means that we no longer have duplicate in-memory
copies of the xpt data across processes, though, so some of this
isn't as big a deal as it used to be.)


For C++, it generally means fewer vtables/virtual calls (which 
is currently a significant win for content process memory), and 
generally a nicer API surface.

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

Re: mozilla-inbound backout policy subject to change (become similar to autoland)

[Kris Maglione]

On Sun, Jun 24, 2018 at 03:28:45PM -0400, Boris Zbarsky wrote:

On 6/19/18 9:04 AM, Sebastian Hengst wrote:
TL;DR: We would like to change the mozilla-inbound backout policy to 
be like autoland’s.


This seems like a pretty reasonable change in general.

Is there a well-documented try syntax string which corresponds to 
"these are the things that need to be green to avoid being backed 
out"? Presumably that string is not "-p all -u all" because it should 
exclude tier2 and lower things, but it's not entirely clear to me what 
the autoland backout criteria are.  I would assume we want developers 
to do a try run with that syntax before pushing to inbound as needed.


I tend to worry about the amount of extra automation runtime 
that kind of policy will trigger. I tend to try to limit my try 
runs to areas they're likely to affect to avoid wasting 
thousands of hours of machine time on things I know cannot be 
affected by a patch. The integration trees try to do something 
similar, based on changed files. Try does not. When you specify 
`-p all -u all`, it really does try to run all tests. Including 
things like ccov, which always fail, and also don't matter for 
try runs.


I also don't know how much it would affect the oranges I get on 
inbound. A significant fraction of those are rebase issues, 
because I tend to do my first try run before review (or while 
waiting for review), and don't want to do a whole new try run 
when I land.


Most of the rest are because I almost always forget to do 
Android runs, and sometimes forget what sorts of weird things 
can break Windows but not other platforms. But our Android and 
Windows slaves tend to be overloaded as it is, and I don't want 
to just start throwing extra jobs at them every time I write a 
commit, on the odd chance that one of my changes will affect 
them. Though there are definitely specific sorts of patches I 
should remember to run Android and Windows mochitests on more 
often.

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

Re: Intent to move Activity Stream into its own process

[Kris Maglione]

On Tue, Jun 19, 2018 at 06:26:11PM +0200, Dão Gottwald wrote:

This is about the code behind about:newtab and about:home, yes? Or some
broader Activity Stream magic? Activity Stream has become kind of a fuzzy
term, to the point where I'm not sure anymore what it means.


This is about about:home and about:newtab, yes. There will still 
be Activity Stream code in the main process, but those pages 
will move from an ordinary content process to their own content 
process.



2018-06-18 21:58 GMT+02:00 Kris Maglione :


+1

This should also have some memory benefits by preventing us from
unnecessarily loading AS things into multiple content processes, like we do
now.


On Mon, Jun 18, 2018 at 03:35:01PM -0400, Mike Conley wrote:


(posted on both dev-platform and firefox-dev. Please send replies to
dev-platform)

Hello all,

Just sending a quick note to let you know that I've filed bug 1469072[1]
to
move Activity Stream into its own content process.

This is something that Fission needs, but we're hoping to squeeze some
performance benefits out of it as well by reducing, reusing and recycling
various Activity Stream things since they'll all be contained within a
single process. It might also allow us to cache more Activity Stream
scripts in ScriptPreloader[2], since we can reasonably assume only trusted
JavaScript will be running within the Activity Stream content process.

This work is going to be handled by my intern, Jay Lim. If you have any
concerns with this plan, please bring them to my attention - commenting in
the bug, emailing me, or responding to this thread on dev-platform is
fine.

Thanks!

-Mike

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1469072
[2]: I wrote about ScriptPreloader on my blog here:
https://mikeconley.ca/blog/2018/05/30/firefox-performance-update-9/

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

Re: Intent to move Activity Stream into its own process

[Kris Maglione]

On Mon, Jun 18, 2018 at 09:19:18PM +0100, Gijs Kruitbosch wrote:

Hey,

This sounds really interesting. However, wouldn't this mean that we 
will do a process switch for the tab's browser whenever we load a URL 
in the same tab that has AS in it? Or would you still intend to run 
the actual AS new tab page in a "normal" tab process?


If it *would* mean a process switch, we may want to take another look 
at some of the bugs relating to those (process switches are currently 
relatively rare) and re-prioritize them.


It will definitely need to mean a process switch. For the sake 
of Fission, we really need to never run content JS in the same 
process that AS runs in. And for the sake of content process 
memshrink, we really need to not load AS into multiple content 
processes.



On 18/06/2018 20:35, Mike Conley wrote:

(posted on both dev-platform and firefox-dev. Please send replies to
dev-platform)

Hello all,

Just sending a quick note to let you know that I've filed bug 1469072[1] to
move Activity Stream into its own content process.

This is something that Fission needs, but we're hoping to squeeze some
performance benefits out of it as well by reducing, reusing and recycling
various Activity Stream things since they'll all be contained within a
single process. It might also allow us to cache more Activity Stream
scripts in ScriptPreloader[2], since we can reasonably assume only trusted
JavaScript will be running within the Activity Stream content process.

This work is going to be handled by my intern, Jay Lim. If you have any
concerns with this plan, please bring them to my attention - commenting in
the bug, emailing me, or responding to this thread on dev-platform is fine.

Thanks!

-Mike

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1469072
[2]: I wrote about ScriptPreloader on my blog here:
https://mikeconley.ca/blog/2018/05/30/firefox-performance-update-9/

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

Re: Intent to move Activity Stream into its own process

[Kris Maglione]

+1

This should also have some memory benefits by preventing us from 
unnecessarily loading AS things into multiple content processes, 
like we do now.


On Mon, Jun 18, 2018 at 03:35:01PM -0400, Mike Conley wrote:

(posted on both dev-platform and firefox-dev. Please send replies to
dev-platform)

Hello all,

Just sending a quick note to let you know that I've filed bug 1469072[1] to
move Activity Stream into its own content process.

This is something that Fission needs, but we're hoping to squeeze some
performance benefits out of it as well by reducing, reusing and recycling
various Activity Stream things since they'll all be contained within a
single process. It might also allow us to cache more Activity Stream
scripts in ScriptPreloader[2], since we can reasonably assume only trusted
JavaScript will be running within the Activity Stream content process.

This work is going to be handled by my intern, Jay Lim. If you have any
concerns with this plan, please bring them to my attention - commenting in
the bug, emailing me, or responding to this thread on dev-platform is fine.

Thanks!

-Mike

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1469072
[2]: I wrote about ScriptPreloader on my blog here:
https://mikeconley.ca/blog/2018/05/30/firefox-performance-update-9/

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

PSA: Please use XPCOMUtils.defineLazyGlobalGetters rather than Cu.importGlobalProperties

[Kris Maglione]

tl;dr: importGlobalProperties is expensive. If you don't need the imported 
properties immediately, please use defineLazyGlobalGetters instead.


Calling importGlobalProperties immediately defines the properties that you're 
importing and any prototypes that they require. Aside from CPU overhead, this 
also tends to consume a lot of memory, especially for objects with complex 
prototypes. And it does this once for every global you call it in. This is 
especially a problem for content processes, since we get this memory overhead 
in each and every content process (and remember, as a part of Project Fission, 
we're aiming to be able to run 100 content processes for ordinary users).


Ordinary WebIDL globals don't really have this problem, since they're already 
defined lazily via resolveProperty hooks. In bug 1464548, I added 
XPCOMUtils.defineLazyGlobalGetters, which has somewhat similar behavior, and 
also has the benefit of always creating the global in the shared JSM module 
scope, no matter which global it's called from. Please use that helper from 
now on unless you have a very good reason not to.


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

Re: PSA: Please use ChromeUtils.generateQI for JS QueryInterface methods

[Kris Maglione]

Already done. :) https://bugzil.la/1460092

On Fri, May 18, 2018 at 02:48:20PM -0400, Jared Wein wrote:

Have you looked in to adding an eslint rule for this? The eslint rule[1]
that recommends usage of ChromeUtils.defineModuleGetter instead of
XPCOMUtils.defineLazyModuleGetter has been very useful.

[1]
https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-import.js#2

Thanks,
Jared


On Tue, May 8, 2018 at 3:26 PM Kris Maglione <kmagli...@mozilla.com> wrote:


Bug 1456035 added a ChromeUtils.generateQI helper to create
QueryInterface methods for JS objects. The QueryInterface methods
generated by this function are highly optimized, and have a fast path
for calls from XPConnect.

Which is to say, please use this method rather than manually writing
your QueryInterface functions, or using XPCOMUtils.generateQI.
QueryInterface methods tend to be very hot code, and all of the extra
overhead from JS implementations adds up fast.

-Kris
___
firefox-dev mailing list
firefox-...@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev



--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

The first principle is that you must not fool yourself — and you are
the easiest person to fool.
--Richard Feynman

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

Re: Investigating leaks using DMD heap scan mode

[Kris Maglione]

On Tue, May 08, 2018 at 04:08:27PM -0700, Andrew McCreight wrote:

A few years ago I came up with an approach for using a combination of cycle
collector logs and DMD (which tracks all live blocks of heap allocated
memory) to investigate leaks of refcounted objects. I've only had to use it
about a half dozen times (most recently in bug 1451985), but it is very
handy when you have no other ideas. I think it is an easier-to-use
alternative to refcount logging in most situations.


Awesome. Thanks.

Whenever I've had to resort to refcount logging to find a leak, 
I've ended the day with a headache. The next time I have the 
occasion to use this, I will probably owe you a bottle of 
Scotch.

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

PSA: Please use ChromeUtils.generateQI for JS QueryInterface methods

[Kris Maglione]

Bug 1456035 added a ChromeUtils.generateQI helper to create 
QueryInterface methods for JS objects. The QueryInterface methods 
generated by this function are highly optimized, and have a fast path 
for calls from XPConnect.


Which is to say, please use this method rather than manually writing 
your QueryInterface functions, or using XPCOMUtils.generateQI. 
QueryInterface methods tend to be very hot code, and all of the extra 
overhead from JS implementations adds up fast.


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

Re: PSA: new helper class for MozPromise in DOM code

[Kris Maglione]

On Thu, Apr 26, 2018 at 04:12:09PM -0400, Ben Kelly wrote:

I pushed a new helper class to inbound today to make it easier to work with
MozPromise in DOM code.  Specifically, it allows you to auto-disconnect a
Thenable request when the global dies.


Thank you! I've been worried about these kinds of footguns for a 
while. We could probably use something similar for JS...



   RefPtr<DOMMozPromiseRequestHolder> holder =
 new DOMMozPromiseRequestHolder(global);


Note, this can be written a bit more concisely as:

   auto holder = MakeRefPtr<DOMMozPromiseRequestHolder>(global);

--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

All the hundreds of millions of people who, in their time, believed
the Earth was flat never succeeded in unrounding it by an inch.
--Isaac Asimov

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

Re: PSA: Deprecating JS-Implemented WebIDL

[Kris Maglione]

On Tue, Apr 24, 2018 at 07:25:18AM +0100, Andreas Tolfsen wrote:

Also sprach Bobby Holley:


For reasons outlined in bug 1450827, the DOM peers have decided to
deprecate support for JS-implemented WebIDL APIs. This means that
new additions of |JSImplementation="foo"| are no longer permitted.



Out of curiosity, and mostly because my knowledge of how this works
is limited, this wouldn’t affect C++ implementations that internally
do_GetService to some XPCOM service implemented in JS?


Correct, it would not affect those implementations.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Is super-review still a thing?

[Kris Maglione]

I can't remember the last time I saw a super-review request, but 
it's still documented as a policy[1]. Is it still a thing? Do we 
want it to still be a thing?


The reason that I ask is that I have a problem that I think I 
might be able to solve by co-opting the super-review flag, but I 
don't want to trample on it if we're still using it as 
originally designed.


My problem is essentially that, for a few areas of code, I'm the 
final arbiter, or at least the main blocker, for a lot of large 
or critical architectural or API changes. The upshot of that is 
that I get a lot of review requests for patches in those areas, 
and most of the patches that make it into my queue are large 
and/or complicated. This situation gets exhausting after a 
while. And since people know that I tend to be busy, they also 
avoid flagging me to review smaller patches, even when I really 
*should* look at those patches (and therefore notice issues with 
them only when I read code later).


For a lot of these patches, my opinion is only really critical 
for certain architectural aspects, or implementation aspects at 
a few critical points. There are other reviewers who are 
perfectly qualified to do a more detailed review of the 
specifics of the patch, and have more spare cycles to devote to 
it. Essentially, what's needed from me in these cases is a 
super-review, which I can do fairly easily, but instead I become 
a bottleneck for the code review as well.


So, for the areas where I have this responsibility, I'd like to 
institute a policy that certain types of changes need a final 
super-review from me, but should get a detailed code review from 
another qualified reviewer when that makes sense.



Does anyone have any objection to this plan? Or any suggestions 
for another way to go about it?


-Kris

[1]: https://www.mozilla.org/en-US/about/governance/policies/reviewers/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using WebIDL objects in XPIDL

[Kris Maglione]

On Thu, Apr 19, 2018 at 12:58:33PM -0700, Andrew McCreight wrote:

On Thu, Apr 19, 2018 at 12:53 PM, Gijs Kruitbosch 
wrote:

If I understand correctly, with these changes I could now just use Promise
instead. Is that correct? Can I declare it including the resolution result
of the promise, ie can I now use:

Promise asyncGetFoo()

in xpidl? Because that would make my life considerably easier...



Bug 1455217 by Nika is going to add a Promise type to XPIDL.  So I think
you have to wait for that.


This actually isn't as difficult currently as you may expect. At 
least, the difference between XPIDL and WebIDL isn't. From the 
C++ consumer, you'd just have to do:


 RefPtr result(Promise::Resolve(global, cx, resultVal, rv));

Which is more or less what the WebIDL layer does.

The real problem is that you're going to have to do the coercion 
on the resolution value yourself. That problem is the same 
whether you're implementing it in WebIDL or XPIDL.


If you're just dealing with an integer value, that's pretty 
easy. Something like `result.toInt32()` should do the job.


If it's something more complicated, well, it's more complicated. 
But WebIDL dictionary types are probably by far the easiest way 
to handle it.



So, basically what I'm saying is that a callback interface is 
still probably the easiest way to handle this from the C++ side, 
but a Promise should work, and shouldn't be that much harder in 
XPIDL than it is in WebIDL.

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

Re: Using WebIDL objects in XPIDL

[Kris Maglione]

\o/

On Wed, Apr 18, 2018 at 11:19:27PM -0400, Nika Layzell wrote:

Yesterday, bug 1444991
 landed on inbound,
which adds support for using WebIDL defined objects directly inside of
XPIDL definition files.

This allows us to avoid using nsISupports to pass WebIDL objects which
don't have corresponding XPIDL interfaces through XPIDL defined methods.

WebIDL objects must be forward declared, like XPIDL types, in the .idl file
which uses them:

 // C++ 'nsFrameLoader', from FrameLoader.webidl
 webidl FrameLoader;

 // C++ 'mozilla::dom::DataTransfer', from DataTransfer.webidl
 webidl DataTransfer;

These types may then be used like XPIDL types in method and attribute
declarations. These types can be used from both C++ and JS code. They are
exposed into Rust as '*const ::libc::c_void`.

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

Re: XPT files shouldn't be added to package-manifest.in any more

[Kris Maglione]

On Wed, Apr 18, 2018 at 03:15:23PM -0400, Boris Zbarsky wrote:

On 4/18/18 1:54 PM, Gijs Kruitbosch wrote:
Given that we're also not supposed to be making new JS-implemented 
webidl things, what's the long-term plan for artifact build support 
for changing JS-implemented interfaces?


For the specific case of "always known to be JS to JS" calls, why are 
we using an IDL-declared interface at all, if I might ask?


Legacy. The blocklist service is ancient, and has some C++ 
consumers. The JS-only parts wound up getting glommed onto the 
XPIDL interface over time. I'd already suggested we fix that 
before I realized it was the cause of this problem.

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

Re: The future of "remote XUL"

[Kris Maglione]

On Tue, Mar 27, 2018 at 03:48:24PM +, Dave Townsend wrote:

My understanding is that it has been effectively unsupported for some time
anyway so I think we should just go ahead and disable it altogether at this
point. If we need bits for automated tests then we should work to switch
tests away from those if we can.


This has been my understanding too. When we removed support for 
remote XUL by default, and then eventually moved it to a site 
whitelist, those were both stepping stones to removing support 
entirely. But it's been officially unsupported since the first 
step, and there hasn't been any way to enable it since we 
removed legacy extension support.


It's also been progressively breaking over time, and I've been 
WONTFIXing bugs about remote XUL with prejudice.


If whatever support is left is causing development friction, we 
should just remove it.



On Tue, Mar 27, 2018 at 8:36 AM Boris Zbarsky  wrote:


Background: We currently have various provisions for "remote XUL",
wherein a hostname is whitelisted to:

1)  Allow parsing XUL coming from that hostname (not normally alllowed
for the web).

2)  Allow access to XPConnect-wrapped objects, assuming someone hands
out such an object.

3)  Run XBL JS in the same global as the webpage.

4)  Allow access to a "cut down" Components object, which has
Components.interfaces but not Components.classes, for example.

This machinery is also used for the "dom.allow_XUL_XBL_for_file"
preference.

The question is what we want to do with this going forward.  From my
point of view, I would like to eliminate item 4 above, to reduce
complexity.  I would also like to eliminate item 2 above, because that
would get us closer to having the invariant that XPConnect is only used
from system principals.  These two changes are likely to break some
remote XUL consumers.

The question is whether we should just go ahead and disable remote XUL
altogether, modulo its usage in our test suites and maybe
"dom.allow_XUL_XBL_for_file" (for local testing).  It seems like that
might be clearer than a dribble of breakage as we remove bits like items
2/4 above, slowly remove various bindings, slowly remove support for
some XUL tags, etc...

Thoughts?  My gut feeling is that we should just turn off remote XUL
outside the IsInAutomation() and maybe the "dom.allow_XUL_XBL_for_file"
case.

-Boris

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

Re: FYI: Short Nightly Shield Study involving DNS over HTTPs (DoH)

[Kris Maglione]

On Tue, Mar 20, 2018 at 11:43:13AM +0100, Frederik Braun wrote:



On 20.03.2018 04:33, Dave Townsend wrote:

The DoH service
we're using is likely more private than anything the user is currently
using.


This is only true for some parts of the world.
I'd like us not to regress for our user base globally here.


That's only true to a certain extent. Some parts of the world may have 
better regulations for ISPs than others, but the situation for users on 
shared networks and unencrypted WiFi is more or less the same. DNS is 
still the most easily snoopable and spoofable security/privacy weak 
point for those users.


In any case, the point of this experiment, as described, is to figure 
out how useful/workable DoH would be for users in the wild. If we limit 
the study to certain regions, it's hard to say with any certainty that 
users in other regions wouldn't benefit from it.

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

Re: FYI: Short Nightly Shield Study involving DNS over HTTPs (DoH)

[Kris Maglione]

On Mon, Mar 19, 2018 at 07:27:39PM -0700, Nicholas Alexander wrote:

Hi all,

On Mon, Mar 19, 2018 at 3:56 PM, Xidorn Quan  wrote:


It's fine to embed this experiment in the product, and blog about it, but
it's definitely not fine to have it enabled by default and send every DNS
request to a third-party.

I can understand that the intent must be good, and for better privacy, but
the approach of doing so is not acceptable. Users would think Firefox is
going to just send data to arbitrary third-party without agreement from
them.

As you can see from the replies, almost all people outside the network
team has expressed concerns about this, which should be considered a signal
already that how other technical users may feel about this experiment, and
how technical news would create a title for this.



Let me add my voice as a person outside the network team who can understand
the concerns and _still thinks we should be doing this_.


I'll second this.

I think it's reasonable to be concerned about the public reaction to 
this, but I also think it's worth doing. The end result of this will 
almost certainly be improved privacy and security for users who have 
this enabled by default, and we can't get to that point without doing a 
study like this.


I think it's worth the risk of a backlash. But I also think we should do 
what we can to minimize that backlash.

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

Re: Use cases for invalid-Unicode atoms

[Kris Maglione]

On Mon, Mar 19, 2018 at 08:49:10PM +0200, Henri Sivonen wrote:

It appears that currently we allow atomicizing invalid UTF-16 string,
which are impossible to look up by UTF-8 key and we don't allow
atomicizing invalid UTF-8.

I need to change some things in this area in response to changing
error handling of UTF-8 to UTF-16 XPCOM string conversions to be more
secure, so I want to check if I should change things a bit more.

I can well imagine that the current state is exactly what we want:
Bogosity on the UTF-16 side round-trips and bogus UTF-8 doesn't
normally reach the atom machinery.

Am I correct in assuming we don't want changes here?

(One imaginable change would be replacing invalid sequences in both
UTF-16 and UTF-8 with U+FFFD and then atomicizing the result.)


Leaving aside the question of whether validation is desirable, 
I'd worry about the performance impact. We atomize UTF-16 
strings all over the place in DOM code (and even have a 
main-thread pseudo-hashtable optimization for them). Validation 
might be relatively cheap, but I'd still expect that relative 
cheapness to add up fairly quickly.

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

PSA: Unsafe CPOWs are no longer permitted in mochitests

[Kris Maglione]

For the last few years, mochitests have run in special extension 
scopes, which did things like interpose them with e10s shims and 
allow them to use unsafe CPOWs. As of today, those extension 
scopes no longer exist, and neither the mochitest harness nor 
the tests it runs have access to unsafe CPOWs.


Existing tests that still rely on unsafe CPOWs have been whitelisted 
with the "uses-unsafe-cpows" annotation in their manifest. Please *do 
not* add this annotation (or permitCPOWsInScope calls) to new tests. 
If you add this annotation to any new tests[1], and I find out[2], I 
will disable those tests without notice.


Thanks.

-Kris

[1]: Yes, even if the CPOW use is in existing code that's used by other 
tests. Yes, even if you're copying and modifying an existing test. Yes, 
even if it's so much more convenient.

[2]: And I will.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Chrome-only WebIDL interfaces no longer require DOM peer review

[Kris Maglione]

On Thu, Mar 08, 2018 at 04:41:38PM -0800, Bobby Holley wrote:

On Thu, Mar 8, 2018 at 3:06 PM, Kris Maglione <kmagli...@mozilla.com> wrote:

That said, if we're worried about binary size becoming an issue for
internal interfaces, there are things we can do to reduce the code size of
bindings. Particularly if we're willing to eat the performance costs.


WebIDL bindings are optimized for speed above all else, and that shouldn't
have to change to mitigate overuse.


My point is that if we're deciding that we need to make a trade-off between 
speed and compiled binary size, I think that we're better off doing that by 
changing how we generate the bindings for interfaces that we decide are not 
performance sensitive than deciding to use XPIDL for them. If nothing else, it 
makes it easier for us to make the change for a particular interface, or to 
change our mind.



At any rate, I don't expect us to convert anywhere near all of our XPIDL
interfaces to WebIDL. A lot of them don't need to be exposed to JS at all.
A lot of those should still go away, but they don't need WebIDL bindings,
just concrete native classes. And a lot of the rest are little-enough used
that I can't see anyone spending the effort on converting them.



I am basically worried about two things:
(1) Wholescale conversions of big interfaces in the name of cleanup and
ergonomics. See bug 1341546 for an example.


Heh. It's interesting that you bring that interface up, because I've been 
thinking for a long time that it's one of the most obvious examples of 
something we should convert to WebIDL. We use it all over the place, and it's 
one of the places that I see XPConnect overhead turn up most for in profiles.



I'm not entirely sure whether a review gate is necessary. But at the very
least, I want to establish a consensus around that we should only use
WebIDL to expose internal interfaces if one of the following applies:
(A) The API is likely to be called hundreds of times under normal browser
execution.
(B) The API is associated with a DOM object, and thus adding it
[ChromeOnly] to that interface is particularly convenient.
(C) The API uses complex arguments like promises that XPIDL doesn't handle
in a nice way.

Opinions?


I don't really have a problem with these criteria. That's more or less what I 
consider when deciding how to implement bindings.


But I'd really rather we didn't have to make this trade-off. There's no 
fundamental reason WebIDL bindings have to have more code size overhead than 
XPIDL bindings. The implementation details are almost completely separated 
from the consumers, and if at some point we decide the overhead is becoming a 
problem and we need to make a trade-off, we can always change the 
implementation that we use for interfaces we think are not performance 
critical.



On Thu, Mar 8, 2018 at 3:16 PM, Stuart Philp <sph...@mozilla.com> wrote:


Generally I think we’d take performance and memory wins over installer
size, but we monitor all three and if installer size grows (gradually) by
an uncomfortable amount we ought to make a call on the trade off. We can
bring it to product should that happen.



The problem is precisely that it's gradual - a few kilobytes at a time,
certainly nothing to trigger our alerts. Waiting for it all to pile up and
then launching a herculean effort to move things _back_ to XPIDL would be a
huge waste of time, which is why I'm trying to address the problem now.



On Thu, Mar 8, 2018 at 2:01 PM, Kris Maglione <kmagli...@mozilla.com>

wrote:

It is now possible[1] to create chrome-only WebIDL interfaces in the

dom/chrome-webidl/ directory that do not require review by DOM peers
after
every change. If you maintain an internal performance-sensitive XPIDL
interface, or are considering creating one, I'd encourage you to consider
migrating it to WebIDL.

Some caveats to keep in mind:

- Interfaces in this directory must be annotated with [ChromeOnly].
Dictionaries, however, can be included without any special annotations.

- If you are new to writing WebIDL files, I'd still encourage you to ask
a
DOM peer to review at least your initial check-in.

- Please make sure that you do not attempt to expose any of the interface
or dictionary types defined in these WebIDL files to web contexts,
through
interfaces defined in dom/webidl/. Doing so would require (and fail) DOM
peer review, in any case, but please think ahead.

Thanks.

- Kris

[1]: As of bugs 1443317 and 1442931





--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation

It is practically impossible to teach good programming style to
students that have had prior exposure to Basic; as potential
programmers they are mentally mutilated beyond hope of regeneration.
--Edsger W. Dijkstra

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

Re: PSA: Chrome-only WebIDL interfaces no longer require DOM peer review

[Kris Maglione]

On Thu, Mar 08, 2018 at 03:06:57PM -0800, Kris Maglione wrote:
The amount of WebIDL overhead I regularly see in profiles can 
be staggering.


The amount of *XPConnect* overhead...
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Chrome-only WebIDL interfaces no longer require DOM peer review

[Kris Maglione]

On Thu, Mar 08, 2018 at 02:40:52PM -0800, Bobby Holley wrote:

I've seen a lot of momentum around migrating chrome-only XPIDL interfaces
to WebIDL. I'm concerned that insufficient attention is being paid to the
impact on our binary size.

Fundamentally, the WebIDL bindings improve performance and spec correctness
at the expense of code size (and build times). This makes sense for things
that are web-exposed or performance-sensitive. But since the webidl
bindings are also more modern and easier to use, I'm concerned that people
will use them indiscriminately for all sorts of internal APIs, and our
binary will bloat by a thousand paper cuts.

A WebIDL method binding can easily cost a kilobyte or more, depending on
the number and types of the arguments. If we were to convert all of our
existing xpidl methods, we could approach 10MB in added code size.


I'm not sure how much of an immediate concern this should be.

There are different costs to WebIDL and XPIDL bindings. WebIDL 
bindings have more cost in terms of compiled code size. XPIDL 
have greater costs in terms of performance and runtime memory. 
I'm not sure exactly where the balance is as far as impact to 
package size.


And I think the benefits of WebIDL interfaces apply as much to 
our internal uses as they do to web-exposed interfaces. The 
amount of WebIDL overhead I regularly see in profiles can be 
staggering. And XPIDL has enough foot-guns when interfacing with 
JS that it's easy enough cause confusion and breakage even when 
dealing with internal code.


That said, if we're worried about binary size becoming an issue 
for internal interfaces, there are things we can do to reduce 
the code size of bindings. Particularly if we're willing to eat 
the performance costs.


At any rate, I don't expect us to convert anywhere near all of 
our XPIDL interfaces to WebIDL. A lot of them don't need to be 
exposed to JS at all. A lot of those should still go away, but 
they don't need WebIDL bindings, just concrete native classes. 
And a lot of the rest are little-enough used that I can't see 
anyone spending the effort on converting them.



Gating on DOM peer review gave us some degree of oversight to prevent
overuse. What should replace it?


How much have DOM peers been focusing on preventing over-use, so 
far? Granted, most of the WebIDL bindings I've created to date 
have been to address measurable performance issues, but I've 
never had a reviewer suggest that I should be worried about 
over-use. 


On Thu, Mar 8, 2018 at 2:01 PM, Kris Maglione <kmagli...@mozilla.com> wrote:


It is now possible[1] to create chrome-only WebIDL interfaces in the
dom/chrome-webidl/ directory that do not require review by DOM peers after
every change. If you maintain an internal performance-sensitive XPIDL
interface, or are considering creating one, I'd encourage you to consider
migrating it to WebIDL.

Some caveats to keep in mind:

- Interfaces in this directory must be annotated with [ChromeOnly].
Dictionaries, however, can be included without any special annotations.

- If you are new to writing WebIDL files, I'd still encourage you to ask a
DOM peer to review at least your initial check-in.

- Please make sure that you do not attempt to expose any of the interface
or dictionary types defined in these WebIDL files to web contexts, through
interfaces defined in dom/webidl/. Doing so would require (and fail) DOM
peer review, in any case, but please think ahead.

Thanks.

- Kris

[1]: As of bugs 1443317 and 1442931

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

PSA: Chrome-only WebIDL interfaces no longer require DOM peer review

[Kris Maglione]

It is now possible[1] to create chrome-only WebIDL interfaces in 
the dom/chrome-webidl/ directory that do not require review by 
DOM peers after every change. If you maintain an internal 
performance-sensitive XPIDL interface, or are considering 
creating one, I'd encourage you to consider migrating it to 
WebIDL.


Some caveats to keep in mind:

- Interfaces in this directory must be annotated with 
[ChromeOnly]. Dictionaries, however, can be included without any 
special annotations.


- If you are new to writing WebIDL files, I'd still encourage 
you to ask a DOM peer to review at least your initial check-in.


- Please make sure that you do not attempt to expose any of the 
interface or dictionary types defined in these WebIDL files to 
web contexts, through interfaces defined in dom/webidl/. Doing so 
would require (and fail) DOM peer review, in any case, but 
please think ahead.


Thanks.

- Kris

[1]: As of bugs 1443317 and 1442931
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Prefs overhaul

[Kris Maglione]

On Wed, Mar 07, 2018 at 02:08:59PM +1100, Nicholas Nethercote wrote:

Hi,

I've been doing a lot of work to improve libpref recently, and there is
more in the pipeline. I've written a brief roadmap that might be of
interest to people: https://wiki.mozilla.org/Platform/PrefsOverhaul

I'd be happy to hear feedback, via this list or other means. Thanks.


Related to this (and particularly in the light of bug 1425613), 
I've been thinking it's about time to migrate the preference 
service bindings to WebIDL, and kill the XPIDL/XPCOM bindings 
altogether.


The XPC overhead on the JS side is pretty bad given how much we 
use the service, and a WebIDL implementation would make it 
easier to add certain optimizations (like native lazy pref 
getters) on top of the ones we'd automatically get from the 
conversion.


From the native side, accessing the pref service via 
do_GetService adds a bunch of get service overhead, and then a 
bunch of virtual call overhead for each method call. We do have 
already have a non-virtual API. We should just make that the 
only option.

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