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 <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
_______________________________________________
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
