Re: Actually-Infallible Fallible Allocations

[Valentin Gosu]

On 3 August 2017 at 23:12, Jim Blandy  wrote:

>
> But my question wasn't, "is pre-reservation ever valuable?" but rather "is
> it valuable in this particular code?" My assumption is that most code isn't
> performance-sensitive, and so simplicity should be the priority.
>
> The code Alexis cited at the top of the thread is here:
> http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f55
> 4da0b89a33/netwerk/base/Dashboard.cpp#435
>
> Is Dashboard performance-sensitive?
>

Not really.
When I wrote the code I copied most of it from code automatically generated
by webidl.
Some things have changed in the meantime - such as fallible allocations -
maybe we should update to resemble something a human would write :)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Actually-Infallible Fallible Allocations

[Jim Blandy]

On Wed, Aug 2, 2017 at 7:58 AM, Ehsan Akhgari 
wrote:

> Vector reallocations show up in profiles all the time, literally in more
> than half of the profiles I look at every day.  If you examine for example
> the large dependency graph of https://bugzilla.mozilla.org/s
> how_bug.cgi?id=Speedometer_V2, a large number of optimizations that are
> landing every day are merely removing various types of extra needless
> buffer reallocations we have in profiles.


Okay, that's a surprise to me, and definitely something I will try to keep
in mind.

But my question wasn't, "is pre-reservation ever valuable?" but rather "is
it valuable in this particular code?" My assumption is that most code isn't
performance-sensitive, and so simplicity should be the priority.

The code Alexis cited at the top of the thread is here:
http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/netwerk/base/Dashboard.cpp#435

Is Dashboard performance-sensitive?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Actually-Infallible Fallible Allocations

[Boris Zbarsky]

On 8/2/17 11:18 AM, Nathan Froyd wrote:

In particular, the API of Sequence<> is constrained because it
inherits from FallibleTArray, which *only* exposes fallible
operations.


We should consider just fixing this.

The history here is that FallibleTArray and InfallibleTArray used to 
bake the allocator into the class type.  There was no way to do fallible 
operations with an InfallibleTArray.


Because Sequence is used for both in parameters and return values in the 
DOM, and in parameters _must_ be fallible (web content trivially 
controls the allocation size: "var arr = []; arr.length = 1e9;"), 
Sequence inherited from FallibleTArray.


But for return values, where _we_ may control the allocation size, it 
may be reasonable to use infallible array bits if we know the array is 
small.


I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1386711 on maybe 
changing this.


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

Re: Actually-Infallible Fallible Allocations

[Nathan Froyd]

On Tue, Aug 1, 2017 at 12:31 PM, Alexis Beingessner
 wrote:
