Re: Gecko shutdown (London session)

[Nicolas Silva]

On Thu, Jun 30, 2016, at 04:35 PM, smaug wrote:
> Not sure if this matters here, but cycle collector doesn't really retain
> objects. Sure, we could explicitly clear so called jsholders, so that
> C++->JS 
> edges are manually cut. But in general, CC doesn't know about a cycle
> collectable objects unless the object has been addrefed/released after
> the 
> previous cycle collection.
> (those operations make the object suspected, so the object is put to
> cycle collector's purple buffer).

Yeah, sorry, the wording was inaccurate. Something phrased like this
would make more sense:

All modules should deallocate all or as much as they can of their
short-lived objects during phase 1 so that at the end of phase 1 a cycle
collection is run and all cycles are already broken. If a cycle still
exist at this point, it should be considered a bug.

The problems we currently are not the CC's "fault", it comes from
garbage collected and reference-counted objects being destroyed at
various times during ShutdownXPCOM, which means we have to shut the CC
down (and run the last cycle-collection which will free some more
dynamic objects) at the very end when there isn't much left of gecko
that is in a valid state.

I'll clarify in the document.

Cheers,

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

Gecko shutdown (London session)

[Nicolas Silva]

Hi dev-platform,

We had a session about shutdown problems during the London workweek. I
did a writeup of what was discussed and as it grew into a large-ish
piece, I put it in a wiki page (instead of the email I intended to send
initially) [1].
There's been a lot of work on reducing shutdown crashes/hangs
(especially with e10s) this year, but we still have a lot of work to do
[2]. 

The page contains some discussions about the kinds of issues we ran into
in the gfx team and of how we went about addressing some of them. If you
are working on shutdown problems, you may (or may not) find useful
information in there. If you have some experience with dealing with
shutdown bugs, I encourage you to do a brain dump on that wiki page.

There's also a two-phase shutdown proposal briefly described in there
[3], that is worth discussing because it would involve some cooperation
between different modules.

Cheers,

Nical

[1] https://wiki.mozilla.org/Gecko:Shutdown_issues
[2]
https://crash-stats.mozilla.com/search/?product=Firefox_progress=shutdown&_facets=signature&_columns=date&_columns=signature&_columns=version&_columns=build_id&_columns=platform&_columns=shutdown_progress#facet-signature
[3]
https://wiki.mozilla.org/Gecko:Shutdown_issues#Two-phase_shutdown_proposal

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

Re: Proposal: use nsresult& outparams in constructors to represent failure

[Nicolas Silva]

Your example is suffering from the fact that PR_CreateThread is taking
as parameter an object that is half-initialized, and then continues
constructing it. I believe this to be a poor design and that
unfortunately leaks into the creating of nsThread.

In such a situation I would personally still use a static Create method
which would go something like:

static
already_addRefed Create(...)
{
  nsRefPtr thread = new nsThread(...);
  if (!thread->Init()) {
// the destructor still has to account for failed init but oh
well...
return nullptr;
  }
  // at least no external code will get access to a nsThread that is not
  fully initialized
  return thread.forget();
}

with the constructor and Init() method protected.

It's not as good as not having to run the destructor on a partially
invalid object, but it is way less foot-gun prone for external code,
since they won't see a half-initialized nsThread and maybe forget to
call IsValid(), Init() or whatnot.

We had a lot of code in gfx that was using the constructor then Init() 
and constructor then check patterns, and we ran into bugs related to
using invalid objects. Eventually we converted a lot of that code to the
static Create() style and I am very happy about how simpler and safer
things got as a result.

In my opinion, making it impossible by construction to get a pointer to
a half-initialized or partially invalid object is the important part. If
you can avoid instantiating the object all together in case of failure
it's perfect, if you have to do the instantiation because poor code
forces that on you, it's not perfect, but I think it is still better
than the risk of passing invalid objects around.

Cheers,

Nical

On Thu, Apr 21, 2016, at 05:05 PM, Nicholas Nethercote wrote:
> On Thu, Apr 21, 2016 at 8:41 PM, Martin Thomson  wrote:
> >
> >> - I suspect that in some heap-allocated objects it will be hard to do
> >> the fallible parts without having an object of the relevant type. I
> >> don't have specific examples, just a hunch.
> >
> > I'm not aware of any way that necessary state can't be fully
> > established before constructing an object.  If you find a way in which
> > this is possible, please let me know.
> 
> I just tried this pattern in three classes I've been experimenting
> with and hit problems in all three.
> 
> In bug 1265965 I wrote patches to convert nsThread+Init style to the
> nsThread+outparam style, which worked well. So I tried the style
> Nicolas and you are advocating, which I'll calls
> "falllible-then-infallible". Here is the Init() function, from the
> current trunk code:
> 
> nsresult
> nsThread::Init()
> {
>   // spawn thread and wait until it is fully setup
>   RefPtr startup = new nsThreadStartupEvent();
> 
>   NS_ADDREF_THIS();
> 
>   mShutdownRequired = true;
> 
>   // ThreadFunc is responsible for setting mThread
>   PRThread* thr = PR_CreateThread(PR_USER_THREAD, ThreadFunc, this,
>   PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD,
>   PR_JOINABLE_THREAD, mStackSize);
>   if (!thr) {
> NS_RELEASE_THIS();
> return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>   // ThreadFunc will wait for this event to be run before it tries to
>   access
>   // mThread.  By delaying insertion of this event into the queue, we
>   ensure
>   // that mThread is set properly.
>   {
> MutexAutoLock lock(mLock);
> mEventsRoot.PutEvent(startup, lock); // retain a reference
>   }
> 
>   // Wait for thread to call ThreadManager::SetupCurrentThread, which
>   completes
>   // initialization of ThreadFunc.
>   startup->Wait();
>   return NS_OK;
> }
> 
> So this is the code that must run before we construct an actual
> nsThread object, right? The PR_CreateThread() call is the main
> problem. It takes |this| as a parameter and proceeds to do all sorts
> of stuff with it. It's probably not *impossible* to rewrite this as
> fallible-then-infallible, but I'm struggling to see what it would look
> like.
> 
> The second example I looked at was nsScriptSecurityManager. The
> constructor+outparam style worked well there, but
> fallible-then-infallible again hits a problem -- InitPrefs() is called
> in the fallible section of initialization, and it calls
> ScriptSecurityPrefChanged(), which touches lots of class members.
> Maybe InitPrefs() could be moved out of the fallible section, but I
> don't know this code well enough to say one way or the other.
> 
> The third example I looked at was CycleCollectedJSRuntime. Again
> problems, specifically this line in Init():
> 
>   mOwningThread->SetScriptObserver(this);
> 
> So, for all three examples I looked at fallible-then-infallible had
> problems ranging from moderate to severe. My conclusion is that
> partially initializing an object when the object doesn't exist is
> difficult, and, as a result, fallible-then-infallible is not a
> reliable general-purpose pattern.
> 
> Nick
___

Re: Proposal: use nsresult& outparams in constructors to represent failure

[Nicolas Silva]

My experience is that stack allocated objects tend to be simpler,
shorter-lived and less prone to accumulating invalid state somewhere and
crash somewhere else. All in all they haven't been a source of pain the
way heap-allocated objects often are when it comes to invalid state
creeping up. There tend to be fewer of them also. So I am perfectly
happy with handling them differently, and I can't think of a stack
allocated object in the code I work with that would need something as
involved as a static creation method or an nsresult& out param, to be
honest.

Cheers,

Nical 

On Thu, Apr 21, 2016, at 12:24 PM, Nicholas Nethercote wrote:
> On Thu, Apr 21, 2016 at 7:38 PM, Nicolas Silva <nical.si...@gmail.com>
> wrote:
> > Fallible construction (even with a way to report failure) is annoying if
> > only because the object's destructor has to account for the possible
> > invalid states. I much prefer having a static creation method that will
> > only instantiate the object in case of success, and mark the constructor
> > protected. Something like:
> >
> > static
> > already_AddRefed Foo::Create(SomeParams...) {
> > // logic that may fail...
> > if (failed) {
> >  return nullptr;
> > }
> > return MakeAndAddRef(stuffThatDidNotFail);
> > }
> 
> So instead of doing the infallible part in the constructor and the
> fallible part in the Init() function, you're suggesting doing the
> fallible part first and only calling the constructor once it succeeds?
> Interesting. I can see it would work nicely for some heap-allocated
> objects. But I see two possible problems:
> 
> - It doesn't appear to work with stack-allocated objects?
> 
> - I suspect that in some heap-allocated objects it will be hard to do
> the fallible parts without having an object of the relevant type. I
> don't have specific examples, just a hunch.
> 
> Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: use nsresult& outparams in constructors to represent failure

[Nicolas Silva]

Fallible construction (even with a way to report failure) is annoying if
only because the object's destructor has to account for the possible
invalid states. I much prefer having a static creation method that will
only instantiate the object in case of success, and mark the constructor
protected. Something like:

static
already_AddRefed Foo::Create(SomeParams...) {
// logic that may fail...
if (failed) {
 return nullptr;
}
return MakeAndAddRef(stuffThatDidNotFail);
}

Between having ways to communicate that an object is constructed in an
invalid state, and never having invalid states, I much prefer the
latter.

Cheers,

Nical

On Thu, Apr 21, 2016, at 07:05 AM, Gerald Squelart wrote:
> On Thursday, April 21, 2016 at 11:15:10 AM UTC+10, Nicholas Nethercote
> wrote:
> > Hi,
> > 
> > C++ constructors can't be made fallible without using exceptions. As a 
> > result,
> > for many classes we have a constructor and a fallible Init() method which 
> > must
> > be called immediately after construction.
> > 
> > Except... there is one way to make constructors fallible: use an |nsresult&
> > aRv| outparam to communicate possible failure. I propose that we start doing
> > this.
> > 
> > Here's an example showing stack allocation and heap allocation. Currently, 
> > we
> > do this (boolean return type):
> > 
> >   T ts();
> >   if (!ts.Init()) {
> > return NS_ERROR_FAILURE;
> >   }
> >   T* th = new T();
> >   if (!th.Init()) {
> > delete th;
> > return NS_ERROR_FAILURE;
> >   }
> > 
> > or this (nsresult return type):
> > 
> >   T ts();
> >   nsresult rv = ts.Init();
> >   if (NS_FAILED(rv)) {
> > return rv;
> >   }
> >   T* th = new T();
> >   rv = th.Init();
> >   if (NS_FAILED(rv)) {
> > delete th;
> > return rv;
> >   }
> > 
> > (In all the examples you could use a smart pointer to avoid the explicit
> > |delete|. This doesn't affect my argument in any way.)
> > 
> > Instead, we would do this:
> > 
> >   nsresult rv;
> >   T ts(rv);
> >   if (NS_FAILED(rv)) {
> > return rv;
> >   }
> >   T* th = new T(rv);
> >   if (NS_FAILED(rv)) {
> > delete th;
> > return rv;
> >   }
> > 
> > For constructors with additional argument, I propose that the |nsresult&|
> > argument go last.
> > 
> > Using a bool outparam would be possible some of the time, but I suggest 
> > always
> > using nsresult for consistency, esp. given that using bool here would be no
> > more concise.
> > 
> > SpiderMonkey is different because (a) its |operator new| is fallible and 
> > (b) it
> > doesn't use nsresult. So for heap-allocated objects we *would* use bool, 
> > going
> > from this:
> > 
> >   T* th = new T();
> >   if (!th) {
> > return false;
> >   }
> >   if (!th.Init()) {
> > delete th;
> > return false;
> >   }
> > 
> > to this:
> > 
> >   bool ok;
> >   T* th = new T(ok);
> >   if (!th || !ok) {
> > delete th;
> > return false;
> >   }
> > 
> > These examples don't show inheritance, but this proposal works out
> > straightforwardly in that case.
> > 
> > The advantages of this proposal are as follows.
> > 
> > - Construction is atomic. It's not artificially split into two, and there's 
> > no
> >   creation of half-initialized objects. This tends to make the code nicer
> >   overall.
> > 
> > - Constructors are special because they have initializer lists -- there are
> >   things you can do in initializer lists that you cannot do in normal
> >   functions. In particular, using an Init() function prevents you from using
> >   references and |const| for some members. This is bad because references 
> > and
> >   |const| are good things that can make code more reliable.
> > 
> > - There are fewer things to forget at call sites. With our current approach 
> > you
> >   can forget (a) to call Init(), and (b) to check the result of
> > Init(). With this
> >   proposal you can only forget to check |rv|.
> > 
> > The only disadvantage I can see is that it looks a bit strange at first. 
> > But if
> > we started using it that objection would quickly go away.
> > 
> > I have some example patches that show what this code pattern looks like in
> > practice. See bug 1265626 parts 1 and 4, and bug 1265965 part 1.
> > 
> > Thoughts?
> > 
> > Nick
> 
> (busy right now, please excuse terseness & typos!)
> 
> Big thumbs up for trying to remove split construction
> 
> My main beef with this proposal is the use of out-params, which require
> (usually uninitialized) declaration of the out-param.
> But I see that it may indeed be the best solution here, so ... fne!
> 
> However, since lots of Mozilla objects are unique-ptr'd or ref-counted, I
> would argue that we could easily fold the construction checks with the
> nullptr-checks that we all know!
> 
> So in addition to your proposal, I would like to see a small library of
> tools that will build on top of your new style, and make it easier &
> cleaner & consistent to use in those cases.
> 
> E.g.:
>   template 
>   T* 

Re: Intent to deprecate: MacOS 10.6-10.8 support

[Nicolas Silva]

Re-re-ping.
Being able to use a more recent standard library would simplify a lot of
things on our end. For example updating 3rd party libraries such as
skia, which depends on a modern stl, is a pain, and there are plenty of
other examples.

Cheers,

Nical

On Mon, Apr 4, 2016, at 07:33 PM, a...@imgland.xyz wrote:
> Doesn't hombrew provide a version of g++ that includes c++11
> 
> 04.04.2016, 18:12, "Nathan Froyd" :
> > Re-ping on this thread. It would be really useful to have a decision
> > one way or the other for figuring out exactly how a C++11 STL on OS X
> > is going to work.
> >
> > -Nathan
> >
> > On Thu, Mar 24, 2016 at 12:51 PM, Ralph Giles  wrote:
> >>  Discussion seems to have wound down. Is there a decision on this?
> >>
> >>   -r
> >>  ___
> >>  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
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Skia Content on OS X

[Nicolas Silva]

I would like to add to what Mason already said about GPU painting. If
one thing is clear, it's that the GPU (as in the many combinations of
hardware and drivers out there) is not a reliable platform, especially
with gecko's current architecture. We get driver bugs for the simplest
things we do and they hit an awful lot of users. We need a solid
baseline that works reliably across the majority of hardware (CPU
drawing), on top of which we can add faster GPU-based backends that gets
enabed when we can detect that things will actually work.

I do think that the GPU is what can provide the best performance in most
cases (when it works), especially as screen resolution goes up, and the
work on WebRender is a good illustration of this. I am hoping that
something similar will happen in gecko eventually, but if it does, I am
certain that it will never fully replace a reliable cpu rendering path.

Anyway, moving to skia is about reducing the amount of code we need to
maintain, and we picked skia rather than, say, cairo because we believe
it is the best choice we have with this kind of immediate mode drawing
APIs.

Cheers,

Nical

On Tue, Mar 22, 2016, at 08:06 PM, Patrick Walton wrote:
> Thanks for the info! This is very interesting to us in Servo.
> 
> Do you have any performance measurements that support the statement
> "While
> it would be nice to use the GPU, we’re not totally confident that using
> the
> GPU to render content is quite worth it."? We have evidence on our side
> to
> support this too (and in fact it's one of the motivations for WebRender),
> but it's be great to have comprehensive data to back it up.
> 
> Patrick
> On Mar 22, 2016 10:56 AM, "Mason Chang"  wrote:
> 
> > Hi David,
> >
> > The main benefit is architectural with a huge simplification of our
> > graphics code, with a nice side benefit of performance. As it stands today,
> > we have multiple different rendering paths for each platform (Linux, OS X,
> > Windows). That means every time we hit a graphics bug, we have to know
> > three APIs that render just slightly different and we have to dig through
> > each backend to figure out what’s going on. These all have slightly
> > different performance characteristics as well, which takes some time. We
> > don’t generally get a lot of benefit from having a distinct backend for
> > platform also because other than on Windows, we’re not using the GPU for
> > content rendering. This essentially means we’re spinning a lot of wheels
> > and cost on small fixes on non-windows platforms for little benefit. [1].
> > This is especially true when you consider that we have special backends for
> > both Linux and OS X which make up 2.6% of our user base.
> >
> > With Skia, we can unify all of our graphics pipeline across all platforms
> > other than Windows for content rendering. It uses CPU rendering and because
> > of that, fixes on one platform translate much better across other platforms
> > that use Skia. While it would be nice to use the GPU, we’re not totally
> > confident that using the GPU to render content is quite worth it. It’s also
> > quite nice that micro-level optimizations at the backend level can mostly
> > be done for us as Skia is optimizing performance with Chrome as one of
> > their use cases. e.g., on Mac, scaling content on the CPU was 2-3x faster
> > with Skia versus CG.
> >
> > Overall, it’s mostly a clean up operation so we spend less time fixing
> > each individual platform.
> >
> > Please let me know if you have any more questions.
> >
> > Thanks,
> > Mason
> >
> > [1] https://people.mozilla.org/~danderson/moz-gfx-telemetry/www/
> >
> > > On Mar 22, 2016, at 8:21 AM, Mike de Boer  wrote:
> > >
> > > I was also quite curious, so I started clicking up the hierarchy of that
> > bug and ended up at:
> > >
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1154825#c1 <
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1154825#c1>
> > >
> > > (comment 2 is also useful info)
> > >
> > > Ultimate goal: no more checkerboarding in APZ. But Mason is the
> > authority here, methinks :-)
> > >
> > > Cheers,
> > >
> > > Mike.
> > >
> > >> On 22 Mar 2016, at 15:45, David Rajchenbach-Teller 
> > wrote:
> > >>
> > >> Out of curiosity, what's the benefit?
> > >>
> > >> On 22/03/16 15:44, Mason Chang wrote:
> > >>> Hi all,
> > >>>
> > >>> We're changing the default graphics backend on OS X from CoreGraphics
> > to
> > >>> Skia. In the best case, you should notice no difference.
> > >>>
> > >>> If you see any graphics artifacts on mac, please file a bug and make it
> > >>> block bug 1207332 <
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1207332>.
> > >>> You can verify that the artifact you're seeing is due to the switch by
> > >>> going to about:config, set the preference "gfx.content.azure.backends"
> > to
> > >>> "cg", restarting Firefox and seeing if the artifact still appears.
> > >>>
> > >>> Thanks to Lee Salzman, Jeff 

Re: From nsIntSize to gfx::IntSize

[Nicolas Silva]

The first part just landed on inbound (Bug 1132854).

On Fri, Mar 27, 2015 at 10:28 PM, Robert O'Callahan rob...@ocallahan.org
wrote:

 Sounds good. But, is gfx::IntSize going to get a ToAppUnits method like
 nsIntSize has?


The method:
nsIntSize::ToAppUnits(nscoord aAppUnitsPerPixel) const

was replaced by a function:
IntSizeToAppUnits(mozilla::gfx::IntSize aSize, nscoord aAppUnitsPerPixel)


As a followup it's probably worth replacing all of nsIntSize with
 gfx::IntSize.


So no more nsIntSize and gfxIntSize typedefs anywhere (I guess layout is
the most impacted non-gfx module)? Sounds go to me, I felt like it wasn't
my decision to make, but I do prefer having ony gfx::IntSize in the entire
gecko code base.

Cheers,

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

Re: From nsIntSize to gfx::IntSize

[Nicolas Silva]

On Fri, Mar 27, 2015 at 10:50 PM, Jet Villegas jville...@mozilla.com
wrote:

 Probably safe for the integer types, but can we add strong assertions when
 converting from Thebes and Moz2D floats? Bugs like this one are tough to
 debug:

 https://bugzilla.mozilla.org/show_bug.cgi?id=1091709

 Thanks!



I haven't planned to do an all-at-once conversion of Moz2D floats,
especially because we ran into bugs like the one you mentioned.
For now I am doing the integer types because they are easy and constantly
getting in my way, but I don't think that I'll look into the float ones in
the short term.

Cheers,

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

From nsIntSize to gfx::IntSize

[Nicolas Silva]

As many of you know, the introduction of Moz2D a while ago added new size,
point and rect classes which are equivalent* to the ones that already
existed in tree (nsIntSize, etc.).

Juggling back and forth between the Moz2D classes and their thebes
equivalent is pretty annoying and until now we have been progressively
replacing uses of thebes classes by Moz2D ones (a slow process bearing the
exquisite name Moz2Dification). Moz2Dification of size classes hasn't
been very efficient, so I decided to just remove the integer thebes
size/rect/point classes and make then typedefs of the moz2D ones.

I will soon (probably over the weekend) land a patch set that does this for
nsIntSize/gfx::IntSize.

What this means if you are writing code outside of the gfx/ directory: Not
much. Apologies if my patch queue causes some merge conflicts.
It's up to module owners to decide if they still want refer to integer
sizes as nsIntSize (the typedef) or as gfx::IntSize (the real thing!). You
won't need to use the conversion helpers ToIntSize and ThebesIntSize
anymore, since nsIntSize and gfx::IntSize will be one and the same, and by
the way those conversion helpers are being removed which is most likely the
reason if you run into merge conflicts.

If you write code under the gfx/ directory, please use gfx::IntSize. After
this lands there won't be any reason to refer to it as nsIntSize, so let's
make things consistent.

nsIntRect and nsIntPoint will soon share the same fate.

Cheers,

Nical

* This only true for integer classes (IntSize, ect.) because the
non-integer ones use double precision floats in thebes and regular floats
in Moz2D, so we can't just make the switch in one go as easily.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Off-main-thread compositing on Linux

[Nicolas Silva]

Tiling is in a usable state (modulo some reftest failures), but I
haven't tried to run talos with tiling enabled yet. We'll probably see
the benefit of tiling when we enable APZ (which I don't know the state
of on Linux). We can also enable OMTA but I haven't tried to run tests
with it or dogfood it in a while.

Nical


On Wed, Mar 11, 2015 at 5:02 PM, Jet Villegas jville...@mozilla.com wrote:

 Nice! This is big news. WIth Linux getting OMTC, we can also enable
 off-main-thread animations (OMTA) on all our supported platforms.

 Is tiling + OMTC on Linux in a testable state? How do we score on the
 scrolling performance test in Talos?

 --Jet


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

Off-main-thread compositing on Linux

[Nicolas Silva]

Hi all,

I am giving the heads up that Off-main-thread compositing (OMTC) will soon
be enabled on Linux and that there are some expected talos regressions.
In particular, a fairly big one in tp5o_scroll (see the compare-talos link
below).

http://perf.snarkfest.net/compare-talos/?oldRevs=56492f7244a9newRev=f8fbbd03280csubmit=true

Enabling OMTC on Linux is very important because it is the foundation on
top of which we will be able to use the optimizations and features that we
have on the other platforms, such as tiled rendering, async scrolling and
video playback, and also because e10s requires it.
The benefits these features will bring outweigh the initial regressions,
and I am confident we'll win back what we have to loose initially along
with implementation of the followup projects.

If you are interested in the details, we believe that the biggest causes of
regressions are:
1) Synchronous communication with the compositor thread. This will be
addressed by tiling which we already ship on mobile platforms and on Mac,
and that will come to Windows and Linux.
2) Cairo's xrender backend also contributes to the regressions, but there
is unfortunately not much we can do in the short term because getting rid
of xrender has a fairly involved dependency chain (cf. replacing gtk2 by
gtk3).

