Re: Extending the length of an XPCOM string when writing to it via a raw pointer

2018-08-30 Thread Henri Sivonen 
On Thu, Aug 30, 2018 at 7:43 PM, Henri Sivonen  wrote:
>> What is then the point of SetCapacity anymore?
>
> To avoid multiple allocations during a sequence of Append()s. (This is
> documented on the header.)

At this point, it's probably relevant to mention that SetCapacity() in
situations other that ahead of a sequence of Append()s is most likely
wrong (and has been so since at least 2004; I didn't bother doing code
archeology further back than that).

SetCapacity() followed immediately by Truncate() is bad. SetCapacity()
allocates a buffer. Truncate() releases the buffer.

SetCapacity() followed immediately by AssignLiteral() of the
compatible character type ("" literal with nsACString and u"" literal
with nsAString) is bad. SetCapacity() allocates a buffer.
AssignLiteral() releases the buffer and makes the string point to the
literal in POD.

SetCapacity() followed immediately by Adopt() is bad. SetCapacity()
allocates a buffer. Adopt() releases the buffer and makes the string
point to the buffer passed to Adopt().

SetCapacity() followed immediately by Assign() is likely bad. If the
string that gets assigned points to a shareable buffer and doesn't
need to be copied, Assign() releases the buffer allocated by
SetCapacity().

Allocating an nsAuto[C]String and immediately calling SetCapacity()
with a constant argument is bad. If the requested capacity is smaller
than the inline buffer, it's a no-op. If the requested capacity is
larger, the inline buffer is wasted stack space. Instead of
SetCapacity(N), it makes sense to declare nsAuto[C]StringN (with
awareness that a very large N may be a problem in terms of overflowing
the run-time stack).

