Never store names in Message. One can get string names from
Message::name() or use IPC::StringFromIPCMessageType() when only
message id is available.
MozReview-Commit-ID: 15ksx6SE90c
--HG--
extra : rebase_source : 1a041dc365b7f42edd540d8c7a4dfd8912e48921
Each protocol in IPDL has a bunch of autogenerated functions that
instantiate IPC::Message with various parameters. Each of these
functions, then:
1) Pays the cost of calling malloc()
2) Setting up various parameters
3) Calling IPC::Message()
There's no reason that we should be duplicating 1) across all of these
autogenerated functions. In step 2), several of the parameters we're
setting up are common across all or nearly all calls: the message
segment size is almost always zero, and we're always indicating that
IPDL-generated messages should be recorded in telemetry.
Instead of duplicating that code several thousand times, we can add a
small helper function that takes the only interesting parameters for an
IPDL message. This helper function can then deal with calling malloc in
a single place and setting up the common parameters. For messages that
require a custom segment size, we'll have to use the old scheme, but
such messages are uncommon.
The previous changes are not required for this scheme to work, but they
do help significantly, as the helper function (Message::IPDLMessage) can
now take four parameters, which ensures that its arguments are passed
solely in registers on Win64 and ARM. The wins from this change are
also larger than they would be without the previous parts: ~100K on
x86-64 Linux (!) and ~80K on ARM Android.
The current IPC::Message constructor takes a large number of arguments,
three of which--the nesting level, the priority, and the
compression--are almost always constant by virtue of the vast majority
of Message construction being done by auto-generated IPDL code. But
then we take these constant values into the Message constructor, we
check them for various values, and then based on those values, we
perform a bunch of bitfield operations to store flags based on those
values. This is wasted work.
Furthermore, for replies to IPDL messages, we'll construct a Message
object, and then call mutating setters on the Message object that will
perform even more bitfield manipulations. Again, these operations are
performing tasks at runtime that are the same every single time, and use
information we already have at compile time.
The impact of these extra operations is not large, maybe 15-30K of extra
code, depending on platform. Nonetheless, we can easily make them go
away, and make everything cleaner to boot.
This patch adds a HeaderFlags class that encapsulates all the knowledge
about the various kinds of flags Message needs to know about. We can
construct HeaderFlags objects with strongly-typed enum arguments for the
various kinds of flags, and the compiler can take care of folding all of
those flags together into a constant when possible (and it is possible
for all the IPDL-generated code that instantiates Messages). The upshot
is that we do no unnecessary work in the Message constructor itself. We
can also remove various mutating operations on Message, as those
operations were only there to support post-constructor flag twiddling,
which is no longer necessary.
Since LinearHistogram and its descendants inherit ranges_ from
Histogram, and we wanted to replace the copying into a std::vec
for Histogram, the simplest approach seemed to just be to
precompute ranges for all histograms, exponential or otherwise.
This should have the added benefit of reducing the memory
footprint for those histograms, since they will benefit from the
deduplication work that the precomputing script already does.
MozReview-Commit-ID: JTV5Dej5ZIb
--HG--
extra : rebase_source : de942d54b3475be54c70d43d2fa8e772ee2e18c4
We should not be declaring forward declarations for nsString classes directly,
instead we should use nsStringFwd.h. This will make changing the underlying
types easier.
--HG--
extra : rebase_source : b2c7554e8632f078167ff2f609392e63a136c299
We were using std::vector::assign, which resizes the vector to
match the incoming data. This isn't what we want, as ranges_ has
already been sized to bucket_count_ + 1. Instead, just use a
copy.
MozReview-Commit-ID: EGuW5jj7Rpq
--HG--
extra : rebase_source : 616d61fc27c7e43c22ea69e11e070ba958bf20a9
There are two problems related with EVENT__SIZEOF_OFF_T:
- When building Firefox with -D_FILE_OFFSET_BITS=64, off_t is 64 bits,
but the in-tree event-config.h still defines EVENT__SIZEOF_OFF_T to 4.
- When building Firefox *without* -D_FILE_OFFSET_BITS=64 (the default)
against a system libevent that was built with -D_FILE_OFFSET_BITS=64,
its event-config.h defines EVENT__SIZEOF_OFF_T to 8, which then
doesn't match off_t size.
For the latter, libevent actually defines its own off_t type, that
callers are supposed to use instead of off_t. So that's what our
static_assert should be checking.
--HG--
extra : rebase_source : 4231530e3c260b2cdd53e15206d48ef0779e394c
The log and exp calls in base::Histogram::InitializeBucketRange()
were showing up in profiles. This patch uses the precomputed
buckets for exponential histograms instead of computing them at
runtime. Though linear histograms do show up in the profile that
prompted this change, they contribute much less, and due to the
trivial nature of generating these, it's unlikely that a static
cache would provide much if any speedup.
MozReview-Commit-ID: IavFwoWjFhk
--HG--
extra : rebase_source : ad7d641ab2982f5cf8d202c7c382bfc26daa4bd5
The log and exp calls in base::Histogram::InitializeBucketRange()
were showing up in profiles. This patch uses the precomputed
buckets for exponential histograms instead of computing them at
runtime. Though linear histograms do show up in the profile that
prompted this change, they contribute much less, and due to the
trivial nature of generating these, it's unlikely that a static
cache would provide much if any speedup.
MozReview-Commit-ID: IavFwoWjFhk
--HG--
extra : rebase_source : 18101da322faf9477acae266e9e27f579464f8d0
Having these functions declared in the class definition and therefore
inlined means that every call site is bloated by having to store the
argument so its address can be taken and load the sizeof() constant.
There's no good reason that we should be doing this; the Read*
counterparts are also out-of-lined, which hasn't seemed to cause any
problems. Moving these out-of-line saves about 200K (!) of space on
x86-64 Linux.
Previously we used the base::StatisticsRecorder object for storage by name.
This is keyed by histogram name, which doesn't match our storage reality anymore.
Instead we use a name to refer to a set of histogram instances that record data from different processes, as well as separating session and subsession data.
In this re-write, we instead introduce the following lookup paths (managed in TelemetryHistogram.cpp):
- Main storage:
- (histogramId, processId, sessionOrSubsession) -> Histogram*
- (histogramId, processId) -> KeyedHistogram* (this handles subsessions internally)
- Lookup:
- (histogramName) -> histogramId
- (HistogramID) -> bool (is recording enabled for this histogram?)
This is wrapped with a few lookup functions.
This also allows us to keep HistogramIDs in the JS histogram instances now, instead of pointers to Histogram instances.
That means Histogram instance life-time management is now properly contained inside TelemetryHistogram.cpp.
MozReview-Commit-ID: 5yijGv7mc89
- A histogram name identifies a set of histogram instances, for which storage and lookup will be handled in TelemetryHistogram.cpp.
So we remove the names from histogram code.
- Various unused macros in the header are removed.
- Remaining traces of StatisticsRecorder are removed from the Histogram class code.
- Some unused methods are dropped that were about printing histograms to ASCII etc.
MozReview-Commit-ID: BF2rLSpKOJ8
The Chromium IPC histogram code used the StatisticsRecorder object for storage.
This is keyed by histogram name, which doesn't match our storage reality anymore.
Instead we use a name to refer to a set of histogram instances that record data from different processes, as well as separating session and subsession data.
Consequently we need to rewrite this storage, which means StatisticsRecorder is not used anymore.
MozReview-Commit-ID: 1LC7YubpKaD
Previously we used the base::StatisticsRecorder object for storage by name.
This is keyed by histogram name, which doesn't match our storage reality anymore.
Instead we use a name to refer to a set of histogram instances that record data from different processes, as well as separating session and subsession data.
In this re-write, we instead introduce the following lookup paths (managed in TelemetryHistogram.cpp):
- Main storage:
- (histogramId, processId, sessionOrSubsession) -> Histogram*
- (histogramId, processId) -> KeyedHistogram* (this handles subsessions internally)
- Lookup:
- (histogramName) -> histogramId
- (HistogramID) -> bool (is recording enabled for this histogram?)
This is wrapped with a few lookup functions.
This also allows us to keep HistogramIDs in the JS histogram instances now, instead of pointers to Histogram instances.
That means Histogram instance life-time management is now properly contained inside TelemetryHistogram.cpp.
MozReview-Commit-ID: 5yijGv7mc89
- A histogram name identifies a set of histogram instances, for which storage and lookup will be handled in TelemetryHistogram.cpp.
So we remove the names from histogram code.
- Various unused macros in the header are removed.
- Remaining traces of StatisticsRecorder are removed from the Histogram class code.
- Some unused methods are dropped that were about printing histograms to ASCII etc.
MozReview-Commit-ID: BF2rLSpKOJ8
The Chromium IPC histogram code used the StatisticsRecorder object for storage.
This is keyed by histogram name, which doesn't match our storage reality anymore.
Instead we use a name to refer to a set of histogram instances that record data from different processes, as well as separating session and subsession data.
Consequently we need to rewrite this storage, which means StatisticsRecorder is not used anymore.
MozReview-Commit-ID: 1LC7YubpKaD
It's silly to use prmem.h within Firefox code given that in our configuration
its functions are just wrappers for malloc() et al. (Indeed, in some places we
mix PR_Malloc() with free(), or malloc() with PR_Free().)
This patch removes all uses, except for the places where we need to use
PR_Free() to free something allocated by another NSPR function; in those cases
I've added a comment explaining which function did the allocation.
--HG--
extra : rebase_source : 0f781bca68b5bf3c4c191e09e277dfc8becffa09
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
This patch gives some structure and order to the profiler's API.
It also renames AutoProfilerRegister as AutoProfilerRegisterThread, to match
profiler_register_thread().
ipc/chromium/src/base/string_util.cc:35:3 [-Wunused-member-function] unused member function 'EmptyStrings'
ipc/glue/BackgroundParentImpl.cpp:405:3: warning: unused member function 'InitUDPSocketParentCallback' [-Wunused-member-function]
The class InitUDPSocketParentCallback was added in bug 1109338. It wasn't used then and it isn't used now.
https://hg.mozilla.org/mozilla-central/rev/eefcbe8e6a0c
MozReview-Commit-ID: BfDbQ5dfgo8
--HG--
extra : source : 7411baa0c64638b9da7de813c578fe9505625690
extra : intermediate-source : 975014d05919be2c9c6a0672c1ae94da86ccb29b