e10s users will not be impacted since e10s enables OMTC automatically, so I
assume a large part of the nightly population will not experience any
difference.

If you find issues related to omtc on linux, please file bugs against the
Graphics: Layers component and cc me (:nical).

Cheers,

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

Re: [RFC] Linux HiDPI support: choose default scale factor based on DPI

[Nicolas Silva]

In the mean time we should really detect hidpi screens and default to an
appropriate scale.
The current scale is a terrible experience on a hidpi screen. I filed bug
1081142.

Cheers,

Nical

On Fri, Oct 10, 2014 at 8:39 AM, Mike Hommey m...@glandium.org wrote:

 On Thu, Oct 09, 2014 at 11:18:28PM -0700, Robert O'Callahan wrote:
  On Thu, Oct 9, 2014 at 10:31 PM, giuseppe.bilo...@gmail.com wrote:
 
   I recently got a HiDPI monitor (15 at 3200x1800, 234 dpi) and I
 noticed
   that Firefox (32) doesn't automatically scale UI/content to take
 advantage
   of the hidpi, with the result that stuff renders uncomfortably small by
   default. If I manually set devPixelsPerPx to 2, I seem to get a
 reasonable
   UI and content scaling.
  
   I've perused the source code and did a little bit of debugging, and
 while
   the DPI is detected correctly (nsWindow::GetDPI returns 234), no
 scaling
   factor is set from this. I've tracked this to the fact that nsWindow in
   Linux does not reimplement GetDefaultScaleInternal().
  
   Since the patch to implement this is rather trivial, I'm attaching a
   preliminary version to this post. Still, I was wondering: was the
 omission
   intentional (e.g. because other means should be used on Linux for
 this, and
   I just happen to have a set-up where this is not detected)?
  
 
  We should really follow what GTK does. E.g. whatever scale factor GTK
 uses
  for system/application icons by default, we should match it. Icons in the
  Firefox UI should use the same scaling as icons in native/system apps.

 But GTK+2 doesn't have support for HiDPI does it? Only recent GTK+3 has,
 and that's covered by bug 975919 (but, obviously, that's limited to
 GTK+3 builds, which are far from being production ready).

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

