_getScriptLocation appears to be a performance bottleneck

[tmcw]

Hi, I've started doing a bit of profiling of the OpenLayers code and it appears that _getScriptLocation in lib/OpenLayers/Util.js is something of a low-hanging fruit. Every time that it's called, it creates and runs a regular expression over every script element found in the document's header. Except in the most extreme of extreme cases, the value it returns will not change, but in the current implementation it is not cached, and it is called quite a few times - enough that it's at the top of the time taken by OpenLayers. It seems like a simple fix would be to store the value of _getScriptLocation in the window element and check for its existence before running the logic again.

[crschmidt]

This seems fine, though I'm curious in what environment you're seeing this actually taking a significant portion of time. I'm willing to accept it; most of our tests are on very small sets of page html, which makes navigating the dom cheaper, but I will say the reason we haven't optimized it is because I've just never seen it on any of our profiles.

[elemoine]

Replying to tmcw:

Hi, I've started doing a bit of profiling of the OpenLayers code and it appears that _getScriptLocation in lib/OpenLayers/Util.js is something of a low-hanging fruit. Every time that it's called, it creates and runs a regular expression over every script element found in the document's header. Except in the most extreme of extreme cases, the value it returns will not change, but in the current implementation it is not cached, and it is called quite a few times - enough that it's at the top of the time taken by OpenLayers. It seems like a simple fix would be to store the value of _getScriptLocation in the window element and check for its existence before running the logic again.

this could be cached as a property to the _getScriptLocation Function object (using arguments.callee), see OpenLayers.Geometry.fromWKT as an example.

[fredj]

cache the function result (with arguments.callee). 10000 call take 1440ms before and 100ms after.

[tmcw]

Great - just for reference, the reason why this popped up is mainly that it shows up as high on the examples in Google Chrome:  http://skitch.com/tmcw/np7ud/developer-tools-http-openlayers.org-dev-examples . This is after a run of just zooming on a point on the map. Since getImagesLocation is called during the spiralTileLoad method whenever clear() is called on a tile, and that calls getScriptLocation, that's why it became a big problem.

I'll keep scanning the code for more possible optimizations.

[ahocevar]

same as above, but using the private scope of OpenLayers.js instead of arguments.callee

[ahocevar]

Using arguments.callee is unnecessary here, because we have a private scope in OpenLayers.js (see e.g. singleFile, which is also in the private scope).

Please commit attachment:openlayers-2389.patch if you agree with the change.

[fredj]

You're right. And it's much readable ...

[fredj]

(In [9935]) cache the result of the OpenLayers._getScriptLocation function. r=ahocevar (closes #2389)

[elemoine]

I would have used

if(scriptLocation === undefined) {

rather than

if(scriptLocation) {

to take into account the case where scriptLocation is set to "".

[fredj]

Very good point Eric, I'll change it.

[fredj]

(In [9936]) Cache the function result if scriptLocation is . r=elemoine (closes #2389)

[fredj]

tests/OpenLayers.html is broken

[fredj]

add a force param to the function. If true dont' use the cache and recompute the location.

[pgiraud]

Are there any cases where we want to modify the scriptlocation ? I don't think so.

Then, if the force argument is there only to ensure that the tests pass, I think it would have been better to manually delete the scriptlocation variable before we call _getScriptLocation.

Does it make sense ?

[fredj]

Replying to pgiraud:

Are there any cases where we want to modify the scriptlocation ? I don't think so.

Only in the unit tests ...

Then, if the force argument is there only to ensure that the tests pass, I think it would have been better to manually delete the scriptlocation variable before we call _getScriptLocation.

Good point but it's not possible: _getScriptLocation is outside window.OpenLayers

1. Move _getScriptLocation into window.OpenLayers ? (just like _scriptName)
2. Use the force param ?
3. Remove the unit test ?

[pgiraud]

patch with modified unit tests

[pgiraud]

With this patch, tests pass on FF3.5, IE7 and IE8. They still don't pass on Chrome and fail on the search string thing, but I'm wondering if OpenLayers _getScriptLocation work on Chrome if there a search string in the url.

[pgiraud]

If everybody is happy with this patch, I would like to commit it so that we have passing tests.

[ahocevar]

I am not happy with this patch, as it removes an important unit test. If the tests pass in chrome before r9936, then we should revert r9936 until a new solution becomes available. If tests also did not pass before, a separate issue should be created, containing a patch with the unit test that is now about to be removed, so it does not get lost.

[fredj]

So why not move the scriptLocation var into window.OpenLayers (just like _scriptName) ?

It's certainly not ideal to change to code to make the unit test pass but it's simple.

[pgiraud]

I first would like to know if chrome support the "OpenLayers.js?foo=bar".

[ahocevar]

Replying to pgiraud:

I first would like to know if chrome support the "OpenLayers.js?foo=bar".

Exactly. That was the rationale behind my comment.

[ahocevar]

With this one we are able to find out whether a search string makes things break in Chrome or not.

[ahocevar]

The above patch should do the trick. pgiraud, please commit if you agree.

[pgiraud]

(In [10066]) fixed the unit tests on OpenLayers.js (broken since r9935), patch from Andreas who found a nice and clean way to bypass the cache system on _getScriptLocation for the tests, r=me,fredj (Closes #2389)