The simplistic shift-based hashing function creates a lot of collisions
for pointers pointing to arrays as it doesn't do a great job at distributing
the data randomly based on the input bytes.
Running with XPCOM_MEM_LOG_CLASSES=nsStringBuffer triggers an
assertion in refcount logging for nsFakeStringBuffers. These are given
an initial refcount of 1, without calling NS_LOG_ADDREF. Then,
AddRef() is called on these objects in StaticAtom::StaticAtom(), and
we tell the refcount logging system about the fake buffer, and that it
has a refcount of 0, triggering the assertion.
The first part of the fix is to call NS_LOG_ADDREF for this initial
refcount, in StaticAtom().
This first fix causes refcount logging to start reporting that the
fake string buffers leak, when XPCOM_MEM_LOG_CLASSES is not set. This
is because refcount logging is now getting told about these objects
being AddRefed at 1, which it takes to mean that an object is created.
To work around this issue, I add an array gFakeBuffers that contains
every fake string buffer we create, and tell the refcount logging
system that these objects are all being destroyed, when the atom table
is being shut down. This could result in some bogosity if the fake
buffers are "leaked" but hopefully this is still an improvement over
the current state.
MozReview-Commit-ID: 5AxoBYAlYRU
--HG--
extra : rebase_source : ba0763cb494894918141774025db525cea9f9c75
Running with XPCOM_MEM_LOG_CLASSES=nsStringBuffer triggers an
assertion in refcount logging for nsFakeStringBuffers. These are given
an initial refcount of 1, without calling NS_LOG_ADDREF. Then,
AddRef() is called on these objects in StaticAtom::StaticAtom(), and
we tell the refcount logging system about the fake buffer, and that it
has a refcount of 0, triggering the assertion.
The first part of the fix is to call NS_LOG_ADDREF for this initial
refcount, in StaticAtom().
This first fix causes refcount logging to start reporting that the
fake string buffers leak, when XPCOM_MEM_LOG_CLASSES is not set. This
is because refcount logging is now getting told about these objects
being AddRefed at 1, which it takes to mean that an object is created.
To work around this issue, I add an array gFakeBuffers that contains
every fake string buffer we create, and tell the refcount logging
system that these objects are all being destroyed, when the atom table
is being shut down. This could result in some bogosity if the fake
buffers are "leaked" but hopefully this is still an improvement over
the current state.
MozReview-Commit-ID: 5AxoBYAlYRU
--HG--
extra : rebase_source : 967520e825d26c369a11acc478d223d190137a43
This patch makes the following changes to the macros.
- Removes PROFILER_LABEL_FUNC. It's only suitable for use in functions outside
classes, due to PROFILER_FUNCTION_NAME not getting class names, and it was
mostly misused.
- Removes PROFILER_FUNCTION_NAME. It's no longer used, and __func__ is
universally available now anyway.
- Combines the first two string literal arguments of PROFILER_LABEL and
PROFILER_LABEL_DYNAMIC into a single argument. There was no good reason for
them to be separate, and it forced a '::' in the label, which isn't always
appropriate. Also, the meaning of the "name_space" argument was interpreted
in an interesting variety of ways.
- Adds an "AUTO_" prefix to PROFILER_LABEL and PROFILER_LABEL_DYNAMIC, to make
it clearer they construct RAII objects rather than just being function calls.
(I myself have screwed up the scoping because of this in the past.)
- Fills in the 'js::ProfileEntry::Category::' qualifier within the macro, so
the caller doesn't need to. This makes a *lot* more of the uses fit onto a
single line.
The patch also makes the following changes to the macro uses (beyond those
required by the changes described above).
- Fixes a bunch of labels that had gotten out of sync with the name of the
class and/or function that encloses them.
- Removes a useless PROFILER_LABEL use within a trivial scope in
EventStateManager::DispatchMouseOrPointerEvent(). It clearly wasn't serving
any useful purpose. It also serves as extra evidence that the AUTO_ prefix is
a good idea.
- Tweaks DecodePool::SyncRunIf{Preferred,Possible} so that the labelling is
done within them, instead of at their callsites, because that's a more
standard way of doing things.
--HG--
extra : rebase_source : 318d1bc6fc1425a94aacbf489dd46e4f83211de4
All the instances are converted as follows.
- nsSubstring --> nsAString
- nsCSubstring --> nsACString
--HG--
extra : rebase_source : cfd2238c52e3cb4d13e3bd5ddb80ba6584ab6d91
All the instances are converted as follows.
- nsAFlatString --> nsString
- nsAFlatCString --> nsCString
--HG--
extra : rebase_source : b37350642c58a85a08363df2e7c610873faa6e41
Also change nsClassHashtable::LookupOrAdd to not regard existing entries with
a nullptr value as non-existent. This is to make it consistent with
nsBaseHashtable::LookupForAdd() and other methods.
MozReview-Commit-ID: 1wYqK8XQbyW
We already have a handle to the entry in the hashtable, we can remove
that directly, rather than implicitly looking up the entry to remove in
Remove().
PLDHashTable takes the result of the hash function and multiplies it by
kGoldenRatio to ensure that it has a good distribution of bits across
the 32-bit hash value, and then zeroes out the low bit so that it can be
used for the collision flag. This result is called hash0. From hash0
it computes two different numbers used to find entries in the table
storage: hash1 is used to find an initial position in the table to
begin searching for an entry; hash2 is then used to repeatedly offset
that position (mod the size of the table) to build a chain of positions
to search.
In a table with capacity 2^c entries, hash1 is simply the upper c bits
of hash0. This patch does not change this.
Prior to this patch, hash2 was the c bits below hash1, padded at the low
end with zeroes when c > 16. (Note that bug 927705, changeset
1a02bec165e16f370cace3da21bb2b377a0a7242, increased the maximum capacity
from 2^23 to 2^26 since 2^23 was sometimes insufficient!) This manner
of computing hash2 is problematic because it increases the risk of long
chains for very large tables, since there is less variation in the hash2
result due to the zero padding.
So this patch changes the hash2 computation by using the low bits of
hash0 instead of shifting it around, thus avoiding 0 bits in parts of
the hash2 value that are significant.
Note that this changes what hash2 is in all cases except when the table
capacity is exactly 2^16, so it does change our hashing characteristics.
For tables with capacity less than 2^16, it should be using a different
second hash, but with the same amount of random-ish data. For tables
with capacity greater than 2^16, it should be using more random-ish
data.
Note that this patch depends on the patch for bug 1353458 in order to
avoid causing test failures.
MozReview-Commit-ID: JvnxAMBY711
--HG--
extra : transplant_source : 2%D2%C2%CE%E1%92%C8%F8H%D7%15%A4%86%5B%3Ac%0B%08%3DA
PLDHashTable's entry store has two types of unoccupied entries: free
entries and removed entries. The search of a chain of entries
(determined by the hash value) in the entry store to search for an entry
can stop at free entries, but it continues across removed entries,
because removed entries are entries that may have been skipped over when
we were adding the value we're searching for to the hash, but have since
been removed. For live entries, we also maintain this distinction by
using one bit of storage for a collision flag, which notes that if the
hashtable entry is removed, its place in the entry store must become a
removed entry rather than a free entry.
When we add a new entry to the table, Add's semantics require that we
return an existing entry if there is one, and only create a new entry if
no existing entry exists. (Bug 1352198 suggests the possibility of a
faster alternative Add API where the caller guarantees that the key is
not already in the hashtable.) When we search for the existing entry,
we must thus continue the search across removed entries, even though we
record the first removed entry found to return if the search for an
existing entry fails.
The existing code adds the collision flag through the entire table
search during an Add. This patch changes that behavior so that we only
add the collision flag prior to finding the first removed entry. Adding
it after we find the first removed entry is unnecessary, since we are
not making that entry part of a path to a new entry. If it is part of a
path to an existing entry, it will already have the collision flag set.
This patch effectively puts an if (!firstRemoved) around the else branch
of the if (MOZ_UNLIKELY(EntryIsRemoved(entry))), and then refactors that
condition outwards since it is now around the contents of both the if
and else branches.
MozReview-Commit-ID: CsXnMYttHVy
--HG--
extra : transplant_source : %80%9E%83%EC%CCY%B4%B0%86%86%18%99%B6U%21o%5D%29%AD%04
The OrInsert() method adds the new entry to the hashtable if needed, and
returns the newly added entry or the pre-existing one. It allows for a
more concise syntax at the call site.
PLDHashTable takes the result of the hash function and multiplies it by
kGoldenRatio to ensure that it has a good distribution of bits across
the 32-bit hash value, and then zeroes out the low bit so that it can be
used for the collision flag. This result is called hash0. From hash0
it computes two different numbers used to find entries in the table
storage: hash1 is used to find an initial position in the table to
begin searching for an entry; hash2 is then used to repeatedly offset
that position (mod the size of the table) to build a chain of positions
to search.
In a table with capacity 2^c entries, hash1 is simply the upper c bits
of hash0. This patch does not change this.
Prior to this patch, hash2 was the c bits below hash1, padded at the low
end with zeroes when c > 16. (Note that bug 927705, changeset
1a02bec165e16f370cace3da21bb2b377a0a7242, increased the maximum capacity
from 2^23 to 2^26 since 2^23 was sometimes insufficient!) This manner
of computing hash2 is problematic because it increases the risk of long
chains for very large tables, since there is less variation in the hash2
result due to the zero padding.
So this patch changes the hash2 computation by using the low bits of
hash0 instead of shifting it around, thus avoiding 0 bits in parts of
the hash2 value that are significant.
Note that this changes what hash2 is in all cases except when the table
capacity is exactly 2^16, so it does change our hashing characteristics.
For tables with capacity less than 2^16, it should be using a different
second hash, but with the same amount of random-ish data. For tables
with capacity greater than 2^16, it should be using more random-ish
data.
Note that this patch depends on the patch for bug 1353458 in order to
avoid causing test failures.
MozReview-Commit-ID: JvnxAMBY711
--HG--
extra : rebase_source : dbde93b9312dd10fb3a549ee4098b57e1df5cadf
This removes the last uses of PR_smprintf from the tree (excluding the
security and nsprpub directories). It also fixes a related latent bug
in nsAppRunner.cpp (which was incorrectly freeing the pointer passed to
PR_SetEnv).
MozReview-Commit-ID: GynP2PhuWWO
--HG--
extra : rebase_source : c3b83c7bd08b1c222e137a00323caf5481352845
Ehsan Akhgari
Andrew McCreight
Wes Kocher
Sylvestre Ledru
Mats Palmgren
Sebastian Hengst
Bill McCloskey
Nicholas Nethercote
Carsten "Tomcat" Book
Florian Quèze
Eric Rahm
Michael Layzell
Nathan Froyd
Daniel Holbert
L. David Baron
Andreas Farre
Samael Wang
Ehsan Akhgari
Tom Tromey