Monday, August 08, 2011

Please Stop Reassigning For No Reason!

I swear it is was a short one but a must write and yes, once again, since I keep seeing this kind of mistake everywhere!
This is not about JavaScript, this is about programming whatever language you want ... if you do this, you are doing it wrong!

The Problem

JS Engines developers are desperate! They are even posting how to help JIT compilers to go faster and developers seem to be so lazy that even most basic good practices are avoided.

Performances a part, this pattern can be also dangerous, specially in this ES5 era where getters and setters are extremely common, specially on mobile browsers where nobody cares about IE gap.

I am looking at you only if you are still writing something like this in your code:

window.whatever = window.whatever || {
/* the whatever it is, object, function, anything */
};

where window is just the generic object example.

OMG, What Can Be Wrong ...

Everything! Any object property could be a native or user defined setter. In this case above technique is invoking the potential setter passing through the potential getter.
This simply means that:

  • accordingly to the task, we are asking N extra computations or function invokes for no reason

  • the setter may accept something different from what the getter returns. If this is true, result could be an error, rather than a "smart assignment"

  • if the setter was set already and it was a lazy re-assignment logic, we are avoiding lazy assignment features/logic requiring it's execution instantly and, once again, without any reason

  • if the object has getter only the operation will throw an error while setting


No getters or setters? It does not matter since in this way any property has to be retrieved and, if present, has to be reassigned to itself.

How JavaScript Engines Work

Thanks for asking. Let's imagine every object refers behind the scene to a stack of strings representing the list of properties. This stacks is used to know if object.hasOwnProperty("propertyName") or if "propertyName" in object is present. Once decided if the object can access the propertyName, a sort of -1 < objectProperties.indexOf("propertyName") procedure, the property has to be retrieved and eventually unboxed. Once this part is complete, the reassignment does not care much about the same property. Here comes the propertyName to propertyValue procedure which most likely will "erase" the older reference to reassign the new one. Even if the engine is truly smart, all possible checks/logics about memory address for the value and property name for the object has to be executed.
If you don't believe me you can simply check the Webkit engine Object source code and compare operations needed to set and get VS JSObjectHasProperty, basically just a call to jsObject->hasProperty rather than a whole logic moved via JSObjectGetProperty plus JSObjectSetProperty, and of course, invoking hasProperty as well.

How To Do The Same Better And Faster


"whatever" in window || (window.whatever = {
/* the whatever it is, object, function, anything */
});

// eventually easier to shortcut via minifier
var key = "whatever";
key in window || (window[key] = {
/* the whatever it is, object, function, anything */
});

// eventually easier to use but requires potentially extra memory
// 'cause the assignment has to be created in any case
// and it can't be ignored as it could be with precedent examples
function setDefault(object, key, value) {
key in object || (object[key] = value);
// here we can play with the returned value
// I chose the function itself but it can be anything
// or nothing if you prefer
return setDefault;
}

setDefault(
window, "whatever", {}
)(
window, "somethingElse", function(){}
)(
window, "howCoolIsIt", "very cool!"
);


The reason it's better is simple: no setters or getters invoked plus no redundant+superfluous operations performed over *reassignment*. Above snippet will simply invoke JSObjectHasProperty and jsObject->hasProperty so that the whole setter logic will be executed only if necessary and in any case no getter logic will ever be involved ... got it?

It Does Not Work

Oh ... Really? Most likely it's a browser bug you should file ASAP and even most likely a new feature not there yet. The in operator should always work as expected indeed with or without inheritance involved.

var o = Object.defineProperty({}, "test", {
enumerable: false,
writable: false,
configurable: false,
value: "OK"
});

alert("test" in o && o.test); // ... guess what ...
// OK

Since defined properties respect the in operator, nobody blocks us to define them using same pattern.

"prop" in object || Object.defineProperty(
object, "prop", descriptor
);


I Have To Do Refactoring

Good, so start with this RegExp to search the evil code in all your failes:

/([$_0-9a-zA-Z]+(?:\.[$_0-9a-zA-Z]+|\[[$_0-9a-zA-Z]+\]))\s*=\s*\1/
.test(fileText)

// or suggested by @joseanpg
/([$\w]+(?:\.[$\w]+|\[[$\w]+\]))\s*=\s*\1(?:[\s\|&,;]|$)/m

and modify accordingly. The replace RegExp requires unfortunately the end of the assignment so it may be weak.

I Kill A Kitten Each Time I Read That

This is what happens if you don't start now with the right way so, please, THINK ABOUT KITTENS!



Update On Alternative Ways

As @diegoperini pointed out in this tweet another way to avoid the setter is:

window.propertyName || (window.propertyName = {
/* the whatever value */
});

However, above technique still invokes the getter, either defined by the user or behind the JavaScript scene ( in core ).
My first suggestion avoids "empty getters" so even if this latter technique may be considered a better approach, it can still suffers or imply side effects.

Update On False Positives
As Diego pointed out the "prop" in obj may result into a false positive if obj.prop is assigned but it is falsy. I consider this really an edge case and even such pattern will be error prone if the value is truish but not the expected one.

this.prop || (this.prop = {});

// now imagine before
this.prop = true;

// bye bye library

There is actually no easy or fast enough way to compare two clones runtime in order to understand if the assigned object is the one expected. Also at that point this extra overhead would be unnecessary since re-assignment will be just faster.
At the end of the day it's up to us to be as safe as possible still preserving common sense and good programming logic but don't tell me I am talking about premature optimizations because all I am talking about is logic over a pattern that is similar in size with the one I am suggesting but it's completely different in therms of required operations and destroyed possibilities.

We should never abuse bad practices behind the "premature" flag!

Update With Benchmark


As asked via comments, I have created a jsperf benchmark. Please bear in mind the used test is not a real use case while described problems are still the same I have been talking about in this post.
JIT compilers may optimize repeated code and this is most likely what happens with the commonPattern approach.
I love the fact the benchmark basically proves me wrong, when getters and setters are not involved on JS side, because it's kinda illogical accordingly with browsers implementations.
Said that, if anybody from Chrome development would be so kind to comment why re-assignment is faster than just in it would be really nice.

Conclusions

100000 VS 2000000 ops for non real cases are not so useful to analyze but what should be underlined is that specially with getters and setter the in operator is both faster and safer cross browser and cross platform.

As example, there is only one proper way to shim Array.isArray.
If we use return typeof obj.length == "number" rather than toString.call(obj) == "[object Array]"we will surely go faster but we will do it totally wrong as well.

Last note about Webkit, we have to deal with it since it has the majority of mobile browsing market share. Special case is webOS which implements V8 rather than JSC but still, don't be blind in front of millions, just understand side effects the common pattern could cause against the right way to know if a property has been set already.

25 comments:

Lea Verou said...

Oops. Guilty as charged!

Although I generally try to avoid it, for the very reasons stated here. I'll avoid it more in the future. :)

Christopher said...

I love it!!! Thanks for showing me a nice use case for chaining function calls! Classic andrea giammarchi style!

Nicolas said...

Great post, would love to see some JSperf tests on comparing different ways of assignments :)

Ionut Popa said...

S do you recommend using define property to avoid the opera browser bug or just use in directly?

Andrea Giammarchi said...

@Nicolas I linked Webkit source on purpose, we don't need a bench here, we can read how many operations are done behind the scene already plus the rest of the downsides ;-)

@Ionut Popa Opera devs instantly fixed it so it's a non problem now, stick with the suggested pattern, test as much as possible, file bugs if any but I bet you won't have to

Christoph said...

Great post!

Webkit source is linked - but what about other browsers? I'd still like to see some JSperf tests on this...

Ionut Popa said...

What about
var prop = window.prop || {}; in the global ns
?

Andrea Giammarchi said...

right ... created jsperf benchmark.

Please read the description since I have added another point to avoid the common, and *wrong*, pattern, thanks.

Andrea Giammarchi said...

Ionut I will create another version with your suggestion rather than functions.

Let's see how different will be the results, thanks.

Anonymous said...

I was about to ask the same as Ionut, only with a slight difference: "var prop = prop || {};" because you can see this as a namespacing pattern on quite some sites. So it would be very interesting to know if it comes with exactly the same shortcomings.

Mathias said...

One of the first jsPerf test cases ever (created while it was still in development!) was about conditional assignment (the easiest solution in this article): http://jsperf.com/conditional-assignment

Andrea Giammarchi said...

Anonymous, Mathias, as tweeted already in that case unless defined in the window/global object no getters/setters impact, more scope lookup (which is local in that case).

The whole point here is about the disaster prone approach a simple and too common pattern could cause on both user defined and core defined getters and setters through property access and/or assignment.

Also we can see in my test performances drop like hell when getters or setters are involved and the in operator wins cross platform and cross browser.

In the old assignment test from Mathias we can see performances there are almost equivalent and moreover, there are not concrete side effects adopting one pattern rather than other.

In that case I would justify the style, if logic and consistent, but in this thread there is a different problem hopefully fully explained.

Thanks Mathias to help my test page have a cleaner description ;)

Andrea Giammarchi said...

P.S. in this ES5 period getters and setters are *more* *than* *common* and specially for mobile where nobody cares about missing IE features ;-)

Paul Irish said...

The pattern that's being discussed in this post is commonly executed a single time during a page (or module) load. As such, it seems more fruitful to discuss perf of js patterns that are found in high volume code paths. :/

Andrea Giammarchi said...

Paul, sad to read it from you ... this post is not about performances only, this post is about how many things could possibly go wrong with the discussed pattern.

Please don't stop at the first paragraph, take 10 minutes and read 'till the end starting from

"OMG, What Can Be Wrong ..."

Paul Irish said...

Sorry sorry.

I really like `'key' in window || (window.key = ... );` myself and think its wise to just switch to it for this common pattern.
+1

Btw here's a slightly better link for the webkit source, as you get syntax highlighting and nice navigation for free: http://goo.gl/YeCz0 (google codesearch)

Andrea Giammarchi said...

oh ... now we talk :D

I have updated the post using your Google link, thanks!

Dua Mairaj said...

Very nice sharing more like that on tutorialsworkshop

Oliver said...

It's interesting that even a short code snippet like the common pattern comes with so many (possible) implications.
I used it on a big e-commerce site and for a small project for namespacing, but switched to the in-operator solution now. Thanks for bringing that to my attention, Andrea. One kitten less you have to kill ;)

check_ca said...

Can someone tell me if it is wrong to write:

if (typeof window.whatever == "undefined")
window.whatever = {
/* the whatever it is, object, function, anything */
};

Andrea Giammarchi said...

still getter invoked ... example:

Object.defineProperty(window, "whatever", {
get: function () {
alert("Yes, Sir?");
}
});
if (typeof window.whatever == "undefined") {}

check_ca said...

Hum, you're right. Now I realize it was obvious the getter will be called to determine the type of whatever property...

Thanks :)

glathoud said...

Andrea, simply thanks for the great post!

Oliver said...

Andrea, since jshint tells me "Expected an assignment or function call and instead saw an expression" when using
"OBJ" in window) || (window.OBJ = {};), and I see no way to get rid of this by using one of the options, I wonder if the following would be OK too (i.e. not invoking an existing getter or something): if (!("OBJ" in window)) {window.OBJ = {};}

Andrea Giammarchi said...

yep, that's lint friendly and same logic ;)