using namespace

[Nicolas Silva]

Now that we have unified builds, writing using namespace in the global
scope of a .cpp file is almost as bad as writing it in a header. Regularly
build errors show up like this one:
https://tbpl.mozilla.org/php/getParsedLog.php?id=40010766tree=Try

What's awesome about these errors is that they typically show up on some
platform but not the others and they come and go as we add files or do
things that can alter the generation of the unified .cpp files.

So, I strongly encourage everyone to stop using using namespace.
If you really really want to use them, please put them inside the scope of
a namespace. For instance a .cpp file in the gfx/layers directory typically
looks like this:

// using namespace gfx; here means nical: r-
namespace mozilla {
namespace layers {

// using namespace gfx; here means nical is grumpy but he'll probably not
block your super important path for this.

// ...

}
}

Beware that once using namespace is placed in an enclosing namespace foo,
it will affect the namespace foo the next time you open it:

namespace foo {
using namespace unicorns;
// ...
}

namespace foo {
// here it's just like you wrote using namespace unicorns; again
}

Which leads to problems with namespaces like mozilla::ipc and
mozilla::dom::ipc being ambiguous in the mozilla namespace. it is probably
just not possible to write using namespace mozilla::dom; in most of the
layers code for this reason.

Honestly, I don't mean to start a bikeshed about whether we should enforce
strict rules about this project-wise, because I know that people have
different tastes as to whether writing Size is vastly better than
gfx::Size. But I'd like to encourage people to carefully make use of this
c++ feature, especially when the namespace in question is used a handful of
times in the file. I personally will be strict about it the reviews I give.

