367229 - makeMemberAttr doesn't mind passed default value

Status: NEW

(Calendar :: Internal Components, defect)

Description

[Daniel Boelzle [:dbo]]

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: setting default properties upon construction

One possible implementation. Performance concerns? Most often items default constructed, then almost all attributes/properties are set.

Comment 2

[Joey Minta]

Comment on attachment 251757 [details] [diff] [review]
setting default properties upon construction

I ran the perf test I've talked about elsewhere, but the property-bag bug we identified is making it so there's too much noise to tell if this is a significant hit.

Without patch:
Import time:22036
FINAL RESULTS:
Day:2265ms (1405,1172,1190,1208,1238,1231,1225,4241,4655,5081)
Week:2441ms (2274,2712,2571,2838,3119,3423,3838,1218,1189,1224)
Month:1342ms (1306,1287,1305,1316,1336,1360,1358,1347,1429,1367)

With patch:
Import time:22095
FINAL RESULTS:
Day:3155ms (1504,1199,1210,2879,3198,3981,3852,4183,4569,4967)
Week:1594ms (2322,2373,2552,1176,1201,1228,1225,1335,1232,1296)
Month:1411ms (1689,1306,1335,1338,1346,1387,1419,1410,1418,1462)

Given that the month numbers have the least deviation, I'm inclined to give them the most weight.  In which case, we have a hit, but not a large one, which I think is pretty much what everyone expects.  I'd be interested in running this code through a test with one of the various patches that dampens this noise (mvl's js-property-bag patch ideally) and see what results we get there.

The biggest problem that I have with this patch is the assumptions it makes in various spots that certain chosen values don't evaluate to false.  For instance:
+    if (dflt && asProperty) {
+        if (!ctor.prototype.mPropertyDefaults)
+            ctor.prototype.mPropertyDefaults = {};
+        ctor.prototype.mPropertyDefaults[varname.toUpperCase()] = dflt;
+    }
If dflt is |false| or |0| this block won't be entered, even though we want it to be.

+    if (!ctor.prototype.setupPropertyDefaults) {
+        ctor.prototype.setupPropertyDefaults = function setupPropertyDefaults() {
+            if (this.mPropertyDefaults) {
Why isn't this included in the normal calItemBase prototype?  Also, playing around with |this| in this way seems really hack-ish.

+        else {
+            var ret = this[varname];
+            if (!ret)
+                ret = dflt;
+            return ret;
Here's another case where we run into the eval-to-false assumption.  Imagine I have dflt being |true|, and set the property explicitly to |false|.  It will come back as |true|!

+// function makeMemberAttr(ctor, varname, dflt, attr, asProperty)
+// {
+//     // XXX handle defaults!
You should remove code rather than comment it out.

Comment 3

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> The biggest problem that I have with this patch is the assumptions it makes in
> various spots that certain chosen values don't evaluate to false.  For
> instance:
> +    if (dflt && asProperty) {
> +        if (!ctor.prototype.mPropertyDefaults)
> +            ctor.prototype.mPropertyDefaults = {};
> +        ctor.prototype.mPropertyDefaults[varname.toUpperCase()] = dflt;
> +    }
> If dflt is |false| or |0| this block won't be entered, even though we want it
> to be.
Yes, right, I see this bug, too. As undefined properties are specified as makeMemberAttr(dflt=null) and getProperty returns null for undefined properties, IMO a strong check against (!== null) is correct here.

> +    if (!ctor.prototype.setupPropertyDefaults) {
> +        ctor.prototype.setupPropertyDefaults = function
> setupPropertyDefaults() {
> +            if (this.mPropertyDefaults) {
> Why isn't this included in the normal calItemBase prototype?  Also, playing
> around with |this| in this way seems really hack-ish.
makeMemberAttr is also called for calAttendee (which needs it, too).

> +        else {
> +            var ret = this[varname];
> +            if (!ret)
> +                ret = dflt;
> +            return ret;
> Here's another case where we run into the eval-to-false assumption.  Imagine I
> have dflt being |true|, and set the property explicitly to |false|.  It will
> come back as |true|!
IMO we should test (=== undefined) here.

> +// function makeMemberAttr(ctor, varname, dflt, attr, asProperty)
> +// {
> +//     // XXX handle defaults!
> You should remove code rather than comment it out.
just left it in for reference as this was only a proposal for discussion.

thanks for reviewing!

Comment 4

[Daniel Boelzle [:dbo]]

Attachment: setting default properties upon construction, v2

another iteration

Comment 5

[Daniel Boelzle [:dbo]]

BTW: can somebody enlighten me why makeMemberAttr can be called in calItemBase.js before it is defined?

Comment 6

[Joey Minta]

Comment on attachment 252479 [details] [diff] [review]
setting default properties upon construction, v2

I'd probably prefer if mPropertyDefaults were declared as a property of the ctors that calls this.  That would save a bunch of if() checks.

+                for (var prop in this.mPropertyDefaults) {
+                    var val = this.mPropertyDefaults[prop];
+                    if (val instanceof Components.interfaces.calIDateTime)
+                        val = val.clone();
+                    this.setProperty(prop, val);
+                }
As I read this, we'll have a problem if someone passes

+                for (var prop in this.mPropertyDefaults) {
+                    var val = this.mPropertyDefaults[prop];
+                    if (val instanceof Components.interfaces.calIDateTime)
+                        val = val.clone();
+                    this.setProperty(prop, val);
+                }
+            }
This is the block that concerns me.  All of our events are doing to have a bunch more data in them by default.  Before patch:
js> var ev = Cc["@mozilla.org/calendar/event;1"].createInstance(Ci.calIEvent);
js> ev.icalString;
BEGIN:VCALENDAR
PRODID:-//Mozilla Calendar//NONSGML Sunbird//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20070210T133713Z
LAST-MODIFIED:20070210T133718Z
DTSTAMP:20070210T133718Z
END:VEVENT
END:VCALENDAR

After patch:
js> var ev = Cc["@mozilla.org/calendar/event;1"].createInstance(Ci.calIEvent);
js> ev.icalString;
BEGIN:VCALENDAR
PRODID:-//Mozilla Calendar//NONSGML Sunbird//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20070210T133551Z
LAST-MODIFIED:20070210T133601Z
DTSTAMP:20070210T133601Z
PRIORITY:0
CLASS:PUBLIC
PERCENT-COMPLETE:0
END:VEVENT
END:VCALENDAR

What about, instead, if this.getProperty() returns null/undefined, then we go access mPropertyDefaults?  Then the defaults wouldn't be in the property bag, and wouldn't get serialized.

Comment 7

[Daniel Boelzle [:dbo]]

Good observation. However default values are correct, it makes little sense to serialize them, bloating ics.

- from an API standpoint, users want default values to be returned (transparently); no need to test for null or whatever
- from an ICS standpoint, default values (as defined by RFC2445) need not be written. Any calendaring app has to fall back to them.

> What about, instead, if this.getProperty() returns null/undefined, then we go
> access mPropertyDefaults?  Then the defaults wouldn't be in the property bag,
> and wouldn't get serialized.

You mean we change getProperty the way that it falls back to mPropertyDefaults? I like that, also from a performance point of view: we need not fill up properties on item construction.
[note: I initially had thoughts in that same direction, too, but found them too invasive to the existing code. Currently, makeMemberAttr is coupled to get/setProperty (which are part of calIItembase, calIAttendee, i.e. ok); then, makeMemberAttr will be coupled to the presence of mPropertyDefaults to fill up the defaults.]

Moreover, wouldn't it be sensible to omit writing default values at all in fillIcalComponentFromBase()?

Comment 8

[Martin Schröder [:mschroeder]]

Comment from bug 483643 from dbo:
I am not sure whether we should use the default value, but leave it to client
code to fall back as in the current code. See also a discussion in bug 367229
about this (maybe we could close it if we come to a consensus). Thus I'd vote
to just return null and remove the |dflt| parameter.