use a javascript implementation of propertybags

VERIFIED FIXED in 0.8

Status

Calendar
Internal Components
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: dbo)

Tracking

unspecified
Bug Flags:
wanted-calendar0.8 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

2.62 KB, patch
dbo
: review-
Details | Diff | Splinter Review
7.68 KB, patch
Michiel van Leeuwen (email: mvl+moz@)
: review+
Details | Diff | Splinter Review
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.
(Reporter)

Comment 1

11 years ago
Created attachment 266198 [details] [diff] [review]
patch

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)
(Assignee)

Comment 2

11 years ago
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-
(Assignee)

Updated

10 years ago
Flags: wanted-calendar0.8+
Version: Trunk → unspecified

Comment 3

10 years ago
mvl, after reading bug 408264, are you going to update your patch here to incorporate daniel's review comments?
(Assignee)

Comment 4

10 years ago
Created attachment 294211 [details] [diff] [review]
patch
Attachment #294211 - Flags: review?(mvl)
(Assignee)

Comment 5

10 years ago
I've taken the liberty to attach a patch, because I had a property bag implementation in stock.
(Reporter)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Created attachment 294232 [details] [diff] [review]
patch - v2

new version, minds deleted properties while enumerating, unclutters nesting
Attachment #294211 - Attachment is obsolete: true
Attachment #294211 - Flags: review?(mvl)
(Reporter)

Comment 8

10 years ago
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]];
?
(Reporter)

Comment 9

10 years ago
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.
(Reporter)

Comment 10

10 years ago
(In reply to comment #9)
Oh, it's not. You should just ignore me.
(Reporter)

Comment 11

10 years ago
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
(Assignee)

Comment 12

10 years ago
(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.
(Reporter)

Comment 13

10 years ago
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-
(Assignee)

Comment 14

10 years ago
Created attachment 294646 [details] [diff] [review]
patch - v3

- incorporated comments
- moved to calUtils.js
Assignee: mvl → daniel.boelzle
Attachment #294232 - Attachment is obsolete: true
Attachment #294646 - Flags: review?(mvl)
(Reporter)

Comment 15

10 years ago
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+
(Assignee)

Comment 16

10 years ago
(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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
(Reporter)

Comment 17

10 years ago
(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.

(Assignee)

Comment 18

10 years ago
(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.
(Reporter)

Comment 19

10 years ago
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?
(Assignee)

Comment 20

10 years ago
comment added

Comment 21

10 years ago
repeating events have lost their title's, perhaps even more?
see bug 410086 
(Assignee)

Comment 22

10 years ago
No further problems reported => verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.