electricessence / TypeScript.NET

A JavaScript-Friendly .NET Based TypeScript Library (Moved)
https://github.com/electricessence/TypeScript.NET-Core
Other
251 stars 36 forks source link

Unnecessary bitwise or operators #23

Closed rmcc13 closed 9 years ago

rmcc13 commented 9 years ago

System/Compart.ts, the CompareResult enum is using a bitwise operator for each value. These operators seem unnecessary because you have a literal number oring against a literal zero. I was just curious as to why the author decided to do this.

electricessence commented 9 years ago

You are correct. These are unnecessary. Early on in the project (sometime late 2014), I dug deep into 'integers'. Since an 'int' wasn't something available with TypeScript yet, I looked for the JavaScript equivalent. Result:

The following is treated as an integer by JavaScript compilers:

var a = 1 | 0;

Considering the possibility that by explicitly declaring an integer, I might gain some determinable performance improvement, I marked all integers with | 0.

The question remains, if there is any benefit at all doing this. I did some jsPerf tests but not in a way that compared the two.

I'm glad you noticed, and brought this to my attention. I will run some more tests and see if there is negative value at retaining them.

rmcc13 commented 9 years ago

OK cool, I didn't know that x | 0 evaluated to an integer. It would definitely be interesting to see the comparison of the two, because, for instance say if you had like 100 bitwise operations, I think it might be more a performance hit to do all those bitwise operations than just using numbers, especially if those numbers are only used once or twice. I might look into that, thanks for explaining your reasoning for doing that.

electricessence commented 9 years ago

Again, this was a great question that needed an answer, and here is the repeatable results and test: http://jsperf.com/integer-speed-test

I ran some tests locally as well on multiple browsers and everything was inconclusive or within margin of error.

So in the end: This is probably a totally unnecessary optimization which probably has little value and ultimately may confuse some people who see it in code. I will likely remove soon.

electricessence commented 9 years ago

OK. The unnecessary bits have been removed. Wanted to say thanks again for pointing it out. One thing that you should know is that any number with | 0 applied is converted to an integer. I started using it instead of Math.floor() because |0 is apparently is faster. Tested this on jsPerf.

So I added a new utility: https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Integer.ts