> I was recently searching through our codebase to look at all the ways we
> use fallible allocations, and was startled when I came across several lines
> that looked like this:
>
> dom::SocketElement  = *sockets.AppendElement(fallible);
>
> For those who aren't familiar with how our allocating APIs work:
>
> * by default we hard abort on allocation failure
> * but if you add `fallible` (or use the Fallible template), you will get
> back nullptr on allocation failure
>
> So in isolation this code is saying "I want to handle allocation failure"
> and then immediately not doing that and just dereferencing the result. This
> turns allocation failure into Undefined Behaviour, rather than a process
> abort.
>
> Thankfully, all the cases where I found this were preceded by something
> like the following:
>
> uint32_t length = socketData->mData.Length();if
> (!sockets.SetCapacity(length, fallible)) {   JS_ReportOutOfMemory(cx);
>   return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
> socketData->mData.Length(); i++) {dom::SocketElement  =
> *sockets.AppendElement(fallible);
>
> So really, the fallible AppendElement *is* infallible, but we're just
> saying "fallible" anyway.

Stating this upfront: I think we should use infallible AppendElement
here, or at the very least use a comment.  But it's worth looking at
the larger context of one of these examples:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/Dashboard.cpp#426-435

(AFAICT, the only such examples of this pattern come from the above
file, and for the reasons outlined below.)

In particular, the API of Sequence<> is constrained because it
inherits from FallibleTArray, which *only* exposes fallible
operations.  One can argue that FallibleTArray shouldn't do this, but
for Sequence, which is used for DOM bindings code, I believe the
intent is to nudge people writing DOM-exposed code to consider how to
recover from allocation failures, and to not blindly assume that
everything succeeds.

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

Re: Actually-Infallible Fallible Allocations

[Ehsan Akhgari]

On 08/01/2017 08:16 PM, Jim Blandy wrote:

I have to ask: does anyone recall the benchmarks that showed that bounds
checks or vector reallocations were a measurable performance hit in this
code?
Extra needless bounds checks certainly have shown up in profiles, for an 
example see https://bugzilla.mozilla.org/show_bug.cgi?id=1374033 where 
we were accidentally doing two bounds checks when using nsTObserverArray 
iterators on release builds as opposed to one.


Vector reallocations show up in profiles all the time, literally in more 
than half of the profiles I look at every day.  If you examine for 
example the large dependency graph of 
https://bugzilla.mozilla.org/show_bug.cgi?id=Speedometer_V2, a large 
number of optimizations that are landing every day are merely removing 
various types of extra needless buffer reallocations we have in profiles.



If we don't, we should just write simple, obviously correct code.

If we do, there should be a comment to that effect, or something to convey
to readers the performance constraints this code is trying to satisfy.


Where do you propose for such a comment to go?

Generic code such as arrays and vectors must be free of additional 
needless overhead.  They are bound to be used in performance sensitive 
areas, and these issues will show up sooner or later.  I believe we have 
tons of practical evidence to suggest that these are far from 
theoretical concerns.


Cheers,
Ehsan


On Tue, Aug 1, 2017 at 2:10 PM, Kris Maglione  wrote:


On Tue, Aug 01, 2017 at 01:28:37PM -0700, Eric Rahm wrote:


Both AppendElements and SetLength default initialize the values, there's
no
intermediate uninitialzed state. SetCapacity doesn't initialize the
values,
but they're also not indexable -- we have release bounds assertions --
unless you try really hard.


For a lot of cases, default initialized isn't much better than completely
uninitialized. And it has an additional, unnecessary performance penalty
for the pattern that you suggested.

SetCapacity brings us back to the same problem where we either have
unnecessary allocation checks for every element we append, or we skip
sanity checks entirely and hope things stay sane if we refactor.

With the infallible*() methods of the Vector class, though, we don't have
to worry about any of that, and the code you suggested below becomes
something like:

  Vector sockets;
  if (!sockets.reserve(inputs.length()))
return false;

  for (auto input : inputs)
sockets.infallibleEmplaceBack(input);

and we still get automatic allocation sanity checks and bounds accounting,
but without any release overhead from default initialization, allocation
checks, or unnecessary reallocations.

And since that's the natural pattern for appending a fixed number of
elements to a Vector, it doesn't really require any thought when writing
it. The safe and efficient approach is basically the default option.


nsTArray doesn't support emplace although it does have AppendElement(T&&),

but that wouldn't really help in this case. It's possible we could add
that
of course!

-e

On Tue, Aug 1, 2017 at 1:11 PM, Kris Maglione 
wrote:

On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:

nsTArray has various Append methods, in this case just using the

infallible
AppendElements w/o out a SetCapacity call would do the job. Another
option
would be to use SetLength which would default initialize the new
elements.

If we're trying to make things fast-but-safeish in this case, the
preferred
way is probably:

auto itr = AppendElements(length, fallible);
if (!itr) ...

// no bounds checking
for (...; i++, itr++)
   auto& mSocket = *itr;

// bounds checking
for (...)
auto& mSocket = *sockets[i];

In general I agree the pattern of fallibly allocating and then fallibly
appending w/o checking the return value is a bit silly. Perhaps we
should
just mark the fallible version MUST_USE? It looks like it's commented
out
for some reason (probably a ton of bustage).



Honestly, I think the Vector infallible* methods are a much cleaner way
to
handle this, especially when we need something like
infallibleEmplaceBack.
They tend to encourage writing more efficient code, with fewer
reallocations and allocation checks. And I think the resulting code tends
to be safer than AppendElements followed by unchecked raw pointer access,
placement `new`s, and an intermediate state where we have a bunch of
indexable but uninitialized elements in the array.

That's been my experience when reading and writing code using the various
approaches, anyway.


On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 


wrote:

On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:


I was recently searching through our codebase to look at all the ways
we


use fallible allocations, and was startled when I came across several
lines
that looked like this:

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

Re: Actually-Infallible Fallible Allocations

[Kris Maglione]

On Tue, Aug 01, 2017 at 05:16:54PM -0700, Jim Blandy wrote:

I have to ask: does anyone recall the benchmarks that showed that bounds
checks or vector reallocations were a measurable performance hit in this
code?

If we don't, we should just write simple, obviously correct code.

If we do, there should be a comment to that effect, or something to convey
to readers the performance constraints this code is trying to satisfy.


There doesn't have to be a conflict between simple, obviously correct, and 
fast here. The reserve() and infallibleAppend/infallibleEmplaceBack approach 
that the Vector class promotes (and is used extensively throughout 
Spidermonkey) is all three.



On Tue, Aug 1, 2017 at 2:10 PM, Kris Maglione  wrote:


On Tue, Aug 01, 2017 at 01:28:37PM -0700, Eric Rahm wrote:


Both AppendElements and SetLength default initialize the values, there's
no
intermediate uninitialzed state. SetCapacity doesn't initialize the
values,
but they're also not indexable -- we have release bounds assertions --
unless you try really hard.



For a lot of cases, default initialized isn't much better than completely
uninitialized. And it has an additional, unnecessary performance penalty
for the pattern that you suggested.

SetCapacity brings us back to the same problem where we either have
unnecessary allocation checks for every element we append, or we skip
sanity checks entirely and hope things stay sane if we refactor.

With the infallible*() methods of the Vector class, though, we don't have
to worry about any of that, and the code you suggested below becomes
something like:

 Vector sockets;
 if (!sockets.reserve(inputs.length()))
   return false;

 for (auto input : inputs)
   sockets.infallibleEmplaceBack(input);

and we still get automatic allocation sanity checks and bounds accounting,
but without any release overhead from default initialization, allocation
checks, or unnecessary reallocations.

And since that's the natural pattern for appending a fixed number of
elements to a Vector, it doesn't really require any thought when writing
it. The safe and efficient approach is basically the default option.


nsTArray doesn't support emplace although it does have AppendElement(T&&),

but that wouldn't really help in this case. It's possible we could add
that
of course!

-e

On Tue, Aug 1, 2017 at 1:11 PM, Kris Maglione 
wrote:

On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:


nsTArray has various Append methods, in this case just using the

infallible
AppendElements w/o out a SetCapacity call would do the job. Another
option
would be to use SetLength which would default initialize the new
elements.

If we're trying to make things fast-but-safeish in this case, the
preferred
way is probably:

auto itr = AppendElements(length, fallible);
if (!itr) ...

// no bounds checking
for (...; i++, itr++)
  auto& mSocket = *itr;

// bounds checking
for (...)
   auto& mSocket = *sockets[i];

In general I agree the pattern of fallibly allocating and then fallibly
appending w/o checking the return value is a bit silly. Perhaps we
should
just mark the fallible version MUST_USE? It looks like it's commented
out
for some reason (probably a ton of bustage).



Honestly, I think the Vector infallible* methods are a much cleaner way
to
handle this, especially when we need something like
infallibleEmplaceBack.
They tend to encourage writing more efficient code, with fewer
reallocations and allocation checks. And I think the resulting code tends
to be safer than AppendElements followed by unchecked raw pointer access,
placement `new`s, and an intermediate state where we have a bunch of
indexable but uninitialized elements in the array.

That's been my experience when reading and writing code using the various
approaches, anyway.


On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 


wrote:

On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:



I was recently searching through our codebase to look at all the ways
we


use fallible allocations, and was startled when I came across several
lines
that looked like this:

dom::SocketElement  = *sockets.AppendElement(fallible);
...
So in isolation this code is saying "I want to handle allocation
failure"
and then immediately not doing that and just dereferencing the result.
This
turns allocation failure into Undefined Behaviour, rather than a
process
abort.

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

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



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


The MFBT Vector class handles 

Re: Actually-Infallible Fallible Allocations

[Jim Blandy]

I have to ask: does anyone recall the benchmarks that showed that bounds
checks or vector reallocations were a measurable performance hit in this
code?

If we don't, we should just write simple, obviously correct code.

If we do, there should be a comment to that effect, or something to convey
to readers the performance constraints this code is trying to satisfy.

On Tue, Aug 1, 2017 at 2:10 PM, Kris Maglione  wrote:

> On Tue, Aug 01, 2017 at 01:28:37PM -0700, Eric Rahm wrote:
>
>> Both AppendElements and SetLength default initialize the values, there's
>> no
>> intermediate uninitialzed state. SetCapacity doesn't initialize the
>> values,
>> but they're also not indexable -- we have release bounds assertions --
>> unless you try really hard.
>>
>
> For a lot of cases, default initialized isn't much better than completely
> uninitialized. And it has an additional, unnecessary performance penalty
> for the pattern that you suggested.
>
> SetCapacity brings us back to the same problem where we either have
> unnecessary allocation checks for every element we append, or we skip
> sanity checks entirely and hope things stay sane if we refactor.
>
> With the infallible*() methods of the Vector class, though, we don't have
> to worry about any of that, and the code you suggested below becomes
> something like:
>
>  Vector sockets;
>  if (!sockets.reserve(inputs.length()))
>return false;
>
>  for (auto input : inputs)
>sockets.infallibleEmplaceBack(input);
>
> and we still get automatic allocation sanity checks and bounds accounting,
> but without any release overhead from default initialization, allocation
> checks, or unnecessary reallocations.
>
> And since that's the natural pattern for appending a fixed number of
> elements to a Vector, it doesn't really require any thought when writing
> it. The safe and efficient approach is basically the default option.
>
>
> nsTArray doesn't support emplace although it does have AppendElement(T&&),
>> but that wouldn't really help in this case. It's possible we could add
>> that
>> of course!
>>
>> -e
>>
>> On Tue, Aug 1, 2017 at 1:11 PM, Kris Maglione 
>> wrote:
>>
>> On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:
>>>
>>> nsTArray has various Append methods, in this case just using the
 infallible
 AppendElements w/o out a SetCapacity call would do the job. Another
 option
 would be to use SetLength which would default initialize the new
 elements.

 If we're trying to make things fast-but-safeish in this case, the
 preferred
 way is probably:

 auto itr = AppendElements(length, fallible);
 if (!itr) ...

 // no bounds checking
 for (...; i++, itr++)
   auto& mSocket = *itr;

 // bounds checking
 for (...)
auto& mSocket = *sockets[i];

 In general I agree the pattern of fallibly allocating and then fallibly
 appending w/o checking the return value is a bit silly. Perhaps we
 should
 just mark the fallible version MUST_USE? It looks like it's commented
 out
 for some reason (probably a ton of bustage).


>>> Honestly, I think the Vector infallible* methods are a much cleaner way
>>> to
>>> handle this, especially when we need something like
>>> infallibleEmplaceBack.
>>> They tend to encourage writing more efficient code, with fewer
>>> reallocations and allocation checks. And I think the resulting code tends
>>> to be safer than AppendElements followed by unchecked raw pointer access,
>>> placement `new`s, and an intermediate state where we have a bunch of
>>> indexable but uninitialized elements in the array.
>>>
>>> That's been my experience when reading and writing code using the various
>>> approaches, anyway.
>>>
>>>
>>> On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 
>>>
 wrote:

 On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:

>
> I was recently searching through our codebase to look at all the ways
> we
>
>> use fallible allocations, and was startled when I came across several
>> lines
>> that looked like this:
>>
>> dom::SocketElement  = *sockets.AppendElement(fallible);
>> ...
>> So in isolation this code is saying "I want to handle allocation
>> failure"
>> and then immediately not doing that and just dereferencing the result.
>> This
>> turns allocation failure into Undefined Behaviour, rather than a
>> process
>> abort.
>>
>> Thankfully, all the cases where I found this were preceded by
>> something
>> like the following:
>>
>> uint32_t length = socketData->mData.Length();if
>> (!sockets.SetCapacity(length, fallible)) {   JS_ReportOutOfMemory(cx);
>>  return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
>> socketData->mData.Length(); i++) {dom::SocketElement  =
>> *sockets.AppendElement(fallible);

Re: Actually-Infallible Fallible Allocations

[Kris Maglione]

On Tue, Aug 01, 2017 at 01:28:37PM -0700, Eric Rahm wrote:

Both AppendElements and SetLength default initialize the values, there's no
intermediate uninitialzed state. SetCapacity doesn't initialize the values,
but they're also not indexable -- we have release bounds assertions --
unless you try really hard.


For a lot of cases, default initialized isn't much better than 
completely uninitialized. And it has an additional, unnecessary 
performance penalty for the pattern that you suggested.


SetCapacity brings us back to the same problem where we either 
have unnecessary allocation checks for every element we append, 
or we skip sanity checks entirely and hope things stay sane if 
we refactor.


With the infallible*() methods of the Vector class, though, we 
don't have to worry about any of that, and the code you 
suggested below becomes something like:


 Vector sockets;
 if (!sockets.reserve(inputs.length()))
   return false;

 for (auto input : inputs)
   sockets.infallibleEmplaceBack(input);

and we still get automatic allocation sanity checks and bounds 
accounting, but without any release overhead from default 
initialization, allocation checks, or unnecessary reallocations.


And since that's the natural pattern for appending a fixed 
number of elements to a Vector, it doesn't really require any 
thought when writing it. The safe and efficient approach is 
basically the default option.



nsTArray doesn't support emplace although it does have AppendElement(T&&),
but that wouldn't really help in this case. It's possible we could add that
of course!

-e

On Tue, Aug 1, 2017 at 1:11 PM, Kris Maglione  wrote:


On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:


nsTArray has various Append methods, in this case just using the
infallible
AppendElements w/o out a SetCapacity call would do the job. Another option
would be to use SetLength which would default initialize the new elements.

If we're trying to make things fast-but-safeish in this case, the
preferred
way is probably:

auto itr = AppendElements(length, fallible);
if (!itr) ...

// no bounds checking
for (...; i++, itr++)
  auto& mSocket = *itr;

// bounds checking
for (...)
   auto& mSocket = *sockets[i];

In general I agree the pattern of fallibly allocating and then fallibly
appending w/o checking the return value is a bit silly. Perhaps we should
just mark the fallible version MUST_USE? It looks like it's commented out
for some reason (probably a ton of bustage).



Honestly, I think the Vector infallible* methods are a much cleaner way to
handle this, especially when we need something like infallibleEmplaceBack.
They tend to encourage writing more efficient code, with fewer
reallocations and allocation checks. And I think the resulting code tends
to be safer than AppendElements followed by unchecked raw pointer access,
placement `new`s, and an intermediate state where we have a bunch of
indexable but uninitialized elements in the array.

That's been my experience when reading and writing code using the various
approaches, anyway.


On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 

wrote:

On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:


I was recently searching through our codebase to look at all the ways we

use fallible allocations, and was startled when I came across several
lines
that looked like this:

dom::SocketElement  = *sockets.AppendElement(fallible);
...
So in isolation this code is saying "I want to handle allocation
failure"
and then immediately not doing that and just dereferencing the result.
This
turns allocation failure into Undefined Behaviour, rather than a process
abort.

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

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



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



The MFBT Vector class handles this with a set of infallibleAppend[1]
methods that assert that space has been reserved for the new elements
before they're called. If we're using this pattern elsewhere without the
same safeties, either those places should probably switch to using
Vectors,
or we should probably add similar checked infallible methods to nsTArray.


[1]: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b2092
9b56489e2e55438de81fa/mfbt/Vector.h#707-733

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



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

Science is interesting, and if you don't agree you can fuck off.
--Nigel 

Re: Actually-Infallible Fallible Allocations

[Eric Rahm]

Both AppendElements and SetLength default initialize the values, there's no
intermediate uninitialzed state. SetCapacity doesn't initialize the values,
but they're also not indexable -- we have release bounds assertions --
unless you try really hard.

nsTArray doesn't support emplace although it does have AppendElement(T&&),
but that wouldn't really help in this case. It's possible we could add that
of course!

-e

On Tue, Aug 1, 2017 at 1:11 PM, Kris Maglione  wrote:

> On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:
>
>> nsTArray has various Append methods, in this case just using the
>> infallible
>> AppendElements w/o out a SetCapacity call would do the job. Another option
>> would be to use SetLength which would default initialize the new elements.
>>
>> If we're trying to make things fast-but-safeish in this case, the
>> preferred
>> way is probably:
>>
>> auto itr = AppendElements(length, fallible);
>> if (!itr) ...
>>
>> // no bounds checking
>> for (...; i++, itr++)
>>   auto& mSocket = *itr;
>>
>> // bounds checking
>> for (...)
>>auto& mSocket = *sockets[i];
>>
>> In general I agree the pattern of fallibly allocating and then fallibly
>> appending w/o checking the return value is a bit silly. Perhaps we should
>> just mark the fallible version MUST_USE? It looks like it's commented out
>> for some reason (probably a ton of bustage).
>>
>
> Honestly, I think the Vector infallible* methods are a much cleaner way to
> handle this, especially when we need something like infallibleEmplaceBack.
> They tend to encourage writing more efficient code, with fewer
> reallocations and allocation checks. And I think the resulting code tends
> to be safer than AppendElements followed by unchecked raw pointer access,
> placement `new`s, and an intermediate state where we have a bunch of
> indexable but uninitialized elements in the array.
>
> That's been my experience when reading and writing code using the various
> approaches, anyway.
>
>
> On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 
>> wrote:
>>
>> On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:
>>>
>>> I was recently searching through our codebase to look at all the ways we
 use fallible allocations, and was startled when I came across several
 lines
 that looked like this:

 dom::SocketElement  = *sockets.AppendElement(fallible);
 ...
 So in isolation this code is saying "I want to handle allocation
 failure"
 and then immediately not doing that and just dereferencing the result.
 This
 turns allocation failure into Undefined Behaviour, rather than a process
 abort.

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

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



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


>>> The MFBT Vector class handles this with a set of infallibleAppend[1]
>>> methods that assert that space has been reserved for the new elements
>>> before they're called. If we're using this pattern elsewhere without the
>>> same safeties, either those places should probably switch to using
>>> Vectors,
>>> or we should probably add similar checked infallible methods to nsTArray.
>>>
>>>
>>> [1]: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b2092
>>> 9b56489e2e55438de81fa/mfbt/Vector.h#707-733
>>>
>>> ___
>>> dev-platform mailing list
>>> dev-platform@lists.mozilla.org
>>> https://lists.mozilla.org/listinfo/dev-platform
>>>
>>>
> --
> Kris Maglione
> Senior Firefox Add-ons Engineer
> Mozilla Corporation
>
> Science is interesting, and if you don't agree you can fuck off.
> --Nigel Calder, former editor of New Scientist
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Actually-Infallible Fallible Allocations

[Kris Maglione]

On Tue, Aug 01, 2017 at 12:57:31PM -0700, Eric Rahm wrote:

nsTArray has various Append methods, in this case just using the infallible
AppendElements w/o out a SetCapacity call would do the job. Another option
would be to use SetLength which would default initialize the new elements.

If we're trying to make things fast-but-safeish in this case, the preferred
way is probably:

auto itr = AppendElements(length, fallible);
if (!itr) ...

// no bounds checking
for (...; i++, itr++)
  auto& mSocket = *itr;

// bounds checking
for (...)
   auto& mSocket = *sockets[i];

In general I agree the pattern of fallibly allocating and then fallibly
appending w/o checking the return value is a bit silly. Perhaps we should
just mark the fallible version MUST_USE? It looks like it's commented out
for some reason (probably a ton of bustage).


Honestly, I think the Vector infallible* methods are a much cleaner way to 
handle this, especially when we need something like infallibleEmplaceBack. 
They tend to encourage writing more efficient code, with fewer reallocations 
and allocation checks. And I think the resulting code tends to be safer than 
AppendElements followed by unchecked raw pointer access, placement `new`s, and 
an intermediate state where we have a bunch of indexable but uninitialized 
elements in the array.


That's been my experience when reading and writing code using the various 
approaches, anyway.



On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 
wrote:


On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:


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

dom::SocketElement  = *sockets.AppendElement(fallible);
...
So in isolation this code is saying "I want to handle allocation failure"
and then immediately not doing that and just dereferencing the result.
This
turns allocation failure into Undefined Behaviour, rather than a process
abort.

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

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



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



The MFBT Vector class handles this with a set of infallibleAppend[1]
methods that assert that space has been reserved for the new elements
before they're called. If we're using this pattern elsewhere without the
same safeties, either those places should probably switch to using Vectors,
or we should probably add similar checked infallible methods to nsTArray.


[1]: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b2092
9b56489e2e55438de81fa/mfbt/Vector.h#707-733

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



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

Science is interesting, and if you don't agree you can fuck off.
--Nigel Calder, former editor of New Scientist

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

Re: Actually-Infallible Fallible Allocations

[Eric Rahm]

nsTArray has various Append methods, in this case just using the infallible
AppendElements w/o out a SetCapacity call would do the job. Another option
would be to use SetLength which would default initialize the new elements.

If we're trying to make things fast-but-safeish in this case, the preferred
way is probably:

auto itr = AppendElements(length, fallible);
if (!itr) ...

// no bounds checking
for (...; i++, itr++)
   auto& mSocket = *itr;

// bounds checking
for (...)
auto& mSocket = *sockets[i];

In general I agree the pattern of fallibly allocating and then fallibly
appending w/o checking the return value is a bit silly. Perhaps we should
just mark the fallible version MUST_USE? It looks like it's commented out
for some reason (probably a ton of bustage).

-e

On Tue, Aug 1, 2017 at 12:18 PM, Kris Maglione 
wrote:

> On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:
>
>> I was recently searching through our codebase to look at all the ways we
>> use fallible allocations, and was startled when I came across several
>> lines
>> that looked like this:
>>
>> dom::SocketElement  = *sockets.AppendElement(fallible);
>> ...
>> So in isolation this code is saying "I want to handle allocation failure"
>> and then immediately not doing that and just dereferencing the result.
>> This
>> turns allocation failure into Undefined Behaviour, rather than a process
>> abort.
>>
>> Thankfully, all the cases where I found this were preceded by something
>> like the following:
>>
>> uint32_t length = socketData->mData.Length();if
>> (!sockets.SetCapacity(length, fallible)) {   JS_ReportOutOfMemory(cx);
>>  return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
>> socketData->mData.Length(); i++) {dom::SocketElement  =
>> *sockets.AppendElement(fallible);
>>
>>
>>
>> So really, the fallible AppendElement *is* infallible, but we're just
>> saying "fallible" anyway. I find this pattern concerning for two reasons:
>>
>
> The MFBT Vector class handles this with a set of infallibleAppend[1]
> methods that assert that space has been reserved for the new elements
> before they're called. If we're using this pattern elsewhere without the
> same safeties, either those places should probably switch to using Vectors,
> or we should probably add similar checked infallible methods to nsTArray.
>
>
> [1]: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b2092
> 9b56489e2e55438de81fa/mfbt/Vector.h#707-733
>
> ___
> 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: Actually-Infallible Fallible Allocations

[Kris Maglione]

On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:

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

dom::SocketElement  = *sockets.AppendElement(fallible);
...
So in isolation this code is saying "I want to handle allocation failure"
and then immediately not doing that and just dereferencing the result. This
turns allocation failure into Undefined Behaviour, rather than a process
abort.

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

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



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


The MFBT Vector class handles this with a set of infallibleAppend[1] methods 
that assert that space has been reserved for the new elements before they're 
called. If we're using this pattern elsewhere without the same safeties, 
either those places should probably switch to using Vectors, or we should 
probably add similar checked infallible methods to nsTArray.



[1]: 
http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mfbt/Vector.h#707-733
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Actually-Infallible Fallible Allocations

[David Major]

I don't think that anyone deliberately set out to write the code this way.
Likely this is fallout from the mass-refactorings in bug 968520 and related
bugs. I'd recommend working with poiru and froydnj to see if there's any
automated follow-up we could do to remove/improve this pattern.

On Tue, Aug 1, 2017 at 12:31 PM, Alexis Beingessner  wrote:

> TL;DR: we're using fallible allocations without checking the result in
> cases where we're certain there is enough space already reserved. This is
> confusing and potentially dangerous for refactoring. Should we stop doing
> this?
>
> -
>
> I was recently searching through our codebase to look at all the ways we
> use fallible allocations, and was startled when I came across several lines
> that looked like this:
>
> dom::SocketElement  = *sockets.AppendElement(fallible);
>
> For those who aren't familiar with how our allocating APIs work:
>
> * by default we hard abort on allocation failure
> * but if you add `fallible` (or use the Fallible template), you will get
> back nullptr on allocation failure
>
> So in isolation this code is saying "I want to handle allocation failure"
> and then immediately not doing that and just dereferencing the result. This
> turns allocation failure into Undefined Behaviour, rather than a process
> abort.
>
> Thankfully, all the cases where I found this were preceded by something
> like the following:
>
> uint32_t length = socketData->mData.Length();if
> (!sockets.SetCapacity(length, fallible)) {   JS_ReportOutOfMemory(cx);
>   return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
> socketData->mData.Length(); i++) {dom::SocketElement  =
> *sockets.AppendElement(fallible);
>
>
>
> So really, the fallible AppendElement *is* infallible, but we're just
> saying "fallible" anyway. I find this pattern concerning for two reasons:
>
> * It's confusing to anyone who encounters this for the first time.
> * It's a time bomb for any bad refactoring which makes the allocations
> *actually* fallible
>
> I can however think of two reasons why this might be desirable
>
> * It encourages the optimizer to understand that the allocations can't
> fail, removing branches (dubious, these branches would predict perfectly if
> they exist at all)
>
> * It's part of a guideline/lint that mandates only fallible allocations be
> used in this part of the code.
>
> If the latter is a concern, I would much rather we use infallible
> allocations with a required comment, or some other kind of decoration to
> state "this is fine". In the absence of a checker that can statically prove
> the the AppendElement calls will never fail (which in the given example
> would in fact warn that we don't use `length` in the loop condition), I
> would rather mistakes lead to us crashing more, rather than Undefined
> Behaviour.
>
> Thoughts?
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform