Closed Bug 599545 Opened 14 years ago Closed 14 years ago

Investigate remote-prefs serialization performance

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: dougt, Assigned: MikeK)

References

Details

Attachments

(1 file, 10 obsolete files)

24.00 KB, patch
dwitte
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #589905 +++

When fixing 589905, we enumerated the preferences hash table using existing libpref apis.  It might be faster if we directly enumerate.  At the very least, we can preallocate the buffer that is remoted.
Assignee: nobody → mkristoffersen
Attached patch Fits m-c from sep. 29 (obsolete) — Splinter Review
This patch gives about a factor 10 in speed increase on the serialization in debug builds and about a factor 5 in release builds on PC.

On a N900, the average time went down from 22.5 ms to 3.2 ms. (sampling 1000 serializations)

This patch does not apply on the current trunk!
(In reply to comment #1)
> Created attachment 479417 [details] [diff] [review]
> Fits m-c from sep. 29

Yeah right, try reading that as sep 27 :)
(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 479417 [details] [diff] [review] [details]
> > Fits m-c from sep. 29
> 
> Yeah right, try reading that as sep 27 :)

(and with the patches from Bug #589905 applied)
Depends on: 589905
Attachment #479417 - Flags: review?(dwitte)
Yargh. This is a lot of extra code.

The way I was envisioning it was this:

* Adjust the declaration of PrefHashEntry (if necessary) to have a sensible copy constructor.

* In SerializePreferences, create an nsTArray<PrefHashEntry>, and set its capacity (not length!) to PL_DHASH_TABLE_SIZE(table) (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.h#234). This will be an overestimate, but that's OK.

* Write a custom enumerator a la pref_savePref to enumerate the hash and directly append the PrefHashEntry struct to the array.

* Use the existing IPC ParamTraits class to serialize the nsTArray<PrefHashEntry>; this will require writing a trivial ParamTraits serializer for PrefHashEntry.

* On the receiving side, traverse the array and add the entries to the hash. Sadly, there's no API for preallocating the hash size; since the serialization occurs on startup, we could potentially delay hash initialization until after we receive the data. (PL_DHashTableInit allows specification of initial size.) We can do this if it's measurably better.

The serialization code for a single entry (the observer case) will become trivial, too -- just use the existing ParamTraits serializer for PrefHashEntry.

How does that sound?
As a side note: IPC 'Message' descriptors currently grow on an exponential basis -- doubling size on each realloc. If this is too malloc-happy (it should be OK I think; certainly better than the one malloc that occurs per pref string now!), we could look at presizing the Message. Not sure if that API exists yet.

cjones would know. ;)
Correct, it doesn't exist yet.  We can add it if malloc-happiness shows up in profiles.
Looking at prefapi, it's way more malloc-happy than the IPDL layer probably is, so I suspect we'd want to whack prefapi first. Need numbers to prioritize, though.
Attachment #479417 - Flags: review?(dwitte) → review-
(In reply to comment #4)
> How does that sound?

Fast ! 

:)
As part of this I have to export the "PrefHashEntry" structure found in the private header file prefapi.h (as it is needed in the serializer for the IPDL)

My question is now:

1) Should I make prefapi.h public ( 1a) by exporting it from the makefile but leaving it in its current folder or 1b) by moving it to the public folder and updating the makefiles)

2) Create a new headerfile for the PrefHashEntry and the stuff that one drags with it, and place this file in the public folder)

3) Use some clever trick to access the prefapi.h file from the serializer code (which trick then)

4) Do something completely different? (like currently I just copied the definition into the IPC serialiser function to be able to move on, which is bad for many reason btw.)
I'd actually go really simple here and declare a new struct -- IPC::PrefTuple -- that has nsCAutoString members for the strings. Then have nsPrefService/Branch copy data into it, do the IPC, and copy out. This avoids having to mess with internal details of prefapi -- the arena and PL_strdup allocators. Let's see how performant that is...
Note: this is posted to get some feedback - I am aware it has inconsistent/wrong use of (file-)names, and that it misses the part that actually feeds the data into the preference service on the content side.
Lets make it easier for everyone by deleting the commented out stuff ;) 

Same comments as above applies - naming is wrong in places, the new file should have a different name, it is missing the part that stores the transfered preferences into the preference service on the content side, it doesn't remove the existing code that it is meant to replace.
Attachment #481080 - Attachment is obsolete: true
Comment on attachment 481082 [details] [diff] [review]
Fits m-c from sep. 27 (work in progress)

How about going down this path?
Attachment #481082 - Flags: feedback?(dwitte)
(In reply to comment #13)
> Comment on attachment 481082 [details] [diff] [review]
> Fits m-c from sep. 27 (work in progress)
> 
> How about going down this path?

I have some initial measurements that shows that this new approach is slower than the way it was done in the original patch - I'm guessing due to the overhead of serializing an array compared to a string in IPC.
How much slower on a release build on N900?

Based on your results in comment 1, a ~20ms/1000 = 20us serialization time isn't that serious in an absolute sense. So I think the value here is making the serialization not rely on parsing in prefapi, rather than actually making things significantly faster.
(In reply to comment #15)
> How much slower on a release build on N900?

This time I have tried to measure the time spend in both the content and the chrome process and the technique has changed, so these new measurements can't be compared directly with the old ones.

The measurements are for an optimized build run on an N900 - there is a potential rounding error of max +/- 2ms pr run.

The original code took 42731 ms for 1000 runs ~ 42 ms/run 
The first patch took 16509 ms for 1000 runs ~ 16 ms/run
The new way took 40223 ms for 1000 runs ~ 40 ms/run

> 
> Based on your results in comment 1, a ~20ms/1000 = 20us serialization time

Nah.. I guess I wasn't clear enough - the ~20 ms, was the average for 1 sample, so the time for the total test was 1000x20ms ~ 20 s. 

> isn't that serious in an absolute sense. So I think the value here is making
> the serialization not rely on parsing in prefapi, rather than actually making
> things significantly faster.

Why is that a goal?

-----

What I would like to see next is a measurement of how much time is spend in serializing an nsTArray, compared to serializing the same amount of data encoded in say a string.  And I will split out the time it takes to rebuild the data on the content side.  

As there is no passing of the data in the new way of doing it, I can only see the penalty being in either the transfer of the nsTArray in it self, or the injection of the data into the preference service as taking the time.

I'll post a complete patch with the new way that includes the missing bits from attachment 481082 [details] [diff] [review].
Comment on attachment 481082 [details] [diff] [review]
Fits m-c from sep. 27 (work in progress)

Some brief comments, pending a final patch:

>diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

>+using nsPreferencesArray;

I'd just refer to this as an nsTArray<> directly, and call nsPreference PrefTuple instead. (The 'ns' prefix is deprecated.)

>diff --git a/modules/libpref/public/nsIPrefService.idl b/modules/libpref/public/nsIPrefService.idl

>+%{C++

This should all go in a header file. Can you use nsPrefService.h/nsPrefBranch.h?

Just use nsCStrings rather than char*; you could probably even use nsCAutoStrings to avoid allocations at the cost of space. But all this lives for a very short time, so that's probably OK.

>diff --git a/modules/libpref/public/nsPrefHashEntryArrayIPCSerialiser.h b/modules/libpref/public/nsPrefHashEntryArrayIPCSerialiser.h

PPrefParams.h?

>new file mode 100644
>--- /dev/null
>+++ b/modules/libpref/public/nsPrefHashEntryArrayIPCSerialiser.h

>+  // Function to de-serialize a geoposition

Really? ;)

>+  static bool Read(const Message* aMsg, void **aIter, paramType* aResult)
>+  {
>+    PRUint32 type;
>+    nsCString tempString;
>+
>+    if (!ReadParam(aMsg, aIter, &tempString)) return false;
>+
>+    aResult->key = new char [tempString.Length()+1];
>+    strcpy(const_cast<char*>(aResult->key), tempString.get());
>+
>+    if (!ReadParam(aMsg, aIter, &type)) return false;
>+
>+    switch (type) {
>+      case nsPreference::PREF_STRING:
>+        aResult->type = nsPreference::PREF_STRING;
>+        if (!ReadParam(aMsg, aIter, &tempString)) return false;
>+        aResult->value.stringVal = new char [tempString.Length()+1];

This will leak, since you're not freeing the data anywhere. You have a few choices:

1) Do it in nsPrefService::Init(), or better, factor the SendReadPrefsArray() into a helper that calls SendReadPrefsArray() and then frees everything.

2) Make nsPreference have owning string members (i.e. nsCString/nsCAutoString).

3) Make nsPreference have an 'owns' flag, and have its dtor free the char* strings if it's set. (And set it in ParamTraits::Read().)

Let's just do 2) with nsCAutoString members for now?

>diff --git a/modules/libpref/src/prefapi.cpp b/modules/libpref/src/prefapi.cpp

>+PLDHashOperator
>+pref_mirrorAllPref(PLDHashTable *table, PLDHashEntryHdr *heh, PRUint32 i, void *arg)
>+{

>+      argData->array->AppendElement(pref);

Do 'nsPreference* pref = argData->array->AppendElement()' at the top, and then you can write directly into 'pref' as you already do.

>diff --git a/modules/libpref/src/prefapi_private_data.h b/modules/libpref/src/prefapi_private_data.h

>+struct pref_mirrorAllPrefArgs {
>+  nsPreferencesArray *array;
>+};

No need for this struct since its only member is the array; just pass that directly into the callback.

General idea looks good.
Attachment #481082 - Flags: feedback?(dwitte) → feedback+
(In reply to comment #16)
> The original code took 42731 ms for 1000 runs ~ 42 ms/run 
> The first patch took 16509 ms for 1000 runs ~ 16 ms/run
> The new way took 40223 ms for 1000 runs ~ 40 ms/run

OK, we should figure out what the difference is from. I suspect it's because of the allocations occurring on the receiving side (the 'new char[]' bits in ParamTraits::Read()). Try with nsCAutoStrings and see?

> Nah.. I guess I wasn't clear enough - the ~20 ms, was the average for 1 sample,
> so the time for the total test was 1000x20ms ~ 20 s. 

OK. Definitely a blocker then.

> Why is that a goal?

Because parsing is more fragile.

> What I would like to see next is a measurement of how much time is spend in
> serializing an nsTArray

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#252

It's basically free -- the nsTArray itself is cheap. Dealing with each element might not be. As noted above, in deserializing your nsPreference you're allocating potentially two strings only to free them a short time after. The parsing approach didn't do that, because the string data went directly into the pref hash struct. (Growing an nsCString, by the way, is similar to growing an nsTArray -- exponential. So a very large string will only incur a handful of reallocs and memcpys.)
(In reply to comment #17)
> Comment on attachment 481082 [details] [diff] [review]
> >diff --git a/dom/ipc/PContent.ipdl b/dom/ipc/PContent.ipdl

Thank you for including this line before each of your comments, you only know how valuable that is, when you have had reviews where they were missing :)

> 
> >+using nsPreferencesArray;
> 
> I'd just refer to this as an nsTArray<> directly, and call nsPreference
> PrefTuple instead. (The 'ns' prefix is deprecated.)

Changed

> >+%{C++
> 
> This should all go in a header file. Can you use
> nsPrefService.h/nsPrefBranch.h?

No it can't for the same reason we have discussed previously, there are no public header files on the service (only IDL/IPDL) - but I have created a new header file for the data structure and I have included the #include inside %{C++ %} - don't know if there are better ways?

> 
> Just use nsCStrings rather than char*; you could probably even use
> nsCAutoStrings to avoid allocations at the cost of space. But all this lives
> for a very short time, so that's probably OK.

The reason I have been using char* is that this is what is used internally in the preference service, so I saw no reasons for using one of the "advanced" string types without gaining anything from it - but after I ensure there is no penalty from doing it I'll put a patch up where I don't use char*.

> 
> >diff --git a/modules/libpref/public/nsPrefHashEntryArrayIPCSerialiser.h b/modules/libpref/public/nsPrefHashEntryArrayIPCSerialiser.h
> 
> PPrefParams.h?

np - changed :)

> >+  // Function to de-serialize a geoposition
> 
> Really? ;)

woups... - changed to something more plausible.

> 
> This will leak, since you're not freeing the data anywhere. You have a few
> choices:

No it doesn't leak, it only looks like it because you couldn't see what my plan with it was - the strings are released after they have been injected into the service (the functional part that was missing in the patch)

> 
> 1) Do it in nsPrefService::Init(), or better, factor the SendReadPrefsArray()
> into a helper that calls SendReadPrefsArray() and then frees everything.
> 
> 2) Make nsPreference have owning string members (i.e. nsCString/nsCAutoString).
> 
> 3) Make nsPreference have an 'owns' flag, and have its dtor free the char*
> strings if it's set. (And set it in ParamTraits::Read().)
> 
> Let's just do 2) with nsCAutoString members for now?

I'll get back to this one in another update

> 
> >+      argData->array->AppendElement(pref);
> 
> Do 'nsPreference* pref = argData->array->AppendElement()' at the top, and then
> you can write directly into 'pref' as you already do.

Good idea - changed!

> 
> >diff --git a/modules/libpref/src/prefapi_private_data.h b/modules/libpref/src/prefapi_private_data.h
> 
> >+struct pref_mirrorAllPrefArgs {
> >+  nsPreferencesArray *array;
> >+};
> 
> No need for this struct since its only member is the array; just pass that
> directly into the callback.

Changed, even I don't think it hurts to have it packed into a struct - looked more consistent to me, but anyway it doesn't anymore...

----

I'm running some more speed tests - hope I can post a new patch with some numbers tomorrow.
(In reply to comment #18)
> > Nah.. I guess I wasn't clear enough - the ~20 ms, was the average for 1 sample,
> > so the time for the total test was 1000x20ms ~ 20 s. 
> 
> OK. Definitely a blocker then.

Just to be sure we really understand each other - the penalty during startup is 20 ms, and it took 20 s to run that bit of the startup sequence 1000 times to get an average number.

> 
> > Why is that a goal?
> 
> Because parsing is more fragile.

And by by parsing, I assume you mean convert the preference to a string, and then parse this string back to a set of preferences?
(In reply to comment #19)
> > This should all go in a header file. Can you use
> > nsPrefService.h/nsPrefBranch.h?
> 
> No it can't for the same reason we have discussed previously, there are no
> public header files on the service (only IDL/IPDL) - but I have created a new
> header file for the data structure and I have included the #include inside
> %{C++ %} - don't know if there are better ways?

So, the idl only needs to know that the inparam is a pointer to a forward-declared type -- I think you can just declare 'class foo;' in the C++ block (or maybe there's a magic idl incantation for it?) and then declare your native [ptr].

The PContent code (and thus ContentChild/ContentParent) should be able to use a forward-declared struct as well. Not sure what the ipdl incantation is for that.

So the only code that needs to know the concrete type is nsPrefHashEntryArrayIPCSerialiser.h. Can this #include nsPrefBranch.h, or does it get built outside of modules/libpref?

> The reason I have been using char* is that this is what is used internally in
> the preference service, so I saw no reasons for using one of the "advanced"
> string types without gaining anything from it - but after I ensure there is no
> penalty from doing it I'll put a patch up where I don't use char*.

The advantage would be if you use nsCAutoString there aren't any allocations, but get some numbers and we'll see. :)

(In reply to comment #20)
> And by by parsing, I assume you mean convert the preference to a string, and
> then parse this string back to a set of preferences?

Yep.
(In reply to comment #21)
> 
> So, the idl only needs to know that the inparam is a pointer to a
> forward-declared type -- I think you can just declare 'class foo;' in the C++
> block (or maybe there's a magic idl incantation for it?) and then declare your
> native [ptr].
> 
> The PContent code (and thus ContentChild/ContentParent) should be able to use a
> forward-declared struct as well. Not sure what the ipdl incantation is for
> that.

Yeah, you are giving me inspiration now - will have a second go - think I got some ideas :)

> 
> So the only code that needs to know the concrete type is
> nsPrefHashEntryArrayIPCSerialiser.h. Can this #include nsPrefBranch.h, or does
> it get built outside of modules/libpref?

I think it's build outside.

> 
> > The reason I have been using char* is that this is what is used internally in
> > the preference service, so I saw no reasons for using one of the "advanced"
> > string types without gaining anything from it - but after I ensure there is no
> > penalty from doing it I'll put a patch up where I don't use char*.
> 
> The advantage would be if you use nsCAutoString there aren't any allocations,
> but get some numbers and we'll see. :)

Yep, better get some number to base the choice on.

> 
> (In reply to comment #20)
> > And by by parsing, I assume you mean convert the preference to a string, and
> > then parse this string back to a set of preferences?
> 
> Yep.

Lets see how fast I can make it with your suggestions and then make the final call based on actual numbers.
I think I really need a second pair of eyes on this patch now - I'm kind of stuck on how to set-up the #includes / forward declarations - the problem lies in the PrefTuple.key (which I would like to be a nsCString) - the patch attached here gives the following error:

In file included from /home/mike/MozillaCode/101011/mozilla-central/modules/libpref/src/prefapi.cpp:41:
../../../dist/include/PrefTuple.h:47: error: ‘nsCString’ does not name a type
../../../dist/include/PrefTuple.h:56: error: ISO C++ forbids declaration of ‘nsCString’ with no type
../../../dist/include/PrefTuple.h:56: error: expected ‘;’ before ‘*’ token
/home/mike/MozillaCode/101011/mozilla-central/modules/libpref/src/prefapi.cpp: In function ‘PLDHashOperator pref_mirrorAllPref(PLDHashTable*, PLDHashEntryHdr*, PRUint32, void*)’:
/home/mike/MozillaCode/101011/mozilla-central/modules/libpref/src/prefapi.cpp:381: error: ‘struct PrefTuple’ has no member named ‘key’
/home/mike/MozillaCode/101011/mozilla-central/modules/libpref/src/prefapi.cpp:390: error: ‘union PrefTuple::<anonymous>’ has no member named ‘stringVal’

(fair enough, as it doesn't know about nsCString)

Now if I uncomment line 41 - so it includes nsStringAPI.h then I get a lot of errors - starting with:

In file included from ../../dist/include/PrefTuple.h:41,
                 from ../../dist/include/PPrefTuple.h:44,
                 from ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:22,
                 from ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PContentParent.h:9,
                 from /home/mike/MozillaCode/101011/mozilla-central/chrome/src/nsChromeRegistryChrome.cpp:40:
../../dist/include/nsStringAPI.h:47:2: error: #error nsStringAPI.h is only usable from non-MOZILLA_INTERNAL_API code!

-----

I'm sure this is very simple to solve - but I have probably looked so much at the code that I'm blind to the solution.

(forget about the "#include "base/basictypes.h"" I was desperate ;) )
Try including nsString.h instead.
About the measurement of timing

So today I also played a bit around with how to get consistent measurements, as I would like to get fairly correct timings on the different suggestions for improvements.

What I did previously was to take the average in a loop calling the code 1000 times - but when I ran the test again the result could change significantly (In a single incident I the difference was above 100% - but usually < ~20%) - so what I have done and what seems to give more consistent results is to still take the average of 1000 runs, but at the same time remembering the duration of the shortest run - because this seems to fluctuate far less than the average - I'm sure someone with more statistical interest than me could come up with a better algorithm for getting a useful measurement, but I think it is good enough for what we need to use it for.

----

I have been measuring on the time it takes from the function is initialized in the content process, to the result is back from the chrome process and has been injected into the preference service. - so as it involves signaling across the process boundary, the numbers are expected to fluctuate on individual measurements.
I agree, the shortest run is much more indicative than the longest - random fluctuations from the system that make you faster are usually much smaller than the opposite.

Still, the shortest run will tend to be shortest because of a 'perfect storm' of where nothing else is running, the cache hits are optimal, memory is mapped nicely, etc. etc. So looking at the average is still best. Just add more test runs, if 1,000 has a standard deviation of %20, then 10,000 will get you 6%, and so forth (1/sqrt(N)). Also a good idea to calculate the standard deviation in each benchmark run.
(In reply to comment #24)
> Try including nsString.h instead.

Was sure I had tried that, but apparently not, it compiles now - thank you!
(In reply to comment #26)
> I agree, the shortest run is much more indicative than the longest - random
> fluctuations from the system that make you faster are usually much smaller than
> the opposite.

Yeah, but the current goal is only to make a change, and see if it gets faster or not - so as long as we are sure to get a "perfect" run then it should be ok - the problem is that we can't be sure - except if the numbers look wrong, then try to re-run the test.

> 
> Still, the shortest run will tend to be shortest because of a 'perfect storm'
> of where nothing else is running, the cache hits are optimal, memory is mapped
> nicely, etc. etc. So looking at the average is still best. Just add more test
> runs, if 1,000 has a standard deviation of %20, then 10,000 will get you 6%,
> and so forth (1/sqrt(N)). Also a good idea to calculate the standard deviation
> in each benchmark run.

Yeah, have been thinking about increasing the number of tests, down side is that they will start to take longer than a coffee break ;) - but would be interesting to take a few runs and see if it improves - should improve like you say thou...

Do we have a library for calculating things like standard deviation, or do I need to find my old math book for the algorithm?

One interesting but not surprising observation, I tried to export the 1000 measurements to a spreadsheet and then plot them in a graph - it looked like there was a tendency for the big fluctuations to happen in the beginning of the test run, and they got more stable towards the end - guess that has to do with the initialization finishing  or the cache filling up with the correct stuff ;)
Measurements and rest of the comments will follow
Attachment #481082 - Attachment is obsolete: true
Attachment #482703 - Attachment is obsolete: true
(In reply to comment #28)
> Do we have a library for calculating things like standard deviation, or do I
> need to find my old math book for the algorithm?

stddev = (average of squared values)  -  (average of values)^2

That's really all you need.

> 
> One interesting but not surprising observation, I tried to export the 1000
> measurements to a spreadsheet and then plot them in a graph - it looked like
> there was a tendency for the big fluctuations to happen in the beginning of the
> test run, and they got more stable towards the end - guess that has to do with
> the initialization finishing  or the cache filling up with the correct stuff ;)

Yes, exactly. Sometimes people ignore the first X runs for that reason.
> stddev = (average of squared values)  -  (average of values)^2

Uh, no.  That's the variance.  The standard deviation is the square root of that, and only if "values" above are all possible values.  If you're sampling, as here, then you should be dividing by (N=1) instead of (N) when calculating the "average" above.

Note also that "doing more samples will reduce your standard deviation" is bunk.  Your standard deviation as computed from the sample can just be expected to converge on your actual standard deviation for the distribution.  What more samples give you is that (standard deviation)/\sqrt{number of samples} is one method of estimating the error in your mean (the difference between your computed mean and the actual mean of the distribution)...
(In reply to comment #31)
> > stddev = (average of squared values)  -  (average of values)^2
> 
> Uh, no.  That's the variance.  The standard deviation is the square root of
> that, and only if "values" above are all possible values.  If you're sampling,
> as here, then you should be dividing by (N=1) instead of (N) when calculating
> the "average" above.
> 
> Note also that "doing more samples will reduce your standard deviation" is
> bunk.  Your standard deviation as computed from the sample can just be expected
> to converge on your actual standard deviation for the distribution.  What more
> samples give you is that (standard deviation)/\sqrt{number of samples} is one
> method of estimating the error in your mean (the difference between your
> computed mean and the actual mean of the distribution)...

Statistics are when what you thought was obvious isn't - what I need to be able to do is to compare two versions of the code and tell which one is faster and by how much.  

When I take the average (sum of all samples/number of samples) the values usually fluctuate about 20% - just taking the fastest value seems much more stable.  But I'm open to trying any algorithm as long as it brings us closer to the goal of telling if version 1 or 2 of the code is faster. 

Actually I should probably put it into flight mode when measuring on the N900 to see if that made the numbers more stable - currently its both on 3G and WLan when I measure, that could have an influence.
> When I take the average (sum of all samples/number of samples) the values
> usually fluctuate about 20% 

Sure.  It's quite possible that the time taken is simply nondeterministic...

If you have reason to believe that your error distribution is all positive (that is, that noise always makes the measured number bigger), then measuring the minimum may be reasonable; the problem is telling, given two minima, whether one is bigger than the other by chance or because of a real effect.

But in any case, what you can do is take a bunch (N) of measurements without the change and compute the mean (M1) and standard deviation (S1).  Then do the same after the change (M2, S2).  Then your estimated means are M1 +- S1/\sqrt{N} and M2 +- S2/\sqrt{N}.  If those two ranges overlap, then you don't really know whether the change helps or hurts.
Mike, how's it going? Let me know if you need any more help with the patch or numbers.
(In reply to comment #34)
> Mike, how's it going? Let me know if you need any more help with the patch or
> numbers.

I was working on some other stuff, but I'm back on this now.

If we let the exact way I measure rest a little, I found a way that gives consistent results, which is good enough for this purpose (meaning these numbers can be compared with each other but not with previous measurements)

I still have some stuff to try, but the most interesting results I got so far:

(all measurements on an optimized build on the N900)

The original code: ~32  ms
My original patch: ~12  ms

With your suggestions:
- using nsCString ~31 ms
- using nsCAutoString ~17 ms

So some good progress.  - I just have a few more things to try, then I'll post a patch.
Attached patch Fits m-c from Oct. 11 (obsolete) — Splinter Review
Attachment #479417 - Attachment is obsolete: true
Attachment #482722 - Attachment is obsolete: true
I almost shaved 1 ms more of the time for my original patch - but I don't think the benefit validates it - so the patch I just submitted is probably close to the final one.  - I don't have much more to add to it, except merge it to trunk and run it through the try server.
mike, can you mark dwitte as a reviewer if it is ready to go?
Comment on attachment 483939 [details] [diff] [review]
Fits m-c from Oct. 11

Also fits trunk from this morning...
Attachment #483939 - Flags: review?(dwitte)
(In reply to comment #39)
> Comment on attachment 483939 [details] [diff] [review]
> Fits m-c from Oct. 11
> 
> Also fits trunk from this morning...

Try server results are still coming in, I'll post if I see anything unexpected in them
Comment on attachment 483939 [details] [diff] [review]
Fits m-c from Oct. 11

>diff --git a/modules/libpref/public/PPrefTuple.h b/modules/libpref/public/PPrefTuple.h

>+  // Function to de-serialize a PrefTuple
>+  static bool Read(const Message* aMsg, void **aIter, paramType* aResult)
>+  {
>+    PRUint32 type;
>+
>+    if (!ReadParam(aMsg, aIter, &(aResult->key))) return false;

Two lines please, and below.

>diff --git a/modules/libpref/public/PrefTuple.h b/modules/libpref/public/PrefTuple.h

>+struct PrefTuple
>+{
>+  nsCAutoString key;
>+
>+  enum {
>+    PREF_STRING = 32,
>+    PREF_INT    = 64,
>+    PREF_BOOL   = 128
>+  } type;

I wouldn't bother assigning explicit values, since you're using switch/case to translate them anyway.

>+  union {
>+    nsCAutoString   *stringVal;  // Needs to be a pointer, as it has a constructor
>+    PRInt32         intVal;
>+    bool            boolVal;
>+  } value;

The 'new nsCAutoString' bit is kinda awful. Let's just drop the union and have all three types be present in the struct. That'll avoid the allocation and keep things nice and simple.

>diff --git a/modules/libpref/public/nsIPrefService.idl b/modules/libpref/public/nsIPrefService.idl

>@@ -164,17 +171,17 @@ interface nsIPrefServiceInternal : nsISu

>-  ACString serializePreferences();
>+  [noscript] void mirrorPreferences(in nsPreferencesArrayPtr aArray);

This changes nsIPrefServiceInternal, so you need to rev the UUID.

This also means it needs to land for b7 -- since this interface was added for b7 -- which means in the next 24 hours. Let's hurry and get this patch out!

>   ACString serializePreference(in ACString aPrefName);

We should implement this using PrefTuple as well, rather than ReadPrefBuffer() as it is now.

>   void readPrefBuffer(in ACString aBuffer);

Which means we can ditch this, and all its related code.

>diff --git a/modules/libpref/src/nsPrefService.cpp b/modules/libpref/src/nsPrefService.cpp

>@@ -138,22 +139,37 @@ nsresult nsPrefService::Init()

>-    return ReadPrefBuffer(prefs);
>+    // Store the array
>+    nsTArray<PrefTuple>::size_type index = array.Length();
>+    while (index-- > 0) {

Factor the code inside this loop into a common function that is also used to implement serializePreference().

>+      PrefTuple pref = array[index];
>+      switch (pref.type) {
>+        case PrefTuple::PREF_STRING:
>+          PREF_SetCharPref(pref.key.get(),pref.value.stringVal->get(), PR_TRUE);

Missing space after first arg, and below.

>+NS_IMETHODIMP nsPrefService::MirrorPreferences(nsTArray<PrefTuple> *aArray)
> {

>+  PL_DHashTableEnumerate(&gHashTable, pref_mirrorAllPref, aArray);

'pref_MirrorPrefs' would be a better name, I think.

>diff --git a/modules/libpref/src/prefapi.cpp b/modules/libpref/src/prefapi.cpp

>@@ -362,16 +363,50 @@ pref_savePref(PLDHashTable *table, PLDHa

>+PLDHashOperator
>+pref_mirrorAllPref(PLDHashTable *table, PLDHashEntryHdr *heh, PRUint32 i, void *arg)

Stick to 80 chars please; and below.

>+        PrefValue value = PREF_HAS_USER_VALUE(entry)?entry->userPref:entry->defaultPref;

Need more spaces.

>+        // This might seem stupid, as it's a union, containing the same elements
>+        // but as the unions are different types, it makes sence in the case where
>+        // anyone changes one of the unions, and don't change the other one

I wouldn't really bother with this comment; it's fairly obvious from the code.

Looking good! Need to see another patch with comments addressed, so r- on this one. Hopefully we can get this out in the next 24 hours. Please also make sure it runs through tryserver OK.
Attachment #483939 - Flags: review?(dwitte) → review-
Thank you for the review.

I made this in to a 2 step thingy - first one is addressing the comments for the patch, the second one will address the suggestion to change "serializePreference".  This is to increase the chance that at least some of this will make it before the codefreeze (it means that I can start try-server etc. and have a chance to correct any issues it might find, and that at least some of this will make it before codefreeze)

(In reply to comment #41)
> Comment on attachment 483939 [details] [diff] [review]
> Fits m-c from Oct. 11
> 
> >diff --git a/modules/libpref/public/PPrefTuple.h b/modules/libpref/public/PPrefTuple.h
> 
> Two lines please, and below.

OK

> I wouldn't bother assigning explicit values, since you're using switch/case to
> translate them anyway.

OK 

> >+  union {
> >+    nsCAutoString   *stringVal;  // Needs to be a pointer, as it has a constructor
> >+    PRInt32         intVal;
> >+    bool            boolVal;
> >+  } value;
> 
> The 'new nsCAutoString' bit is kinda awful. Let's just drop the union and have
> all three types be present in the struct. That'll avoid the allocation and keep
> things nice and simple.

*GG* Ok, as you please :)

> >-  ACString serializePreferences();
> >+  [noscript] void mirrorPreferences(in nsPreferencesArrayPtr aArray);
> 
> This changes nsIPrefServiceInternal, so you need to rev the UUID.

Woups - forgot about that, new one generated

> 
> This also means it needs to land for b7 -- since this interface was added for
> b7 -- which means in the next 24 hours. Let's hurry and get this patch out!
> 
> >   ACString serializePreference(in ACString aPrefName);

Good idea, will come in the second half of the patch
 
> >diff --git a/modules/libpref/src/nsPrefService.cpp b/modules/libpref/src/nsPrefService.cpp
> 
> >@@ -138,22 +139,37 @@ nsresult nsPrefService::Init()
> 
> >-    return ReadPrefBuffer(prefs);
> >+    // Store the array
> >+    nsTArray<PrefTuple>::size_type index = array.Length();
> >+    while (index-- > 0) {
> 
> Factor the code inside this loop into a common function that is also used to
> implement serializePreference().

Second half of the patch

> 
> >+      PrefTuple pref = array[index];
> >+      switch (pref.type) {
> >+        case PrefTuple::PREF_STRING:
> >+          PREF_SetCharPref(pref.key.get(),pref.value.stringVal->get(), PR_TRUE);
> 
> Missing space after first arg, and below.

Fixed 

> 
> >+NS_IMETHODIMP nsPrefService::MirrorPreferences(nsTArray<PrefTuple> *aArray)
> > {
> 
> >+  PL_DHashTableEnumerate(&gHashTable, pref_mirrorAllPref, aArray);
> 
> 'pref_MirrorPrefs' would be a better name, I think.

Changed

> 
> >diff --git a/modules/libpref/src/prefapi.cpp b/modules/libpref/src/prefapi.cpp
> 
> >@@ -362,16 +363,50 @@ pref_savePref(PLDHashTable *table, PLDHa
> 
> >+PLDHashOperator
> >+pref_mirrorAllPref(PLDHashTable *table, PLDHashEntryHdr *heh, PRUint32 i, void *arg)
> 
> Stick to 80 chars please; and below.

Changed, nothing personal in that - seems like a waste of time for all of us that there are no consensus on the line length in the project, even this file is far from consistent.

> 
> >+        PrefValue value = PREF_HAS_USER_VALUE(entry)?entry->userPref:entry->defaultPref;
> 
> Need more spaces.

Not sure how to do, but changed to: 

PREF_HAS_USER_VALUE(entry) ? entry->userPref : entry->defaultPref;

> 
> >+        // This might seem stupid, as it's a union, containing the same elements
> >+        // but as the unions are different types, it makes sence in the case where
> >+        // anyone changes one of the unions, and don't change the other one
> 
> I wouldn't really bother with this comment; it's fairly obvious from the code.

Yeah, especially now when the union has been deleted ;)

> Looking good! Need to see another patch with comments addressed, so r- on this
> one. Hopefully we can get this out in the next 24 hours. Please also make sure
> it runs through tryserver OK.

First version looked ok on try - will push again after the update
Attached patch Patch version 2 (1/2) (obsolete) — Splinter Review
Will yell here if try server complains
Attachment #483939 - Attachment is obsolete: true
Attachment #484251 - Flags: review?(dwitte)
Attachment #484251 - Attachment is obsolete: true
Attachment #484256 - Flags: review?(dwitte)
Attachment #484251 - Flags: review?(dwitte)
Comment on attachment 484256 [details] [diff] [review]
Now also includes the changes from the previous patch (part 1/2)

>diff --git a/modules/libpref/public/PrefTuple.h b/modules/libpref/public/PrefTuple.h

>+struct PrefTuple
>+{
>+  nsCAutoString key;
>+
>+  // We don't use a union to avoid allocations when using the string component

I'd add a note here saying that only one of these values will be valid, depending on type.

>diff --git a/modules/libpref/src/prefapi.cpp b/modules/libpref/src/prefapi.cpp

>+PLDHashOperator
>+pref_MirrorPrefs(PLDHashTable *table,
>+                 PLDHashEntryHdr *heh,
>+                 PRUint32 i,
>+                 void *arg)
>+{

>+            case PREF_BOOL:
>+                newEntry->boolVal = value.boolVal;

This will warn on MSVC, since you're forcing a PRBool (integer) value into a bool (bistate) value. You need to do:

  newEntry->boolVal = !!value.boolVal;

r=dwitte.
Attachment #484256 - Flags: review?(dwitte) → review+
Attachment #484256 - Attachment is obsolete: true
Attached patch Part 2/2 of the patch (obsolete) — Splinter Review
Note this is a delta, that must be applied on top of (part 1/2)
Attachment #484366 - Flags: review?(dwitte)
(In reply to comment #47)
> Created attachment 484366 [details] [diff] [review]
> Part 2/2 of the patch
> 
> Note this is a delta, that must be applied on top of (part 1/2)

Just pushed to try as: "Try submission 26ecd3da7d51"
Comment on attachment 484366 [details] [diff] [review]
Part 2/2 of the patch

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp

>+ContentChild::RecvPreferenceUpdate(const PrefTuple& aPref)
> {
>     nsCOMPtr<nsIPrefServiceInternal> prefs = do_GetService("@mozilla.org/preferences-service;1");
>-    prefs->ReadPrefBuffer(aPref);
>+    if (!prefs)
>+        return false;
>+
>+    prefs->SetPreference(const_cast<PrefTuple*>(&aPref));

You can declare, in the idl:

  [ptr] native nsPreferencePtrConst(const PrefTuple);

and then use it in the declaration of setPreference.

>diff --git a/modules/libpref/src/nsPrefService.cpp b/modules/libpref/src/nsPrefService.cpp

>@@ -145,28 +145,17 @@ nsresult nsPrefService::Init()

>+      PREF_SetPrefTuple(array[index], PR_TRUE);

Can we make this an internal method, i.e. pref_SetPrefTuple, and declare it by pref_MirrorPrefs?

Can also make the first arg 'const'.

>+NS_IMETHODIMP nsPrefService::MirrorPreference(const nsACString& aPrefName,
>+                                              PrefTuple *aPref)
>+{

>+  pref_ConvertHash2Tuple(pref, aPref);

Let's call this pref_GetTupleFromEntry.

>diff --git a/modules/libpref/src/prefapi.cpp b/modules/libpref/src/prefapi.cpp

>+nsresult
>+PREF_SetPrefTuple(PrefTuple &aPref, PRBool set_default)
>+{
>+    switch (aPref.type) {
>+        case PrefTuple::PREF_STRING:
>+            return PREF_SetCharPref(aPref.key.get(), aPref.stringVal.get(), set_default);
>+
>+        case PrefTuple::PREF_INT:
>+            return PREF_SetIntPref(aPref.key.get(), aPref.intVal, set_default);
>+
>+        case PrefTuple::PREF_BOOL:
>+            return PREF_SetBoolPref(aPref.key.get(), aPref.boolVal, set_default);
>+    }
>+    return NS_ERROR_INVALID_ARG;

Assert here.

>+void
>+pref_ConvertHash2Tuple(PrefHashEntry *aHashEntry, PrefTuple *aTuple)
>+{
>+    aTuple->key = aHashEntry->key;
>+
>+    PrefValue *value = PREF_HAS_USER_VALUE(aHashEntry) ?
>+        &(aHashEntry->userPref) : &(aHashEntry->defaultPref);
>+
>+    switch (aHashEntry->flags & PREF_VALUETYPE_MASK) {
>+        case PREF_STRING:
>+            aTuple->stringVal = value->stringVal;
>+            aTuple->type = PrefTuple::PREF_STRING;
>+            return;
>+
>+        case PREF_INT:
>+            aTuple->intVal = value->intVal;
>+            aTuple->type = PrefTuple::PREF_INT;
>+            return;
>+
>+        case PREF_BOOL:
>+            aTuple->boolVal = !!value->boolVal;
>+            aTuple->type = PrefTuple::PREF_BOOL;
>+            return;
>+    }
>+}

Does this warn about 'control reaches end of non-void function'?

Looks great. r=dwitte
Attachment #484366 - Flags: review?(dwitte) → review+
(In reply to comment #49)
> Comment on attachment 484366 [details] [diff] [review]
> Part 2/2 of the patch
> 
> >diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
> 
> >+ContentChild::RecvPreferenceUpdate(const PrefTuple& aPref)
> > {
> >     nsCOMPtr<nsIPrefServiceInternal> prefs = do_GetService("@mozilla.org/preferences-service;1");
> >-    prefs->ReadPrefBuffer(aPref);
> >+    if (!prefs)
> >+        return false;
> >+
> >+    prefs->SetPreference(const_cast<PrefTuple*>(&aPref));
> 
> You can declare, in the idl:
> 
>   [ptr] native nsPreferencePtrConst(const PrefTuple);

That solved it, thank you!

> 
> and then use it in the declaration of setPreference.
> 
> >diff --git a/modules/libpref/src/nsPrefService.cpp b/modules/libpref/src/nsPrefService.cpp
> 
> >@@ -145,28 +145,17 @@ nsresult nsPrefService::Init()
> 
> >+      PREF_SetPrefTuple(array[index], PR_TRUE);
> 
> Can we make this an internal method, i.e. pref_SetPrefTuple, and declare it by
> pref_MirrorPrefs?
> 
> Can also make the first arg 'const'.

yes and yes :)

> 
> >+NS_IMETHODIMP nsPrefService::MirrorPreference(const nsACString& aPrefName,
> >+                                              PrefTuple *aPref)
> >+{
> 
> >+  pref_ConvertHash2Tuple(pref, aPref);
> 
> Let's call this pref_GetTupleFromEntry.

Done

> 
> >diff --git a/modules/libpref/src/prefapi.cpp b/modules/libpref/src/prefapi.cpp
> 
> >+nsresult
> >+PREF_SetPrefTuple(PrefTuple &aPref, PRBool set_default)
> >+{
> >+    switch (aPref.type) {
> >+        case PrefTuple::PREF_STRING:
> >+            return PREF_SetCharPref(aPref.key.get(), aPref.stringVal.get(), set_default);
> >+
> >+        case PrefTuple::PREF_INT:
> >+            return PREF_SetIntPref(aPref.key.get(), aPref.intVal, set_default);
> >+
> >+        case PrefTuple::PREF_BOOL:
> >+            return PREF_SetBoolPref(aPref.key.get(), aPref.boolVal, set_default);
> >+    }
> >+    return NS_ERROR_INVALID_ARG;
> 
> Assert here.

How about NS_NOTREACHED("Unknown type") ?

> 
> >+void
> >+pref_ConvertHash2Tuple(PrefHashEntry *aHashEntry, PrefTuple *aTuple)
> >+{
> >+    aTuple->key = aHashEntry->key;
> >+
> >+    PrefValue *value = PREF_HAS_USER_VALUE(aHashEntry) ?
> >+        &(aHashEntry->userPref) : &(aHashEntry->defaultPref);
> >+
> >+    switch (aHashEntry->flags & PREF_VALUETYPE_MASK) {
> >+        case PREF_STRING:
> >+            aTuple->stringVal = value->stringVal;
> >+            aTuple->type = PrefTuple::PREF_STRING;
> >+            return;
> >+
> >+        case PREF_INT:
> >+            aTuple->intVal = value->intVal;
> >+            aTuple->type = PrefTuple::PREF_INT;
> >+            return;
> >+
> >+        case PREF_BOOL:
> >+            aTuple->boolVal = !!value->boolVal;
> >+            aTuple->type = PrefTuple::PREF_BOOL;
> >+            return;
> >+    }
> >+}
> 
> Does this warn about 'control reaches end of non-void function'?

Probably not, since it _is_ a void function ;)

> 
> Looks great. r=dwitte

Thanks
(In reply to comment #50)
> How about NS_NOTREACHED("Unknown type") ?

Sounds good.

> > Does this warn about 'control reaches end of non-void function'?
> 
> Probably not, since it _is_ a void function ;)

Gahh.
Attachment #484360 - Attachment is obsolete: true
Attachment #484366 - Attachment is obsolete: true
I don't think the warnings I got from the try-server has anything to do with this patch
Keywords: checkin-needed
Attachment #484441 - Flags: review+
Interface change, needs to go into b7 relbranch as well.
blocking2.0: --- → beta7+
http://hg.mozilla.org/mozilla-central/rev/da156c724d1a

Will land on relbranch tomorrow.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Adding milestone per beltzner.
Status: RESOLVED → REOPENED
blocking2.0: beta7+ → ---
Resolution: FIXED → ---
Target Milestone: --- → mozilla2.0b7
Uh, thanks, bugzilla.

Landed on GECKO20b7pre_20101006_RELBRANCH. http://hg.mozilla.org/mozilla-central/rev/d48d4bc3142c
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Depends on: 606702
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: