Closed
Bug 382121
Opened 17 years ago
Closed 17 years ago
use a javascript implementation of propertybags
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: mvl, Assigned: dbo)
Details
Attachments
(2 files, 2 obsolete files)
2.62 KB,
patch
|
dbo
:
review-
|
Details | Diff | Splinter Review |
7.68 KB,
patch
|
mvl
:
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•17 years ago
|
||
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 | ||
Comment 2•17 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•17 years ago
|
Flags: wanted-calendar0.8+
Version: Trunk → unspecified
Comment 3•17 years ago
|
||
mvl, after reading bug 408264, are you going to update your patch here to incorporate daniel's review comments?
Assignee | ||
Comment 4•17 years ago
|
||
Attachment #294211 -
Flags: review?(mvl)
Assignee | ||
Comment 5•17 years ago
|
||
I've taken the liberty to attach a patch, because I had a property bag implementation in stock.
Reporter | ||
Comment 6•17 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•17 years ago
|
||
new version, minds deleted properties while enumerating, unclutters nesting
Attachment #294211 -
Attachment is obsolete: true
Attachment #294211 -
Flags: review?(mvl)
Reporter | ||
Comment 8•17 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•17 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•17 years ago
|
||
(In reply to comment #9) Oh, it's not. You should just ignore me.
Reporter | ||
Comment 11•17 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•17 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•17 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•17 years ago
|
||
- incorporated comments - moved to calUtils.js
Assignee: mvl → daniel.boelzle
Attachment #294232 -
Attachment is obsolete: true
Attachment #294646 -
Flags: review?(mvl)
Reporter | ||
Comment 15•17 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•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Reporter | ||
Comment 17•17 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•17 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•17 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•17 years ago
|
||
comment added
Comment 21•17 years ago
|
||
repeating events have lost their title's, perhaps even more? see bug 410086
Assignee | ||
Comment 22•16 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.
Description
•