Closed Bug 382121 Opened 17 years ago Closed 17 years ago

use a javascript implementation of propertybags

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mvl, Assigned: dbo)

Details

Attachments

(2 files, 2 obsolete files)

There seems to be a performance problem with the propertybag that's used in calItemBase (bug 362762)
To work around that, I suggest to use a javascript propertybag instead.
Attached patch patchSplinter Review
This patch makes it so.
A simple performance test, using a testscript once made by jminta, shows:

without the patch:
Day:659ms (617,578,592,627,640,653,671,709,724,773)
Week:674ms (576,599,622,651,659,680,700,731,751,769)
Month:760ms (631,657,862,709,741,750,773,803,827,840)

with the patch:
Day:503ms (551,487,488,491,491,499,499,525,497,498)
Week:560ms (498,501,510,514,506,1018,509,515,510,514)
Month:637ms (544,548,550,547,550,562,555,560,554,1392)

The important thing is not so much the absolute values, but the changes of the values over time. Wken looking at the pre-patch values, the timing is not very consistent, and there seem to be an upward trend. With the patch, the numbers are stable (with two exceptions, those are because there were other processes running on my box)

(test done using a macbook)
Assignee: nobody → mvl
Status: NEW → ASSIGNED
Attachment #266198 - Flags: review?(daniel.boelzle)
Comment on attachment 266198 [details] [diff] [review]
patch

Index: base/src/calUtils.js

Shouldn't we start to split off utils into a different files, not bloating calUtils.js too much?

>+EXPORTED_SYMBOLS = ["calPropertyBag"];
why?