(I've seen all of the above in our code base and have a patch coming up.)

-- 
Henri Sivonen
hsivo...@mozilla.com
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Layout Bugzilla components

2018-08-30 Thread Cameron McCormack 
[bcc: firefox-dev, firefox-triage-leads]

Hi,

The Layout team has done some Bugzilla component reorganization to distribute 
features and areas of code a bit better.  There are a number of new components 
split out from existing ones:

* Previously covered by CSS Parsing and Computation:
** CSS Transitions and Animations

* Previously covered by the general Layout component:
** Layout: Columns
** Layout: Flexbox
** Layout: Generated Content, Lists, and Counters
** Layout: Grid
** Layout: Ruby
** Layout: Scrolling and Overflow

Some lesser used components have been merged into other components:

* Layout: HTML Frames => Layout: Images (which is renamed; see below)
* Layout: Misc Code => Layout
* Layout: View Rendering => Layout: Web Painting

And a couple of components have been renamed:

* Layout: Images -> Layout: Images, Video, and HTML Frames
* Layout: R & A Pos -> Layout: Positioned
* Layout: Text -> Layout: Text and Fonts

Also, the descriptions for all of these components have been updated, and now 
include a list of specifications they are intended to cover.

Hopefully this makes it easier for developers to watch for bugs related to 
features they are interested in, and for bug filers to find the right component 
to file their bugs in.

Note that apart from the components that were merged, I haven't asked for 
existing bugs to be moved from their old components to their new, more 
specialized components.  I will leave it to the new triage leads for individual 
components to decide which bugs to move across if any.

Please check the Bugzilla components you are watching and whether you need to 
update any bookmarked search or bug filing URLs.

Thanks,

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

Re: Rebuild speeds (Was: Re: JS compilation/evaluation APIs are now in #include "js/CompilationAndEvaluation.h")

2018-08-30 Thread Michael Shal 
On Thu, Aug 30, 2018 at 6:04 PM, Michael Shal  wrote:
>
>
> In this case, the 99:44 rebuild times look to be an artifact of how the
> outputs of GenerateServoStyleConsts.py are consumed by a large chunk of the
> Rust and C++ code. Attached is a graphviz .dot file for reference (with
> graphviz, 'dot -Tpng servo-style-consts.dot > servo-style-consts.png' and
> view the image).
>

I'm told the attachment was eaten. Here it is inline:

digraph G {
node_148923 [label="lib.rs\n" shape="oval" color="#00"
fontcolor="#00" style=solid];
node_1746856 -> node_148923 [dir=back,style="solid",arrowtail="empty"];
node_272075 [label="\n" shape="hexagon"
color="#00" fontcolor="#00" style=solid];
node_1632132 -> node_272075 [dir=back,style="solid",arrowtail="empty"];
node_285514 -> node_272075 [dir=back,style="solid",arrowtail="empty"];
node_272674 -> node_272075 [dir=back,style="solid",arrowtail="empty"];
node_272099 [label="Unified_cpp_storage0.o\n529 files" shape="oval"
color="#00" fontcolor="#00" style=solid];
node_1632132 -> node_272099 [dir=back,style="solid",arrowtail="empty"];
node_286141 -> node_272099 [dir=back,style="solid",arrowtail="empty"];
node_272100 [label="CXX Unified_cpp_storage0.cpp\n529 commands"
shape="rectangle" color="#00" fontcolor="#00" style=solid];
node_1632132 -> node_272100 [dir=back,style="solid",arrowtail="empty"];
node_272099 -> node_272100 [dir=back,style="solid",arrowtail="empty"];
node_288318 [label="TestDownscalingFilterNoSkia.o\n22 files"
shape="oval" color="#00" fontcolor="#00" style=solid];
node_1632132 -> node_288318 [dir=back,style="solid",arrowtail="empty"];
node_288319 [label="CXX
/home/mshal/mozilla-central-git/image/test/gtest/TestDownscalingFilterNoSkia.cpp\n22
commands" shape="rectangle" color="#00" fontcolor="#00"
style=solid];
node_1632132 -> node_288319 [dir=back,style="solid",arrowtail="empty"];
node_288318 -> node_288319 [dir=back,style="solid",arrowtail="empty"];
node_1826863 [label="atom_macro.rs\n52 files" shape="oval"
color="#00" fontcolor="#00" style=solid];
node_1826929 -> node_1826863 [dir=back,style="solid",arrowtail="empty"];
node_1826917 [label="RUN style v0.0.1 Target -> atom_macro.rs
bindings.rs ...\n" shape="rectangle" color="#00" fontcolor="#00"
style=solid];
node_1826863 -> node_1826917 [dir=back,style="solid",arrowtail="empty"];
node_1746854 [label="ServoStyleConsts.h\n" shape="oval" color="#00"
fontcolor="#00" style=solid];
node_1826917 -> node_1746854 [dir=back,style="solid",arrowtail="empty"];
node_288319 -> node_1746854 [dir=back,style="solid",arrowtail="empty"];
node_272100 -> node_1746854 [dir=back,style="solid",arrowtail="empty"];
node_272075 -> node_1746854 [dir=back,style="solid",arrowtail="empty"];
node_1746855 [label="ServoStyleConsts.h.pp\n" shape="oval"
color="#00" fontcolor="#00" style=solid];
node_272075 -> node_1746855 [dir=back,style="solid",arrowtail="empty"];
node_1746856 [label="python
/home/mshal/mozilla-central-git/layout/style/GenerateServoStyleConsts.py:generate
-> [ServoStyleConsts.h, ServoStyleConsts.h.pp]\n" shape="rectangle"
color="#00" fontcolor="#00" style=solid];
node_1746855 -> node_1746856 [dir=back,style="solid",arrowtail="empty"];
node_1746854 -> node_1746856 [dir=back,style="solid",arrowtail="empty"];
node_272075 -> node_1746856 [dir=back,style="solid",arrowtail="empty"];
node_1826930 [label="libgeckoservo-390312fceaa9ecec.a\n" shape="oval"
color="#00" fontcolor="#00" style=solid];
node_1826931 [label="libgeckoservo-390312fceaa9ecec.rlib\n"
shape="oval" color="#00" fontcolor="#00" style=solid];
node_1826970 -> node_1826931 [dir=back,style="solid",arrowtail="empty"];
node_1826964 -> node_1826931 [dir=back,style="solid",arrowtail="empty"];
node_1826932 [label="RUSTC geckoservo v0.0.1 Target ->
libgeckoservo-390312fceaa9ecec.a libgeckoservo-390312fceaa9ecec.rlib\n"
shape="rectangle" color="#00" fontcolor="#00" style=solid];
node_1826931 -> node_1826932 [dir=back,style="solid",arrowtail="empty"];
node_1826930 -> node_1826932 [dir=back,style="solid",arrowtail="empty"];
node_1826963 [label="libgkrust_shared-241fc0da610eca62.rlib\n"
shape="oval" color="#00" fontcolor="#00" style=solid];
node_1826970 -> node_1826963 [dir=back,style="solid",arrowtail="empty"];
node_1826964 [label="RUSTC gkrust-shared v0.1.0 Target ->
libgkrust_shared-241fc0da610eca62.rlib\n" shape="rectangle" color="#00"
fontcolor="#00" style=solid];
node_1826963 -> node_1826964 [dir=back,style="solid",arrowtail="empty"];
node_272674 [label="\n" shape="hexagon" color="#00"
fontcolor="#00" style=solid];
node_1632132 -> node_272674 [dir=back,style="solid",arrowtail="empty"];
node_285514 -> node_272674 [dir=back,style="solid",arrowtail="empty"];
no

Re: Dead-code removal of unused Rust FFI exports

2018-08-30 Thread Mike Hommey 
On Thu, Aug 30, 2018 at 10:21:09AM -0500, Tom Ritter wrote:
> CFI vcall requires one to specify a -fvisibility flag on the command line,
> with hidden being the preffered. We set visibility explicitly in some
> difficult-to-quickly-identify ways, and adding -fvisibility=hidden
> triggered issues with NSS (as well as apparently being redundant to what we
> currently do).  I tracked them in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1459314

The real requirement is for the maximum number of symbols not to be
exported, which usually doesn't happen. But it does in our build system,
without -fvisibility. If clang doesn't want to honor this, that is a
problem, and short-sighted on their part.

> That bug includes a monster patch to set visibility manually on a ton of
> NSS stuff to get the browser running as a POC, but CFI vcall would need
> something much more intelligent. I think the answer is to change the
> compiler to not require the flag be present.
> 
> > For Firefox, simply having an equivalent to `-fvisilbility=hidden` so
> that all the symbols in the generated static library are hidden would be
> sufficient to meet our current needs
> 
> The understanding I got from my bug was that we were already doing the
> equivalent of this... somehow.

Rust doesn't export as many symbols as C++ does by default. Practically
speaking, it's doing the same as we do for C++, without an explicit
compiler flag. The "problem" is that it still places *some* symbols
public, but we don't need them to be, and it would be better for us to
be able to *force* those to be hidden.

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

Re: Rebuild speeds (Was: Re: JS compilation/evaluation APIs are now in #include "js/CompilationAndEvaluation.h")

2018-08-30 Thread Michael Shal 
On Wed, Aug 29, 2018 at 11:36 AM, Boris Zbarsky  wrote:

> On 8/29/18 10:32 AM, Ted Mielczarek wrote:
>
>> so it's possible that there are things here that are artifacts of our tup
>> build implementation.
>>
>
> Worth keeping in mind, thank you.  Would that possibly account for the
> exactly-the-same 99:44 rebuild times for a number of files?
>
> Reading through this list again, I'm a little suspicious that
> testing/webdriver/src/lib.rs has the same rebuild time as
> servo/components/style/values/specified/mod.rs, for example...
>

In this case, the 99:44 rebuild times look to be an artifact of how the
outputs of GenerateServoStyleConsts.py are consumed by a large chunk of the
Rust and C++ code. Attached is a graphviz .dot file for reference (with
graphviz, 'dot -Tpng servo-style-consts.dot > servo-style-consts.png' and
view the image). At the bottom of the image are testing/webdriver/src/lib.rs
and servo/components/style/values/specified/mod.rs. The rebuild time is
calculated by following the links up the graph and adding up the runtimes
of the commands (shown as rectangles). Even though the mod.rs file is
directly consumed by compiling the style crate, lib.rs ends up with the
same subgraph of the DAG, so it has the same total runtime. Since
GenerateServoStyleConsts.py has a lot of input files, you'll likely see the
same runtime for all of them.

In practice for incremental builds, changing lib.rs will likely have a lot
faster build time than changing mod.rs. When cargo looks at lib.rs, it only
checks that it exists and ignores its contents. So modifying that file will
probably not change the contents of ServoStyleConsts.h, meaning we can skip
the rest of the DAG (this is done for all GENERATED_FILES). In comparison,
changing mod.rs will both regenerate ServoStyleConsts.h, *and* force the
style crate compile even if ServoStyleConsts.h is unchanged. Unfortunately
there's no easy way to tell how likely or unlikely it is that a particular
change will affect the output of a GENERATED_FILES script from the DAG, so
this report is limited to taking a worst-case view and assumes that
everything downstream needs to be updated.

The idea behind the report was that we could quantify what we know
anecdotally (ie: editing jsapi.h is annoying!) and consider the worst
offenders for further investigation. It's entirely possible we'll determine
that a file isn't as bad as the report indicates, or maybe we determine
that a particular file is just always going to be bad because it would be a
monumental task to split it up. I thought jsapi.h was going to fall into
the latter group, so I'm glad to see jwalden and others tackle it! As that
effort progresses, we should see jsapi.h fall down the list, and then
perhaps we can use the new top offenders in the report to direct future
efforts.

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

Re: Intent to remove DHE ciphers from WebRTC DTLS handshake

2018-08-30 Thread Nicholas Alexander 
On Wed, Aug 29, 2018 at 3:56 PM, Nils Ohlmeier 
wrote:

> Summary:
>
> We are looking at removing the DHE cipher suites from the DTLS handshake
> in Firefox soon.
>
> Ciphers:
> - TLS_DHE_RSA_WITH_AES_128_CBC_SHA
> - TLS_DHE_RSA_WITH_AES_256_CBC_SHA
> are the  two suites which we want to remove, because they are considered
> too weak.
>

Are these suites considered "too weak" across the board?  For historical
reasons Firefox for Android will handshake to Firefox Sync servers using
these suites:
https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/mobile/android/services/src/main/java/org/mozilla/gecko/background/common/GlobalConstants.java#73.
Sounds like we should drop those suites there too -- can you confirm?

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

Re: Extending the length of an XPCOM string when writing to it via a raw pointer

2018-08-30 Thread Henri Sivonen 
On Thu, Aug 30, 2018 at 6:00 PM, smaug  wrote:
> On 08/30/2018 11:21 AM, Henri Sivonen wrote:
>>
>> We have the following that a pattern in our code base:
>>
>>   1) SetCapacity(newCapacity) is called on an XPCOM string.
>>   2) A pointer obtained from BeginWriting() is used for writing more
>> than Length() but no more than newCapacity code units to the XPCOM
>> string.
>>   3) SetLength(actuallyWritten)  is called on the XPCOM string such
>> that actuallyWritten > Length() but actuallyWritten <= newCapacity.
>>
>> This is an encapsulation violation, because the string implementation
>> doesn't know that the content past Length() is there.
>
> How so? The whole point of capacity is that the string has that much
> capacity.

It has that much capacity for the use of the string implementation so
that you can do a sequence of Append()s without reallocating multiple
times. It doesn't mean that the caller is eligible to write into the
internal structure of the string in an undocumented way.

>  The caller
>>
>> assumes that step #3 will not reallocate and will only write a
>> zero-terminator at actuallyWritten and set mLength to actuallyWritten.
>> (The pattern is common enough that clearly people have believed it to
>> be a valid pattern. However, I haven't seen any in-tree or on-wiki
>> string documentation endorsing the pattern.)
>>
>> It should be non-controversial that this is an encapsulation
>> violation,
>
> Well, I'm not seeing any encapsulation violation ;)
>
>
>> but does the encapsulation violation matter? It matters if
>> we want SetLength() to be able to conserve memory by allocating a
>> smaller buffer when actuallyWritten code units would fit in a smaller
>> mozjemalloc bucket.
>
>
> Please be very very careful when doing allocations and deallocations. They
> are very slow, showing
> up all the time in performance profiles.

There is a threshold so that we don't reallocate from small to even
smaller. There's a good chance that the threshold should be higher
than it is now.

>  In order for the above pattern to work if
>>
>> SetLength() can reallocate in such case, SetLength() has to memcpy the
>> whole buffer in case someone has written content that the string
>> implementation is unaware of instead of just memcpying the part of the
>> buffer that the string implementation knows to be in use. Pessimizing
>> the number of code units to memcpy is bad for performance.
>>
>> It's unclear if trying to use a smaller mozjemalloc bucket it is a
>> worthwhile thing. It obviously is for large long-lived strings and it
>> obviously isn't for short-lived strings even if they are large.
>> SetLength() doesn't know what the future holds for the string. :-( But
>> in any case, it's bad if we can't make changes that are sound from the
>> perspective of the string encapsulation, because we have other code
>> violating the encapsulation.
>>
>> After the soft freeze, I'd like to change things so that we memcpy
>> only the part of the buffer that the string implementation knows is in
>> use. To that end, we should stop using the above pattern that is an
>> encapsulation violation.
>>
> What is then the point of SetCapacity anymore?

To avoid multiple allocations during a sequence of Append()s. (This is
documented on the header.)

>> For m-c, I've filed
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1472113 and worked on
>> fixing the bugs that block it. For c-c, I've filed
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1486706 but don't intend
>> to do the work of investigating or fixing the string usage in c-c.
>>
>> As for fixing the above pattern, there are two alternatives. The first one
>> is:
>>
>>   1) SetLength(newCapacity)
>>   2) BeginWriting()
>>   3) Truncate(actuallyWritten) (or SetLength(actuallyWritten), Truncate
>> simply tells to the reader that the string isn't being made longer)
>>
>> With this pattern, writing happens to the part of the buffer that the
>> string implementation believes to be in use. This has the downside
>> that the first SetLength() call (like, counter-intuitively,
>> SetCapacity() currently!) writes the zero terminator, which from the
>> point of view of CPU caches is an out-of-the-blue write that's not
>> part of a well-behaved forward-only linear write pattern and not
>> necessarily near recently-accessed locations.
>>
>> The second alternative is BulkWrite() in C++ and bulk_write() in Rust.
>
> The API doesn't seem to be too user friendly.

Zero-termination is sad. An API that doesn't zero-terminate eagerly
but maintains some kind of encapsulation necessarily leads to a
complex API. If you don't care about how capacity was rounded up and
don't care about avoiding cache-unfriendly eager zero termination, you
can use the other pattern suggested above, which is almost like the
old pattern.

-- 
Henri Sivonen
hsivo...@mozilla.com
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-plat

Re: Dead-code removal of unused Rust FFI exports

2018-08-30 Thread Tom Ritter 
CFI vcall requires one to specify a -fvisibility flag on the command line,
with hidden being the preffered. We set visibility explicitly in some
difficult-to-quickly-identify ways, and adding -fvisibility=hidden
triggered issues with NSS (as well as apparently being redundant to what we
currently do).  I tracked them in
https://bugzilla.mozilla.org/show_bug.cgi?id=1459314

That bug includes a monster patch to set visibility manually on a ton of
NSS stuff to get the browser running as a POC, but CFI vcall would need
something much more intelligent. I think the answer is to change the
compiler to not require the flag be present.

> For Firefox, simply having an equivalent to `-fvisilbility=hidden` so
that all the symbols in the generated static library are hidden would be
sufficient to meet our current needs

The understanding I got from my bug was that we were already doing the
equivalent of this... somehow.

-tom


On Thu, Aug 30, 2018 at 9:52 AM, Ted Mielczarek  wrote:

> I thought we had filed a Rust issue on this but apparently not. There's
> some good discussion in this issue:
> https://github.com/rust-lang/rust/issues/37530
>
> For Firefox, simply having an equivalent to `-fvisilbility=hidden` so that
> all the symbols in the generated static library are hidden would be
> sufficient to meet our current needs, since we don't need to export any
> actual public APIs from Rust code. For that feature to be usable for more
> use cases you'd probably also want an equivalent to `__attribute__
> ((visibility ("default")))`, but I don't know what exactly that'd look like.
>
> -Ted
>
> On Wed, Aug 29, 2018, at 3:38 AM, Michael Woerister wrote:
> > For some background: The Rust compiler will mark everything as `hidden`
> in
> > LLVM IR unless it is exported via a C interface (via either #[no_mangle]
> or
> > #[export_name]). So the FFI symbols in gkrust will have default
> visibility.
> > LLD will thus probably retain and re-export them, even if they are not
> > called from within libXUL. But we should double-check that.
> >
> > It might be a good idea to provide a way to tell the Rust compiler to
> make
> > everything `hidden` by default.
> >
> > On Tue, Aug 28, 2018 at 3:56 PM, Till Schneidereit <
> > t...@tillschneidereit.net> wrote:
> >
> > > CC'ing mw, who is working on cross-language LTO.
> > >
> > > On Tue, Aug 28, 2018 at 3:20 PM Mike Hommey  wrote:
> > >
> > >> On Tue, Aug 28, 2018 at 09:14:02AM -0400, Jeff Muizelaar wrote:
> > >> > We don't LTO yet on Mac.
> > >>
> > >> We don't LTO across languages on any platform yet. Rust is LTOed on
> all
> > >> platforms, which removes a bunch of its symbols. Everything that is
> > >> exposed for C/C++ from Rust, though, is left alone. That's likely to
> > >> stay true even with cross-language LTO, because as far as the linker
> is
> > >> concerned, those FFI symbols might be used by code that link against
> > >> libxul, so it would still export them. We'd essentially need the
> > >> equivalent to -fvisibility=hidden for Rust for that to stop being
> true.
> > >>
> > >> Mike
> > >>
> > >>
> > >> > On Tue, Aug 28, 2018 at 5:17 AM, Emilio Cobos Álvarez <
> emi...@crisal.io>
> > >> wrote:
> > >> > >
> > >> > >
> > >> > > On 8/28/18 9:35 AM, Henri Sivonen wrote:
> > >> > >>
> > >> > >> Does some lld mechanism successfully remove dead code when gkrust
> > >> > >> exports some FFI function that the rest of Gecko never ends up
> > >> > >> calling?
> > >> > >
> > >> > >
> > >> > > I would expect LTO to get rid of it, but haven't checked myself.
> > >> > >
> > >> > >> I.e. in terms of code size, is it OK to vendor an FFI-exposing
> Rust
> > >> > >> crate where not every FFI function is used (at least right away)?
> > >> > >>
> > >> > > ___
> > >> > > 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
> ___
> 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: Extending the length of an XPCOM string when writing to it via a raw pointer

2018-08-30 Thread smaug 

On 08/30/2018 11:21 AM, Henri Sivonen wrote:

We have the following that a pattern in our code base:

  1) SetCapacity(newCapacity) is called on an XPCOM string.
  2) A pointer obtained from BeginWriting() is used for writing more
than Length() but no more than newCapacity code units to the XPCOM
string.
  3) SetLength(actuallyWritten)  is called on the XPCOM string such
that actuallyWritten > Length() but actuallyWritten <= newCapacity.

This is an encapsulation violation, because the string implementation
doesn't know that the content past Length() is there.

How so? The whole point of capacity is that the string has that much capacity.



 The caller

assumes that step #3 will not reallocate and will only write a
zero-terminator at actuallyWritten and set mLength to actuallyWritten.
(The pattern is common enough that clearly people have believed it to
be a valid pattern. However, I haven't seen any in-tree or on-wiki
string documentation endorsing the pattern.)

It should be non-controversial that this is an encapsulation
violation,

Well, I'm not seeing any encapsulation violation ;)



but does the encapsulation violation matter? It matters if
we want SetLength() to be able to conserve memory by allocating a
smaller buffer when actuallyWritten code units would fit in a smaller
mozjemalloc bucket.


Please be very very careful when doing allocations and deallocations. They are 
very slow, showing
up all the time in performance profiles.


 In order for the above pattern to work if

SetLength() can reallocate in such case, SetLength() has to memcpy the
whole buffer in case someone has written content that the string
implementation is unaware of instead of just memcpying the part of the
buffer that the string implementation knows to be in use. Pessimizing
the number of code units to memcpy is bad for performance.

It's unclear if trying to use a smaller mozjemalloc bucket it is a
worthwhile thing. It obviously is for large long-lived strings and it
obviously isn't for short-lived strings even if they are large.
SetLength() doesn't know what the future holds for the string. :-( But
in any case, it's bad if we can't make changes that are sound from the
perspective of the string encapsulation, because we have other code
violating the encapsulation.

After the soft freeze, I'd like to change things so that we memcpy
only the part of the buffer that the string implementation knows is in
use. To that end, we should stop using the above pattern that is an
encapsulation violation.


What is then the point of SetCapacity anymore?



For m-c, I've filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1472113 and worked on
fixing the bugs that block it. For c-c, I've filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1486706 but don't intend
to do the work of investigating or fixing the string usage in c-c.

As for fixing the above pattern, there are two alternatives. The first one is:

  1) SetLength(newCapacity)
  2) BeginWriting()
  3) Truncate(actuallyWritten) (or SetLength(actuallyWritten), Truncate
simply tells to the reader that the string isn't being made longer)

With this pattern, writing happens to the part of the buffer that the
string implementation believes to be in use. This has the downside
that the first SetLength() call (like, counter-intuitively,
SetCapacity() currently!) writes the zero terminator, which from the
point of view of CPU caches is an out-of-the-blue write that's not
part of a well-behaved forward-only linear write pattern and not
necessarily near recently-accessed locations.

The second alternative is BulkWrite() in C++ and bulk_write() in Rust.

The API doesn't seem to be too user friendly.



This is new API that is well-behaved in terms of the cache access
pattern and is also more versatile in the sense that it lets the
caller know how newCapacity was rounded up, which is relevant to
callers that ask for best-case capacity and then ask more capacity if
there turns out to be more to write. When the caller is made aware of
the rounding, a second request for added capacity can be avoided if
the amount that actually needs to be written exceeds the best case
estimate but fits within the rounded-up capacity.

In Rust, bulk_write() is rather nicely misuse-resistant.  However, on
the C++ side the lack of a borrow checker as well as mozilla::Result
not working with move-only types
(https://bugzilla.mozilla.org/show_bug.cgi?id=1418624) pushes more
things to documentation. The documentation can be found at
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Bulk_Write
https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/xpcom/string/nsTSubstring.h#1190
https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/xpcom/string/nsTSubstring.h#32

P.S. GetMutableData() is redundant with BeginWriting() and
SetLength(). It's used very rarely and I'd like to remove it as
redundant, so please don't use GetMutableData()

Re: Dead-code removal of unused Rust FFI exports

2018-08-30 Thread Ted Mielczarek 
I thought we had filed a Rust issue on this but apparently not. There's some 
good discussion in this issue:
https://github.com/rust-lang/rust/issues/37530

For Firefox, simply having an equivalent to `-fvisilbility=hidden` so that all 
the symbols in the generated static library are hidden would be sufficient to 
meet our current needs, since we don't need to export any actual public APIs 
from Rust code. For that feature to be usable for more use cases you'd probably 
also want an equivalent to `__attribute__ ((visibility ("default")))`, but I 
don't know what exactly that'd look like.

-Ted

On Wed, Aug 29, 2018, at 3:38 AM, Michael Woerister wrote:
> For some background: The Rust compiler will mark everything as `hidden` in
> LLVM IR unless it is exported via a C interface (via either #[no_mangle] or
> #[export_name]). So the FFI symbols in gkrust will have default visibility.
> LLD will thus probably retain and re-export them, even if they are not
> called from within libXUL. But we should double-check that.
> 
> It might be a good idea to provide a way to tell the Rust compiler to make
> everything `hidden` by default.
> 
> On Tue, Aug 28, 2018 at 3:56 PM, Till Schneidereit <
> t...@tillschneidereit.net> wrote:
> 
> > CC'ing mw, who is working on cross-language LTO.
> >
> > On Tue, Aug 28, 2018 at 3:20 PM Mike Hommey  wrote:
> >
> >> On Tue, Aug 28, 2018 at 09:14:02AM -0400, Jeff Muizelaar wrote:
> >> > We don't LTO yet on Mac.
> >>
> >> We don't LTO across languages on any platform yet. Rust is LTOed on all
> >> platforms, which removes a bunch of its symbols. Everything that is
> >> exposed for C/C++ from Rust, though, is left alone. That's likely to
> >> stay true even with cross-language LTO, because as far as the linker is
> >> concerned, those FFI symbols might be used by code that link against
> >> libxul, so it would still export them. We'd essentially need the
> >> equivalent to -fvisibility=hidden for Rust for that to stop being true.
> >>
> >> Mike
> >>
> >>
> >> > On Tue, Aug 28, 2018 at 5:17 AM, Emilio Cobos Álvarez 
> >> wrote:
> >> > >
> >> > >
> >> > > On 8/28/18 9:35 AM, Henri Sivonen wrote:
> >> > >>
> >> > >> Does some lld mechanism successfully remove dead code when gkrust
> >> > >> exports some FFI function that the rest of Gecko never ends up
> >> > >> calling?
> >> > >
> >> > >
> >> > > I would expect LTO to get rid of it, but haven't checked myself.
> >> > >
> >> > >> I.e. in terms of code size, is it OK to vendor an FFI-exposing Rust
> >> > >> crate where not every FFI function is used (at least right away)?
> >> > >>
> >> > > ___
> >> > > 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

(Mobile) Component Idea - Performance Policy Manager

2018-08-30 Thread Stefan Arentz 
Hello everyone,

I was debugging something completely unrelated to Firefox on my Mac when I
saw the following events in my console:

501:com.apple.safarishared.WBSParsecDSession.*autoFillDataUpdate*:FD5887:[
{name: *DeviceActivityPolicy*, policyWeight: 2.000, response: {*Decision:
Can Proceed*, Score: 0.89}}
 ] sumScores:31.798889, denominator:32.01, *FinalDecision: Can Proceed,*
Final Score: 0.993405}

501:com.apple.Safari.SafeBrowsing.*BrowsingDatabases.Update*:D8F622:[
{name: *ChargerPluggedInPolicy*, policyWeight: 20.000, response:
{*Decision:
Must Not Proceed*, Score: 0.00, Rationale: [{/*device/system/isPluggedIn ==
0*}]}}
 ], *FinalDecision: Must Not Proceed*}

What you are looking at is an OS-provided policy engine that can tell an
application if now is a good time to perform a specific task. Specifically
tasks that use a scarce resource like the network, battery or cpu. This is
all about not being that energy hog on a mobile device.

In the above two samples the tasks involve talking to the network; syncing
autofill data with iCloud and keeping a SafeBrowsing database up to date.
The first request is allowed to happen while the second is not. This is
because I removed the charging cable from my macBook and to safe energy,
macOS recommends to not do unnecessary network activity in that case.

This would be wonderful to have in our Android products. But probably also
in Gecko - assuming we have a lot of people using laptops.

For example those products could start asking questions like:

 - Should we vacuum our SQLite databases? (No - the user is actively using
the application and the battery level is low)
 - Should we download the safe browsing database? (No - the device is not
connected to the charger)
 - Should we resize screenshots for top sites? (No  - the device is running
a bit hot currently)
 - Should we sync history and bookmarks? (No - the device is in a
downgraded network state, like GPRS or 3G vs the normal LTE)
 - Should we download the latest Nightly update? (No - disk space is low)

I think if we build a policy engine that lets you describe your intent ("I
want to download an 80 MB file") together with weighted policies
(DeviceActivity, NetworkType, DiskSpace, NetworkType, etc.) we could
improve the scheduling of tasks and be more efficient on battery powered
devices.

What do you think? Is this something we should invest in?

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

Re: Dead-code removal of unused Rust FFI exports

2018-08-30 Thread Michael Woerister 
For some background: The Rust compiler will mark everything as `hidden` in
LLVM IR unless it is exported via a C interface (via either #[no_mangle] or
#[export_name]). So the FFI symbols in gkrust will have default visibility.
LLD will thus probably retain and re-export them, even if they are not
called from within libXUL. But we should double-check that.

It might be a good idea to provide a way to tell the Rust compiler to make
everything `hidden` by default.

On Tue, Aug 28, 2018 at 3:56 PM, Till Schneidereit <
t...@tillschneidereit.net> wrote:

> CC'ing mw, who is working on cross-language LTO.
>
> On Tue, Aug 28, 2018 at 3:20 PM Mike Hommey  wrote:
>
>> On Tue, Aug 28, 2018 at 09:14:02AM -0400, Jeff Muizelaar wrote:
>> > We don't LTO yet on Mac.
>>
>> We don't LTO across languages on any platform yet. Rust is LTOed on all
>> platforms, which removes a bunch of its symbols. Everything that is
>> exposed for C/C++ from Rust, though, is left alone. That's likely to
>> stay true even with cross-language LTO, because as far as the linker is
>> concerned, those FFI symbols might be used by code that link against
>> libxul, so it would still export them. We'd essentially need the
>> equivalent to -fvisibility=hidden for Rust for that to stop being true.
>>
>> Mike
>>
>>
>> > On Tue, Aug 28, 2018 at 5:17 AM, Emilio Cobos Álvarez 
>> wrote:
>> > >
>> > >
>> > > On 8/28/18 9:35 AM, Henri Sivonen wrote:
>> > >>
>> > >> Does some lld mechanism successfully remove dead code when gkrust
>> > >> exports some FFI function that the rest of Gecko never ends up
>> > >> calling?
>> > >
>> > >
>> > > I would expect LTO to get rid of it, but haven't checked myself.
>> > >
>> > >> I.e. in terms of code size, is it OK to vendor an FFI-exposing Rust
>> > >> crate where not every FFI function is used (at least right away)?
>> > >>
>> > > ___
>> > > 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: How do I inject the WebExtensions API into a ?

2018-08-30 Thread Andrew Swan 
Geoff,

First, I'm moving this over to dev-addons since it is about the internals
of the webextensions implementation and probably not of interest to many of
the people on dev-platform.  Anybody from dev-platform who is interested,
feel free to follow us over to dev-addons.

The short answer is that the ExtensionPolicyService observes
"document-element-inserted" and, for every document created with an
extension principal (ie, for moz-extension: pages), it calls this function:
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/components/extensions/extension-process-script.js#275-282

If you trace the calls made from there, you'll eventually end up here,
which is where the actual "browser" and "chrome" objects are created:
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/components/extensions/ExtensionPageChild.jsm#170-183

I've glossed over several steps, I'm happy to answer further questions on
dev-addons or in #webextensions on IRC

-Andrew


On Tue, Aug 28, 2018 at 1:54 AM, Geoff Lankow  wrote:

>
> I'm trying to make WebExtensions usable on Thunderbird. My current problem
> is trying to make moz-extension:// pages work in the UI. Everything happens
> as expected except the page has no access to the "browser" API.
>
> Can somebody show me how to set things up so the API is added to new
> browser elements and available when it should be?
>
> GL
> ___
> 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: (Mobile) Component Idea - Performance Policy Manager

2018-08-30 Thread Michael Comella 
Hey Stefan - good idea!

fwiw, the Android support library has the WorkManager API

which handles similar functionality. However, it's a push rather than a
poll (as you suggested): you tell it the constraints under which you want
your job to run (any battery level, metered network, run once) and it'll
find a good time for it to run. We could consider providing bindings to
this library for our native code and Gecko, or write our own policy
framework.
- Mike (:mcomella)

On Wed, Aug 29, 2018 at 12:52 PM Stefan Arentz  wrote:

> Hello everyone,
>
> I was debugging something completely unrelated to Firefox on my Mac when I
> saw the following events in my console:
>
> 501:com.apple.safarishared.WBSParsecDSession.*autoFillDataUpdate*:FD5887:[
> {name: *DeviceActivityPolicy*, policyWeight: 2.000, response: {*Decision:
> Can Proceed*, Score: 0.89}}
>  ] sumScores:31.798889, denominator:32.01, *FinalDecision: Can
> Proceed,* Final Score: 0.993405}
>
> 501:com.apple.Safari.SafeBrowsing.*BrowsingDatabases.Update*:D8F622:[
> {name: *ChargerPluggedInPolicy*, policyWeight: 20.000, response: 
> {*Decision:
> Must Not Proceed*, Score: 0.00, Rationale: [{/*device/system/isPluggedIn
> == 0*}]}}
>  ], *FinalDecision: Must Not Proceed*}
>
> What you are looking at is an OS-provided policy engine that can tell an
> application if now is a good time to perform a specific task. Specifically
> tasks that use a scarce resource like the network, battery or cpu. This is
> all about not being that energy hog on a mobile device.
>
> In the above two samples the tasks involve talking to the network; syncing
> autofill data with iCloud and keeping a SafeBrowsing database up to date.
> The first request is allowed to happen while the second is not. This is
> because I removed the charging cable from my macBook and to safe energy,
> macOS recommends to not do unnecessary network activity in that case.
>
> This would be wonderful to have in our Android products. But probably also
> in Gecko - assuming we have a lot of people using laptops.
>
> For example those products could start asking questions like:
>
>  - Should we vacuum our SQLite databases? (No - the user is actively using
> the application and the battery level is low)
>  - Should we download the safe browsing database? (No - the device is not
> connected to the charger)
>  - Should we resize screenshots for top sites? (No  - the device is
> running a bit hot currently)
>  - Should we sync history and bookmarks? (No - the device is in a
> downgraded network state, like GPRS or 3G vs the normal LTE)
>  - Should we download the latest Nightly update? (No - disk space is low)
>
> I think if we build a policy engine that lets you describe your intent ("I
> want to download an 80 MB file") together with weighted policies
> (DeviceActivity, NetworkType, DiskSpace, NetworkType, etc.) we could
> improve the scheduling of tasks and be more efficient on battery powered
> devices.
>
> What do you think? Is this something we should invest in?
>
>  S.
>
> ___
> android-components mailing list
> android-compone...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/android-components
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Extending the length of an XPCOM string when writing to it via a raw pointer

2018-08-30 Thread Henri Sivonen 
We have the following that a pattern in our code base:

 1) SetCapacity(newCapacity) is called on an XPCOM string.
 2) A pointer obtained from BeginWriting() is used for writing more
than Length() but no more than newCapacity code units to the XPCOM
string.
 3) SetLength(actuallyWritten)  is called on the XPCOM string such
that actuallyWritten > Length() but actuallyWritten <= newCapacity.

This is an encapsulation violation, because the string implementation
doesn't know that the content past Length() is there. The caller
assumes that step #3 will not reallocate and will only write a
zero-terminator at actuallyWritten and set mLength to actuallyWritten.
(The pattern is common enough that clearly people have believed it to
be a valid pattern. However, I haven't seen any in-tree or on-wiki
string documentation endorsing the pattern.)

It should be non-controversial that this is an encapsulation
violation, but does the encapsulation violation matter? It matters if
we want SetLength() to be able to conserve memory by allocating a
smaller buffer when actuallyWritten code units would fit in a smaller
mozjemalloc bucket. In order for the above pattern to work if
SetLength() can reallocate in such case, SetLength() has to memcpy the
whole buffer in case someone has written content that the string
implementation is unaware of instead of just memcpying the part of the
buffer that the string implementation knows to be in use. Pessimizing
the number of code units to memcpy is bad for performance.

It's unclear if trying to use a smaller mozjemalloc bucket it is a
worthwhile thing. It obviously is for large long-lived strings and it
obviously isn't for short-lived strings even if they are large.
SetLength() doesn't know what the future holds for the string. :-( But
in any case, it's bad if we can't make changes that are sound from the
perspective of the string encapsulation, because we have other code
violating the encapsulation.

After the soft freeze, I'd like to change things so that we memcpy
only the part of the buffer that the string implementation knows is in
use. To that end, we should stop using the above pattern that is an
encapsulation violation.

For m-c, I've filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1472113 and worked on
fixing the bugs that block it. For c-c, I've filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1486706 but don't intend
to do the work of investigating or fixing the string usage in c-c.

As for fixing the above pattern, there are two alternatives. The first one is:

 1) SetLength(newCapacity)
 2) BeginWriting()
 3) Truncate(actuallyWritten) (or SetLength(actuallyWritten), Truncate
simply tells to the reader that the string isn't being made longer)

With this pattern, writing happens to the part of the buffer that the
string implementation believes to be in use. This has the downside
that the first SetLength() call (like, counter-intuitively,
SetCapacity() currently!) writes the zero terminator, which from the
point of view of CPU caches is an out-of-the-blue write that's not
part of a well-behaved forward-only linear write pattern and not
necessarily near recently-accessed locations.

The second alternative is BulkWrite() in C++ and bulk_write() in Rust.
This is new API that is well-behaved in terms of the cache access
pattern and is also more versatile in the sense that it lets the
caller know how newCapacity was rounded up, which is relevant to
callers that ask for best-case capacity and then ask more capacity if
there turns out to be more to write. When the caller is made aware of
the rounding, a second request for added capacity can be avoided if
the amount that actually needs to be written exceeds the best case
estimate but fits within the rounded-up capacity.

In Rust, bulk_write() is rather nicely misuse-resistant.  However, on
the C++ side the lack of a borrow checker as well as mozilla::Result
not working with move-only types
(https://bugzilla.mozilla.org/show_bug.cgi?id=1418624) pushes more
things to documentation. The documentation can be found at
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Bulk_Write
https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/xpcom/string/nsTSubstring.h#1190
https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/xpcom/string/nsTSubstring.h#32

P.S. GetMutableData() is redundant with BeginWriting() and
SetLength(). It's used very rarely and I'd like to remove it as
redundant, so please don't use GetMutableData().

-- 
Henri Sivonen
hsivo...@mozilla.com
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform