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