>+// Enumerate properties
>+function calPropertyEnumerator(aObj) {
>+    this.mProperties = [];
>+    var i=0;
>+    for (key in aObj) {
>+        this.mProperties[i] = new calProperty(key, aObj[key]);
>+        i++;
>+    }
use this.mProperties.push(...), no need for i.

>+// this is an implementation of the nsIWriteablePropertyBag interface
>+calPropertyEnumerator.prototype = {
>+    hasMoreElements: function cpe_hasMoreElements() {
>+        return (this.mCounter < this.mProperties.length - 1);
IMO wrong: hasMoreElements() would return false for a bag of size 1.

>+function calPropertyBag() {
>+    this.mData = [];
better this.mData = {};
Attachment #266198 - Flags: review?(daniel.boelzle) → review-
Flags: wanted-calendar0.8+
Version: Trunk → unspecified
mvl, after reading bug 408264, are you going to update your patch here to incorporate daniel's review comments?
Attached patch patch (obsolete) — Splinter Review
Attachment #294211 - Flags: review?(mvl)
I've taken the liberty to attach a patch, because I had a property bag implementation in stock.
Comment on attachment 294211 [details] [diff] [review]
patch

>+                return { // nsIProperty:
>+                    QueryInterface: function cpb_enum_prop_QueryInterface(aIID) {
>+                        ensureIID([Components.interfaces.nsIProperty, Components.interfaces.nsISupports], aIID);
>+                        return this;
>+                    },
>+                    name: name,
>+                    value: bag.mData[name]
>+                };

This makes the property depend on the property bag to get the value. I can see that going wrong for cases where you first get an enumerator, then delete properties, and then want to access the enumerator. Not sure if that's a real use case, though.

For readability, I'm not too happy with the very 'nested' code. The thing you return is very buried and hard to find. I personally like it better to have it defined as object.
Attached patch patch - v2 (obsolete) — Splinter Review
new version, minds deleted properties while enumerating, unclutters nesting
Attachment #294211 - Attachment is obsolete: true
Attachment #294211 - Flags: review?(mvl)
Comment on attachment 294232 [details] [diff] [review]
patch - v2

>Index: calendar/base/src/calItemBase.js
>+calPropertyBagEnumerator.prototype = {
>+    hasMoreElements: function cpb_enum_hasMoreElements() {
>+        while (this.mIndex < this.mKeys.length) {
>+            this.mCurrentValue = this.mBag.mData[this.mKeys[this.mIndex]];
>+            if (this.mCurrentValue !== undefined) {

undefined can be a valid value (i think). What about:
+            if (this.mKeys[this.mIndex] in this.mBag.mData) {
+              this.mCurrentValue = this.mBag.mData[this.mKeys[this.mIndex]];
?
Comment on attachment 294232 [details] [diff] [review]
patch - v2

>Index: calendar/base/src/calItemBase.js
>+calPropertyBagEnumerator.prototype = {
>+    getNext: function cpb_enum_getNext() {
>+        if (!this.hasMoreElements()) {
>+        var name = this.mKeys[this.mIndex++];
>+    },
>+    hasMoreElements: function cpb_enum_hasMoreElements() {
>+            ++this.mIndex;

From looking at the code, it seems that mIndex is incremented twice: once in hasMoreElements, and then again in getNext. And for a normal enumeration loop, you need to call both functions, so mIndex is incremented three times. A bit much, if you ask me.
(In reply to comment #9)
Oh, it's not. You should just ignore me.
For a little speed improvement, you can remove the QueryInterface call from makeItemBaseImmutable, in the loop over the properties:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/calendar/base/src/calItemBase.js&rev=1.77&mark=170#153

In cloneItemBaseInto you can do the same, and should switch to the JS property bag:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/calendar/base/src/calItemBase.js&rev=1.77&mark=242-243,247#242
(In reply to comment #8)
> undefined can be a valid value (i think). What about:
undefined is no value, there is actually no such property set on mData.
Comment on attachment 294232 [details] [diff] [review]
patch - v2

>+    getProperty: function cpb_getProperty(aName) {
>+        return this.mData[aName] || null;
>+    },

Running the unit tests found a problem with this: PRIORITY:0 would get lost on roundtrip. A solution could be:
    getProperty: function cpb_getProperty(aName) {
        if (aName in this.mData)
            return this.mData[aName];
        return null;
    },

r- based on the failing roundtrip. If you create a new patch, please fix what I mentioned in comment 11. The speed improvement is noticeable in my tests :)
Attachment #294232 - Flags: review-
Attached patch patch - v3Splinter Review
- incorporated comments
- moved to calUtils.js
Assignee: mvl → daniel.boelzle
Attachment #294232 - Attachment is obsolete: true
Attachment #294646 - Flags: review?(mvl)
Comment on attachment 294646 [details] [diff] [review]
patch - v3

Nice catch on the attendees change.

>+calPropertyBagEnumerator.prototype = {
>+    getNext: function cpb_enum_getNext() {
>+        var name = this.mKeys[this.mIndex++];
>+    hasMoreElements: function cpb_enum_hasMoreElements() {
>+            ++this.mIndex;

The increment of mIndex is split over two functions. I think that it would be nicer to have them all in the same function. hasMoreElements might be the easiest to move it to, although getNext has a better name.

r=mvl. I'll leave it to you to say if you agree and want to fix the above
Attachment #294646 - Flags: review?(mvl) → review+
(In reply to comment #15)
> The increment of mIndex is split over two functions. I think that it would be
> nicer to have them all in the same function. hasMoreElements might be the
> easiest to move it to, although getNext has a better name.
There are two different purposes:
- index incrementation in hasMoreElements() only skips deleted items
- index incrementation in getNext() actually moves the enumerator forward
Thus I don't see a valid possibility to unify that code.

Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
(In reply to comment #16)
> - index incrementation in hasMoreElements() only skips deleted items
But still increments
> - index incrementation in getNext() actually moves the enumerator forward
But can point to a deleted item afterwards.

I think you can move the check for deleted items into getNext, or move the mIndex++ call from getNext into hasMoreElements.

(In reply to comment #17)
> I think you can move the check for deleted items into getNext, or move the
> mIndex++ call from getNext into hasMoreElements.
Doing this
1. If I move the check into getNext, hasMoreElements might return true although there are only (already) deleted items left which is wrong.
2. If I move mIndex++ into hasMoreElements (with the intention to move the enumerator forward, not skipping deleted items), then calling hasMoreElements multiple times will behave incorrect, because elements are skipped.

getNext already calls hasMoreElements first by intention, to skip items that might have been deleted.
I think I missed the call to hasMoreElements from getNext. Could you add a comment explaining that the call is not just a sanity check, but also does a bit more?
comment added
repeating events have lost their title's, perhaps even more?
see bug 410086 
No further problems reported => verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.