Cheers,

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

Re: [e10s] Changes to the browser.tabs.remote preference in desktop Firefox

[Nicolas Silva]

We are working on getting OMTC to perform well on Windows. D3D11 OMTC may
not be too far off, D3D9 will have to wait for tiling to be ready though.
On Linux there are a few glitches here and there and If we want to ship it
we'll have to investigate performance and probably implement texture from
pixmap but this is not staffed so it's a good place for external
contributions. I haven't been following closely the state of the software
backend for each platform but there's some activity on bugzilla.
In general I think we have a problem when resizing windows where the
compositor is one frame later than the window manager which doesn't look
good (Linux, Windows).

Cheers,

Nical


On Fri, Feb 14, 2014 at 10:04 PM, Leman Bennett (Omega X) 
Redacted.For.Spam@request.contact wrote:

 On 2/13/2014 7:33 PM, Bill McCloskey wrote:

 Hi everyone,


 I just wanted to make a quick announcement about preference changes for
 out-of-process tabs. Bug 960783, which landed recently, added a New OOP
 Window menu option to open a new window with out-of-process tabs. Right
 now this option is only enabled on Macs because it requires OMTC, but it
 will move to other platforms as they get OMTC. It's also restricted to
 nightly.

 This change required some reinterpretation of the existing
 browser.tabs.remote preference. We use this pref in a fair number of
 places, so I want to make sure that everyone understands how it works now.
 This is the new setup:

 If browser.tabs.remote = false, then remote (i.e., out of process) tabs
 are completely inaccessible. This is how we'll keep this feature disabled
 in non-nightly builds.

 If browser.tabs.remote = true and browser.tabs.remote.autostart = false,
 then browser windows will normally be in-process. However, the user will
 see a new menu option, New OOP Window that will open a window with remote
 tabs. This configuration is now the default in nightly on Macs. For
 non-OMTC platforms, browser.tabs.remote=false will remain the default.

 If browser.tabs.remote = true and browser.tabs.remote.autostart = true,
 then browser windows will normally have remote tabs. However, the user will
 see a new menu option, New In-process Window that will open a window with
 in-process tabs. This configuration is for people who want to test e10s
 more extensively.

 We're hoping that exposing the New OOP Window menu item will make it
 easier for people to test electrolysis.

 -Bill



 What is the status of OMTC for other platforms? It seems that the movement
 to get this to other platforms has stalled.


 --
 ==
 ~Omega X
 MozillaZine Nightly Tester

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

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

