Entry storage allocation now occurs on the first lookupForAdd()/put()/putNew().
This removes the need for init() and initialized(), and matches how
PLDHashTable/nsTHashtable work. It also removes the need for init() functions
in a lot of types that are built on top of mozilla::Hash{Map,Set}.
Pros:
- No need for init() calls and subsequent checks.
- No memory allocated for empty tables, which are not that uncommon.
Cons:
- An extra branch in lookup() and lookupForAdd(), but not in put()/putNew(),
because the existing checkOverloaded() can handle it.
Specifics:
- Construction now can take a length parameter.
- init() is removed. Explicit length-setting, when necessary, now occurs in the
constructors.
- initialized() is removed.
- capacity() now returns zero when the entry storage is absent.
- lookupForAdd() is no longer `const`, because it can instantiate the storage,
which requires modifications.
- lookupForAdd() can now return an invalid AddPtr in two cases:
- old: hashing failure (due to OOM in the hasher)
- new: OOM while instantiating entry storage
The existing failure handling paths for the old case work for the new case.
- clear(), finish(), and clearAndShrink() are replaced by clear(), compact(),
and reserve(). The old compactIfUnderloaded() is also removed.
- Capacity computation code is now in its own functions, bestCapacity() and
hashShift(). setTableSizeLog2() is removed.
- uint32_t is used throughout for capacities, instead of size_t, for
consistency with other similar values.
- changeTableSize() now takes a capacity instead of a deltaLog2, and it can now
handle !mTable.
Measurements:
- Total source code size is reduced by over 900 lines. Also, lots of existing
lines got shorter (i.e. two checks were reduced to one).
- Executable size barely changed, down by 2 KiB on Linux64. The extra branches
are compensated for by the lack of init() calls.
- Speed changed negligibly. The instruction count for Bench_Cpp_MozHash
increased from 2.84 billion to 2.89 billion but any execution time change was
well below noise.
We're currently using NDK r15c, which is rather old, and happens to come
with a buggy gold linker. Let's use a more recent NDK, with a fixed
linker.
Unfortunately, we're currently at NDK API level 9, which the newer NDK
doesn't provide for x86 anymore. But that corresponds to Gingerbread
(2.3), which we've long stopped supporting. On the SDK side, we already
dropped support of versions before Jelly Bean, so we can do the same on
the NDK side. That corresponds to API level 16. So let's just use that
as a baseline.
Another change in the newer NDK is that the target-name changed from
i386-linux-android to i686-linux-android, so adjust for that in the
android x86 mozconfigs.
In PLDHashTable the equivalent functions have a "Shallow" prefix, which makes
it clear that they don't measure things hanging off the table. This patch makes
mozilla::Hash{Set,Map} do likewise.
MozReview-Commit-ID: 3kwCJynhW7d
--HG--
extra : rebase_source : 9c03d11f376a9fd4cfd5cfcdc0c446c00633b210
Also use mozilla::HashNumber where appropriate.
MozReview-Commit-ID: BTq0XDS5UfQ
--HG--
extra : rebase_source : 28f45a9b27e831e99620a2b575f373003f1301f2
Summary: GTest is permafailing on Windows because of timeout.
Reviewers: glandium
Reviewed By: glandium
Bug #: 1474254
Differential Revision: https://phabricator.services.mozilla.com/D2043
--HG--
extra : rebase_source : cea68e50f96a1788bf15dc6ca7859e5f698a6209
This adds an '--allocation-filter' param that can be used to limit output to
only allocations that include the filter in their stack. For example:
dmd.py --allocation-filter fontconfig dmd.json.gz
limits its output to just allocations that have an instance of 'fontconfig' in
in one of their stack frames.
--HG--
extra : rebase_source : 9ed34d5c7a6a2b76e96455e66b2e8196686ade16
This was done automatically replacing:
s/mozilla::Move/std::move/
s/ Move(/ std::move(/
s/(Move(/(std::move(/
Removing the 'using mozilla::Move;' lines.
And then with a few manual fixups, see the bug for the split series..
MozReview-Commit-ID: Jxze3adipUh
Bug 1364359 is to fix a leaked arena. Until that is fixed; it is unsafe to
call moz_dispose_arena more than once.
MozReview-Commit-ID: KIby1RLtrPK
--HG--
extra : rebase_source : 6ea41001e9f0c4d5eb24ee678d6c1c0218991ac3
This behavior matches what gets used in mozalloc.h to define these
wrappers, and is particularly necessary for newer versions of clang to
not complain about our definitions.
That NDK bug has been fixed since r8c, and we now require something more
recent than that. This effectively reverts the changes from bug 720621
and bug 734832.
--HG--
extra : rebase_source : 9ff76a790ec4135dc0172cfd0f11fc1ecef7df64
Base allocator commit stats were added in bug 515556, along other commit
stats, but they have actually been wrong since then: the committed count
is updated with the difference between pbase_next_addr and
base_next_decommitted *after* the latter is set to the former, making
the difference always 0.
--HG--
extra : rebase_source : a2aed523314549a37a61bd4ab300c98f198f9252
Since TreeNode::{Left,Right,Color} is always a valid call to make, we
don't need to check if for nullity before calling those functions.
This effectively kind of reverts some parts of bug 1412722.
--HG--
extra : rebase_source : 3deb316f463b51fdbb3aebc2e57e437018b3a829
The code before bug 1412722 benefitted from the sentinel being an actual
node object, and some code paths ended up checking its color (always
black) or getting its right and left node, that always returned to the
sentinel.
When TreeNode currently contains a nullptr, all those lead to a null
deref if the calling code is not doing the right checks, which happens
to be the case in at least some places. Instead of relying on the
callers doing the right thing, make the TreeNode do the right thing when
it contains a nullptr, effectively acting as the sentinel in that case.
We additionally ensure that nothing in the calling code will be trying
to change the color or left/right pointers on the sentinel, which is an
extra safe net compared to the code before bug 1412722.
--HG--
extra : rebase_source : 09ab0bf8682092ef6d9a0a5921be3da787d0d548
This will allow the upcoming changes to add some safety back to the code
after bug 1412722.
--HG--
extra : rebase_source : 5033b8034cabaf5a7fdd578459588d5099402d02
Since TreeNode::{Left,Right,Color} is always a valid call to make, we
don't need to check if for nullity before calling those functions.
This effectively kind of reverts some parts of bug 1412722.
--HG--
extra : rebase_source : 172f1c042bdbb4d500e1afb4d57774ab76826876
The code before bug 1412722 benefitted from the sentinel being an actual
node object, and some code paths ended up checking its color (always
black) or getting its right and left node, that always returned to the
sentinel.
When TreeNode currently contains a nullptr, all those lead to a null
deref if the calling code is not doing the right checks, which happens
to be the case in at least some places. Instead of relying on the
callers doing the right thing, make the TreeNode do the right thing when
it contains a nullptr, effectively acting as the sentinel in that case.
We additionally ensure that nothing in the calling code will be trying
to change the color or left/right pointers on the sentinel, which is an
extra safe net compared to the code before bug 1412722.
--HG--
extra : rebase_source : ac61ea259ac49bf76e2f8f6f54dda991498d4664
This will allow the upcoming changes to add some safety back to the code
after bug 1412722.
--HG--
extra : rebase_source : c906e9b3168fe738cba8a3de3fdf4efee1f0d4df
- Turn MOZ_DIAGNOSTIC_ASSERTs related to double-frees into
MOZ_RELEASE_ASSERTs with a crash message making them more identifiable
than the asserted condition.
- In huge_dalloc, MOZ_RELEASE_ASSERT early, instead of letting
RedBlackTree::Remove end up crashing because the node is not in the
tree.
--HG--
extra : rebase_source : e051caaf371e88a9db6b5153f58c8a4aa4cde757
Before bug 1412722, which removed the sentinels, the code looked like:
if (rbp_r_c->Right()->Left()->IsBlack()) {
At that point in the code, rbp_r_c is the root node of the tree. If
rbp_r_c->Right() was the sentinel, ->Right()->Left() would be the
sentinel too, and the sentinel is black. Which means the condition would
be true.
The code after was:
if (rbp_r_c->Right() && (!rbp_r_c->Right()->Left() ||
rbp_r_c->Right()->Left()->IsBlack())) {
The second half correctly deals with the case of
rbp_r_c->Right()->Left() being the sentinel. But the first half now
makes things different: ->Right() being null would correspond to the
previous case where it was the sentinel, and the test would not return
true in that case when it would have before. When ->Right() is not null,
things are normal again.
The correct check is to make the branch taken when ->Right() is null.
Now, looking under which conditions we may get in that branch wrongly:
- The root node's right link must be empty, which means a very small tree.
- The comparison between the removed key and the root node must indicate
the key is greater than the value of the root node.
- There's another case where the comparison result (rbp_r_cmp) can be
eGreater, when it is reassigned under one of the branches under the
eEqual test, and that branch is only taken when ->Right() on the root
node was non-null, which was the non-broken case.
So it would seem we can't reach that code when rbp_r_c->Right() is null
anyways, so it /should/ practically make no difference. Better safe than
sorry, though. It's hard to tell anything from crash stats, because
since the templatization in bug 1403444, all crashes fit in one bucket,
when there used to be 5 functions before :(
While here, add a missing include in rb.h.
--HG--
extra : rebase_source : 2ebcb84345ad52059b0c081b9e2e1af1d0bbb7bc
