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<Socket> 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 <kmagli...@mozilla.com> 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 <kmagli...@mozilla.com>
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 &mSocket = *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 &mSocket =
*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
--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation
I'm confident that tomorrow's Unix will look like today's Unix, only
cruftier.
--Russ Cox
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