Re: Layer Construction for Off Main Thread canvas

[Nicolas Silva]

With OMTC these days the Layer classes don't hold any logic. All the fancy
stuff goes into classes inheriting from CompositableClient (and
CompositableHost on the compositor side). While Layer classes can only be
manipulated from the main thread and use the PLayerTransaction IPDL
protocol to sync with the compositor, Compositable classes can be used by
either the PLayerTransaction or the PImageBridge protocol. The logic to
attach a Compositable using PImageBridge to a Layer using PLayerTransaction
is rather simple (look at ImageClientBridge and ImageContainer) and mostly
implemented in the CompositableClient/Host abstraction so you don't have to
redo most of it.

As Nick said you could have on the main thread a dummy CanvasClient
analogous to ImageClientBridge which just forward an asyncID which is the
ID used to connect the layer and the compositable on the compositor side,
and on the ImageBridge thread you would have the usual CanvasClient. Then
you'd need the equivalent of ImageContainer that would interface with the
WebGLContext and forward frames to the ImageClient on the ImageBridge
thread.

This is the way we do with async video, which works well.

This email is probably a bit confusing if you haven't looked at the
Compositable stuff yet, so don't hesitate to ping me and ask questions
about the compositor's IPC stuff and async updates.

Cheers,

Nical



