Remove overhead from OpenLayers.Class

[ahocevar]

See  http://lists.osgeo.org/pipermail/openlayers-dev/2010-October/006666.html

[erilem]

I've just quickly gone through your patch and

C = OpenLayers.inherit.apply(this, newArgs);

looks suspicious to me. What does this refer to here?

Also, for you to know, I actually did not mean have my OpenLayers.inherit function return C.

[ahocevar]

@erilem: The OpenLayers.inherit function in my patch is taken as-is from your original post on the thread liked from the description. And no need to worry about "this" there, you could also use "null". The OpenLayers.inherit function does not need a scope. But since we are dealing with an unknown number of arguments, we cannot just do C = OpenLayers.inherit(...?..?...)

[ahocevar]

Also, for those who missed in the dev list thread: with this patch applied (yes, it does work), OpenLayers.Class performs equally well as the Pure JavaScript approach, because it is the Pure JavaScript approach. Thanks erilem for this dev list thread, the code samles there were excellent and are now all in OpenLayers.Class in this patch :-).

[pwr]

would suggest adding some comments

[crschmidt]

I got a couple of meaningful-looking errors from the Class tests:

test_Class_inheritance planned 7 assertions but got 6; fail 0 ok 6 exception: : object: Result of expression 'P.prototype' [undefined] is not an object.

test_Class_backwards planned 4 assertions but got 5; fail 1 ok 4 fail the base class is never instantiated

I'm not sure that this is super important, but we should explain these failures and change them if we don't expect them to work anymore.

[ahocevar]

Right, comments and tests are still missing.

[ahocevar]

attachment:Class.2.diff adds comments and fixes the legacy OpenLayers.Class.inherit method to ensure backwards compatibility.

All tests pass in Safari 5, Firefox 3.6 and IE 8.

Thanks for any review.

[erilem]

I'd like to look more closely at the patch. I can do that on monday.

[erilem]

I have questions prior to reviewing. Do we keep OpenLayers.Class for compatibility reasons (application code defining classes with it), and want to eventually not rely on OpenLayers.Class for defining classes in the lib? Also, would we keep OpenLayers.Class in 3.x? I personally see no point to keep it in 3.x.

[ahocevar]

@erilem: I have no strong opinion pro or contra OpenLayers.Class, but with my patch applied, all overhead is removed from it.

[erilem]

Andreas, with Class.3.diff OpenLayers.inherit doesn't return C, as I don't think this is needed. Tests continue to pass. You can commit if you agree with this change. But, naturally, feel also free to wait for Tim's review.

Now why I'd be in favor to no longer use OpenLayers.Class in the lib:

(a) I still observe that "Pure JavaScript" performs better than "OpenLayers.Class". perf-type-instantiation.html gives me the following results in FF3:

--- Type Instantiation ---
OpenLayers.Class 177.8
Pure JavaScript 159.1
Object Litteral 140

(I'm still not sure why though.)

(b) More importantly, defining types is a lot faster when not using OpenLayers.Class. So not using OpenLayers.Class will reduce the initial parsing of OpenLayers.js. perf-type-definition.html gives me the following results in FF3:

--- Type Definition ---
OpenLayers.Class 65.8
Pure JavaScript 4.6

Thanks a lot for the great collaboration on this!

[pwr]

according to  http://trac.osgeo.org/openlayers/wiki/CodingStandards :

"All identifiers should be long enough to be understandable and unambiguous. For example, zoomHasChanged is preferable to zlch."

P, C and F don't seem to conform to this :-)

[ahocevar]

r10858

[ahocevar]

@pwr: Of course you are right. If the purpose of variables is as obvious as here (similar to i in for loops), I think we can make an exception for the sake of saving bytes for the poor folks out there that don't use a js compressor that shortens variable names.

@erilem: I ran the benchmark you created for (a) on my machine, and after several reloads I have to say that sometimes pure js, and sometimes OpenLayers.Class wins. The benchmark you created for (b) is unfair, because we do not have 10000 base classes in the library, only (educated guess) about 20. And to compare OpenLayers.Class and pure js for classes that inherit from other classes, you would also have to call OpenLayers.inherit in case 2 of (b). But again, I'm not against removing OpenLayers.Class in 3.0. Thanks for the cooperation on this! We just made class instantiation 10 times faster.

[erilem]

@erilem: I ran the benchmark you created for (a) on my machine, and after several reloads I have to say that sometimes pure js, and sometimes OpenLayers.Class wins. The benchmark you created for (b) is unfair, because we do not have 10000 base classes in the library, only (educated guess) about 20. And to compare OpenLayers.Class and pure js for classes that inherit from other classes, you would also have to call OpenLayers.inherit in case 2 of (b). But again, I'm not against removing OpenLayers.Class in 3.0. Thanks for the cooperation on this! We just made class instantiation 10 times faster.

You're right that my bench isn't representative of what we have in the lib, but the thing is: defining a type with OpenLayers.Class is ~13x slower than defining it with plain JavaScript. I'll try to come up with realistic benchmarks. I'd like to be sure that it's worth editing 250+ files :-)

[erilem]

Defining sub-classes with OpenLayers.Class is also much slower than when relying directly on OpenLayers.inherit (~10 times):

--- Type Definition ---
[Base Classes] OpenLayers.Class 66.45
[Base Classes] Pure JavaScript 4.2
[Sub-classes] OpenLayers.Class 277.25
[Sub-classes] Pure JavaScript 30.4

(perf-type-definition.html ).

But, yes, I still need to find out and show what it means in terms of loading/parsing OpenLayers.js.