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 <kmagli...@mozilla.com> 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<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
--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation
Wanting to meet an author because you like his work is like wanting to
meet a duck because you like paté.
--Margaret Atwood
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