On Thu, Oct 31, 2013 at 7:03 AM, Kyle Huey m...@kylehuey.com wrote:

 On Thu, Oct 31, 2013 at 12:36 PM, Robert O'Callahan rob...@ocallahan.org
 wrote:

  On Thu, Oct 31, 2013 at 5:28 PM, Kyle Huey m...@kylehuey.com wrote:
 
  One thing that's come up that we're not quite how to deal with for
  OMTcanvas is
  how to modify GetCanvasLayer.  Our problem here is that the context here
  lives on the worker thread, and presumably we need to construct the
 layer
  on the main thread, but creating that layer requires data that also
 lives
  on the worker thread.
 
  We obviously can't block the main thread on a synchronous call to the
  worker, and we can't just lock around the values because that will break
  run to completion semantics on the worker (e.g. if the worker never
 yields,
  changes in the canvas size shouldn't take effect).
 
  I think the right thing to do is probably to ship the values from the
  worker to the main thread asynchronously as they are updated on the
 worker
  (once the worker yields/etc) and create the layer using those values.
  How
  often do we create layers?  It would be a shame if rendering straight
 from
  the worker to the compositor were blocked by the main thread due to
 layer
  creation.
 
  Anything else we should be thinking about here?
 
 
  What values do you need from the worker in order to create the layer?
 

 Well, at the very least we need the dimensions of the canvas buffer.  We
 need to know if the context is lost too, but that can already happen
 anytime so that's not too bad.  The dimensions live on the worker thread
 (which can update them) so we need to either lock and read them from the
 main thread or maintain a separate copy of the dimensions on the main
 thread and post messages to update them.  If we do the first then it's
 possible for the dimensions the compositor sees to be newer than the last
 data we pushed to it.  If we do the latter then the opposite is possible.
 Does the compositor care if the layer size and the data we push to it are
 not in sync?

 The CanvasLayer also wants the GLContext to do stuff with it.  Not sure
 what we're going to do with that ...


  It seems to me that we should be able to create a CanvasLayer on the main
  thread that is sized to the canvas element. Then from the CanvasLayer we
  get some kind of thread-safe object (analogous to the ImageContainer of
 an
  ImageLayer) which can be handed to the worker in the WorkerCanvas. This
  object would support being one end of the surface stream for WebGL. You
  would feed a series of surfaces into it which could have different sizes
  and the compositor will size them to the layer.
 

 That sounds reasonable ... it seems like just getting everything set up is
 the hard part.

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

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

Re: unified shader for layer rendering

[Nicolas Silva]

I do appreciate the fact that it reduces complexity (in addition to less
state changes).

I agree that the decision of dedicating resources on that rather than on
other high priority projects that are in the pipes should be motivated by
some numbers.

Cheers,

Nical




On Thu, Oct 10, 2013 at 11:04 AM, Benoit Jacob jacob.benoi...@gmail.comwrote:

 2013/10/10 Benoit Jacob jacob.benoi...@gmail.com

  I'll pile on what Benoit G said --- this is the kind of work that would
  require very careful performance measurements before we commit to it.
 
  Also, like Benoit said, we have seen no indication that glUseProgram is
  hurting us. General GPU wisdom is that switching programs is not per se
  expensive as long as one is not relinking them, and besides the general
  performance caveat with any state change, forcing to split drawing into
  multiple draw-calls, which also applies to updating uniforms, so we're
 not
  escaping it here.
 
  In addition to that, not all GPUs have real branching. My Sandy Bridge
  Intel chipset has real branching, but older Intel integrated GPUs don't,
  and I'd be very surprised if all of the mobile GPUs we're currently
  supporting did. To put this in perspective, in the world of discrete
  desktop NVIDIA GPUs, this was only introduced in the Geforce 6 series.
 

 In fact, even on a Geforce 6, we only get full real CPU-like (MIMD)
 branching in vertex shaders, not in fragment shaders.

 http://http.developer.nvidia.com/GPUGems2/gpugems2_chapter34.html

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

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

Re: partial GL buffer swap

[Nicolas Silva]

From an API/feature point of view the partial buffer swap does not sound
like a bad idea, especially since, as Mat said, the OMTC BasicLayers will
need something along these lines to work efficiently.
One thing to watch out for, though, is that it is the kind of fine tuning
that, I suspect, will give very different results depending on the
hardware. On tile based GPUs, doing this without well working extensions
like QCOM_tiled_rendering will most likely yield bad performances, for
example. More importantly I am not sure how much we can rely on these
extensions reliably behaving across the different hardware.
Would we use something like WebGL's blacklisting for this optimization?
I heard that our WebGL blacklisting is a bit of a mess.

Are these the lowest hanging fruits to improve performances?




On Sun, Sep 1, 2013 at 6:53 AM, Matt Woodrow m...@mozilla.com wrote:

 We actually have code that does the computation of the dirty area
 already, see

 http://mxr.mozilla.org/mozilla-central/ident?i=LayerPropertiestree=mozilla-central
 .

 The idea is that we take a snapshot of the layer tree before we update
 it, and then do a comparison after we've finished updating it.

 We're currently only using this for main-thread BasicLayers, but we're
 almost certainly going to need to extend it to work on the compositor
 side too for OMTC BasicLayers.

 It shouldn't be too much work, we just need to make ThebesLayers shadow
 their invalid region, and update some of the LayerProperties comparison
 code to understand the **LayerComposite way of doing things.

 Once we have that, adding compositor specific implementations of
 restricting composition to that area should be easy!

 - Matt

 On 1/09/13 4:50 AM, Andreas Gal wrote:
 
  Soon we will be using GL (and its Windows equivalent) on most
  platforms to implement a hardware accelerated compositor. We draw into
  a back buffer and with up to 60hz we perform a buffer swap to display
  the back buffer and make the front buffer the new back buffer (double
  buffering). As a result, we have to recomposite the entire window with
  up to 60hz, even if we are only animating a single pixel.
 
  On desktop, this is merely bad for battery life. On mobile, this can
  genuinely hit hardware limits and we won't hit 60 fps because we waste
  a lot of time recompositing pixels that don't change, sucking up
  memory bandwidth.
 
  Most platforms support some way to only update a partial rect of the
  frame buffer (AGL_SWAP_RECT on Mac, eglPostSubBufferNVfor Linux,
  setUpdateRect for Gonk/JB).
 
  I would like to add a protocol to layers to indicate that the layer
  has changed since the last composition (or not). I propose the
  following API:
 
  void ClearDamage(); // called by the compositor after the buffer swap
  void NotifyDamage(Rect); // called for every update to the layer, in
  window coordinate space (is that a good choice?)
 
  I am using Damage here to avoid overloading Invalidate. Bike shedding
  welcome. I would put these directly on Layer. When a color layer
  changes, we damage the whole layer. Thebes layers receive damage as
  the underlying buffer is updated.
 
  The compositor accumulates damage rects during composition and then
  does a buffer swap of that rect only, if supported by the driver.
 
  Damage rects could also be used to shrink the scissor rect when
  drawing the layer. I am not sure yet whether its easily doable to take
  advantage of this, but we can try as a follow-up patch.
 
  Feedback very welcome.
 
  Thanks,
 
  Andreas
 
  PS: Does anyone know how this works on Windows?
  ___
  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

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

Re: C++ style question: virtual annotations on methods

[Nicolas Silva]

FWIW, I like to write both virtual and MOZ_OVERRIDE.
I care a lot about always using MOZ_OVERRIDE when applicable. For virtual
(when MOZ_OVERRIDE is present) I suppose it is more of a matter of tastes.
Always explicitly writing both virtual and MOZ_OVERRIDE is a simpler rule
than marking virtual only in the cases where needed.
Plus, virtual is highlighted in our editors and people often (at least I)
get used to looking for the answer to is it virtual? by glancing at the
beginning of the line in the declaration. So I find always explicitly
writing virtual to be easier to read.

Cheers,

Nical


On Wed, Sep 4, 2013 at 6:45 AM, Chris Pearce cpea...@mozilla.com wrote:

 On 04-Sep-13 4:18 PM, Robert O'Callahan wrote:

 I like to put virtual on all methods that are virtual, even when it's
 not
 strictly necessary because the method overrides a virtual method  of the
 parent class.

 Other people disagree, especially when the method has MOZ_OVERRIDE on it
 as
 well.


 virtual on non-overrides, MOZ_OVERRIDE on overrides. MOZ_OVERRIDE
 implies virtual, you get a compile error when you put MOZ_OVERRIDE on a non
 virtual, so it enforces that you're doing what you think you're doing, so
 we should use it.


 Chris P.


 __**_
 dev-platform mailing list
 dev-platform@lists.mozilla.org
 https://lists.mozilla.org/**listinfo/dev-platformhttps://lists.mozilla.org/listinfo/dev-platform

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

Re: Reproducing bug 837489

[Nicolas Silva]

On Sat, Aug 24, 2013 at 9:19 PM, garys...@gmail.com wrote:

 I can't reliably reproduce the problem,  it just seems to happen  Most of
 my corruption is on the chrome.  It gets completely or partially overlayed
 with blocks of color.  Of interest might be the fact that the color is
 whatever color I set in Windows 8 for the title bar.

 Content corruption is a more rare for me.  I've seen black squares and
 various geometric shapes of different colors. This is not a frequent
 occurrence.

 In most cases the chrome corruption can be cleared up by minimizing or
 maximizing the screen.  Content corruption can be gotten rid of my
 scrolling past the corruption.

 Turning off Azure 'cures' the problem for me.  If there is any way I can
 help you nail this down let me know.  Perhaps when it does happen I can
 produce some sort of report for you.



Maybe Bas's Azure recorder could help then?



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

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

Re: Sandboxed, off-screen pages for thumbnail capture

[Nicolas Silva]

I feel somewhat uneasy about the idea that thumbnails generate more network
traffic. It would be great to at least throttle that when connectivity is
bad, or when the the user's data plan bill could suffer from it (not sure
how to detect/address something like that). If nothing else, users should
be able to disable this feature.

Also it's worth noting that thumbnails also have the problem of when it
should be taken. As soon as web pages become applications rather than
simple documents, we tend to screenshot before most of the content is on
the screen. Maybe it's worth thinking about how potential solutions to both
problems interact.

Cheers,

Nical



On Tue, Jun 18, 2013 at 6:48 AM, Drew Willcoxon a...@mozilla.com wrote:

 The desktop Firefox team is building a new Toolkit module that captures
 thumbnails of off-screen web pages. Critically, we want to avoid capturing
 any data in these thumbnails that could identify the user. More generally,
 we're looking for a way to visit pages in a sandboxed manner that does not
 interact with the user's normal browsing session. Does anyone know of such
 a way or know how we might change Gecko to support something like that?

 To provide context, I'll describe the original problem that motivated the
 new thumbnail module, the poor way in which the new module interacts with
 the user's browsing state, and then some things we think we need in order
 to sandbox pages. See bug 870179 for motivation for this post.

 The Original Problem

 Toolkit already has a thumbnail module, [PageThumbs], but it can only
 capture thumbnails of open content windows, same as they appear to the
 user. Windows may contain sensitive data that should not be recorded in an
 image, however, like bank account numbers and so on, so Firefox uses some
 [heuristics] to determine when it's safe to capture a given window. If it's
 not safe, then Firefox makes no further attempt to capture the window's
 page until the user happens to visit it again. If it's never safe to
 capture any visit to the page, then the page never has a thumbnail. This is
 why you might end up with lots of blank thumbnails in Firefox's
 about:newtab page. (One of the most notable heuristics is the presence of
 the header Cache-Control: [no-cache]. Firefox treats it as an indication
 that a page may contain sensitive data and therefore should not be
 captured.)

 Our Solution

 We wrote a [new module] that's not limited to capturing thumbnails of open
 windows. It loads pages on demand in a xul:browser in the hidden window and
 then captures them. This browser is remote so that its page loads don't
 block the main thread (to the extent that page loads in remote browsers
 don't block the main thread). Further, the browser uses private browsing
 mode to sandbox its pages from the user's normal browsing session. If
 you're logged in to a site in a main window, your logged-in status is not
 reflected in thumbnails of that site.

 The Problem with Our Solution

 The thumbnail browser sandboxes its pages from your normal browsing
 session, but of course it doesn't sandbox them from your private browsing
 session. If you're logged in to a site in a [private browsing] window, then
 you'll also be logged in in the page we load in the thumbnail browser,
 which is the exact thing we're trying to avoid by using a private browser.
 This is a consequence of private browsing's binary design: it's either on
 or off, and all the various Gecko bits that manage state are designed to
 use one state for private requests and another state for normal requests.
 There's no notion of multiple concurrent private browsing sessions.

 We avoid this problem in a crude way by ignoring capture requests while
 there are private windows open. We need a better solution that allows us to
 capture sandboxed pages no matter what the user is doing.

 Requirements

 We think we need the following to sandbox pages in our thumbnail browser:

 (a) Requests must be stateless. Requests must not include any information
 derived from previous requests. e.g., requests must not include cookies.

 (b) Requests must leave no trace. Requests must not be stored in whole or
 part, and no information about or derived from requests must be stored.
 e.g., the request must not be recorded in the user's history.

 (c) Responses must leave no trace. Responses must not be stored in whole
 or part, and no information about or derived from responses must be stored.
 e.g., cookies returned by the response must be ignored, and the response
 must not be recorded in the user's history.

 A fourth requirement unrelated to sandboxing is:

 (d) We need to load pages off-screen, and the user should never have to
 know about it. It shouldn't impact browser responsiveness, windows and
 dialogs triggered by pages shouldn't pop up, audio shouldn't be audible,
 etc. (We've done some of this work already by making the browser remote,
 putting it in the hidden window, and in bugs 875157 and 759964.)