<< What's In a Workflow Queue Name? | Home | Working MasterPage Samples >>

What's Wrong With This Code? (#8)

Joe Developer is working with a simple struct:

struct Point
{
    public int x;
    public int y;
}

Joe's tech lead asked him to write a method that will return an array of 10,000 initialized points. Joe wrote following code.

Point[] CreatePoints()
{
    Point[] points = new Point[10000];

    for (int i = 0; i < points.Length; i++)
    {
        points[i] = new Point();
    }

    return points;
}

The code doesn't create any runtime errors, but Joe is worried because his tech lead looked at the code and frowned. What could provoke such a reaction?

Hint: Joe's lead is a performance nut.

Print | posted @ Tuesday, October 24, 2006 3:34 AM

Comments on this entry:

Gravatar # re: What's Wrong With This Code? (#8)
by Alan Dean at 10/24/2006 3:48 AM
The iteration is unnecessary - structs are value types.

The declaration:

Point[] points = new Point[10000];

will call the default ctor.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Anthony Mills at 10/24/2006 4:13 AM
Given that he's working with structs, the memory is zeroed anyway and "return new Point[10000];" is sufficient?
  
Gravatar # re: What's Wrong With This Code? (#8)
by Geoff Appleby at 10/24/2006 4:39 AM
Ha! That's a simple one :)

Because Point is a struct, it's a valuetype, which means it ALWAYS exist, it doesn't need to be new'ed.

As a result, all he needed was:

Point[] points = new Point[10000];
return points;

  
Gravatar # re: What's Wrong With This Code? (#8)
by Jeff at 10/24/2006 4:50 AM
Unless I'm missing a shorter way, each element of the array must be created via "new" so the structure is valid. This means we can't escape touching each index of the array. It feels like a naive answer to me, but just unroll the loop. Perhaps mod the length of the array by 10, allocate ten points per loop iteration and handle any extra indices after the loop (incase the length isn't evenly divisible by 10).
  
Gravatar # re: What's Wrong With This Code? (#8)
by Eber Irigoyen at 10/24/2006 4:52 AM
Joe's lead was pissed because Joe didn't initialize the structures (instances were created but not initialized)
  
Gravatar # re: What's Wrong With This Code? (#8)
by Haacked at 10/24/2006 5:09 AM
Joe should have done this instead, since it accomplishes the same thing. Instead he creates 10,000 points twice.

Point[] CreatePoints()
{
return new Point[10000];
}
  
Gravatar # re: What's Wrong With This Code? (#8)
by stej at 10/24/2006 5:31 AM
Hi, is the problem in "return points;"? I think the whole array will be copied (twice?)since the Point[] is array of structs.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Phil Weber at 10/24/2006 5:40 AM
The loop is superfluous. Structs are value types, and as such do not need to be initialized with 'new'.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Caffenero at 10/24/2006 6:26 AM
Has it something to do with the fact that Structs and arrays are passed by value? :)

So, maybe:

void CreatePoints(out Point[] result)
{
result = new Point[10000];

for (int i = 0; i < points.Length; i++)
{
result[i] = new Point();
}
}
  
Gravatar # re: What's Wrong With This Code? (#8)
by Wilhelm Svenselius at 10/24/2006 6:28 AM
Perhaps calling 'points.Length' on each iteration is a bit of a waste, since we know the size from the start. We could just put '10000' as the loop limit or put it in a separate const int and use it for both the array creation and loop limiter.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Adrian. at 10/24/2006 6:42 AM
I would have said that in this case initialising the structs in the for loop is redundant.

  
Gravatar # re: What's Wrong With This Code? (#8)
by Eric at 10/24/2006 7:03 AM
... Because those 10,000 comments will be physically copied in memory when returned, instead of referenced?
  
Gravatar # re: What's Wrong With This Code? (#8)
by Laurent at 10/24/2006 7:08 AM
Because Point is a structure, considered as a value type, an instance exists as soon it is declared, so the 1-10000 loop is unusefull ?
  
Gravatar # re: What's Wrong With This Code? (#8)
by JP at 10/24/2006 7:19 AM
When the Array is created, it is already initialized with a point in each slot, with x=0 and y=0 so the loop has no use.
Although, it takes 10 000 000 points in the array to waist 125 ms on my system.
  
Gravatar # re: What's Wrong With This Code? (#8)
by fromaustria at 10/24/2006 7:22 AM
just a wild guess:
array of points gets created on the heap.
new Point() creates a value type on the stack.
points[i]=... copies the Point from the stack to the heap.
better: return new Point[10000]; // done.


  
Gravatar # re: What's Wrong With This Code? (#8)
by Duckboy at 10/24/2006 7:43 AM
Dude, you're allocating and initializing all your points twice!

Point is a value type, so the new Point[10000] will actually allocate them all and set their x and y fields to 0. Then, the for loop allocates and initialializes them again, unnecessarily.

So, you're wasting valuable CPU time, causing the GC to have a minor anurism and ultimately using more electricity, therefore hurting the environment.

No sir, I don't like it!
-- Mr. Horse (from Ren & Stimpy)
  
Gravatar # re: What's Wrong With This Code? (#8)
by James Newton-King at 10/24/2006 7:45 AM
The Points are automatically initialized when the array is created. The loop is unnecessary.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Alexey at 10/24/2006 7:56 AM
The only thing I can think of is calling points.Length 10000 times, but won't the compiler inline it? Oh, Point is a struct, not a class, so we don't need to call the constructor 10000 times.
  
Gravatar # re: What's Wrong With This Code? (#8)
by fireping at 10/24/2006 8:03 AM
Property value points.Length is calculated every iteration while you know exact array length.
  
Gravatar # re: What's Wrong With This Code? (#8)
by jayson knight at 10/24/2006 8:26 AM
Point[] points should be initialized to 10,000 outside the scope of the CreatePoints method, i.e. Point[] CreatePoints(ref Points[])
  
Gravatar # re: What's Wrong With This Code? (#8)
by Martin at 10/24/2006 8:58 AM
Value types (structs) are 'auto allocated'.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Andrew at 10/24/2006 9:01 AM
I'd guess it's to do with the use of a for loop instead of do until
  
Gravatar # re: What's Wrong With This Code? (#8)
by Saamorim at 10/24/2006 9:48 AM
Well, it simple.

Why use the for?
The simple fact that the Joe’s Developer as created the array of Point structs, doesn't required him to create each of the Points. This is because the compiler already allocates the necessary space to accommodate the structs. This, however, didn't work if the Point object was a class. In that case it was necessary to create each of the point objects.
This is basically the biggest difference between a class and a struct?

Am I correct?

Regards
  
Gravatar # re: What's Wrong With This Code? (#8)
by Mitch Wheat at 10/24/2006 10:18 AM
Because Point is a defined as a struct it is a value type. Therefore, the line

Point[] points = new Point[10000];

does not just allocate space for an array of pointers to points, it actually allocates the space for them. The loop and subsequent allocations are unnecessary.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Wesley at 10/24/2006 10:36 AM
Theres no need to construct a new point in every loop.

Since the point is a struct (=value type) Joe could create one new Point() before the start of the loop and asign it to every point in the array. Since it's a value type the Point will be copied to the array.

Cheers,
Wes
  
Gravatar # re: What's Wrong With This Code? (#8)
by Wesley at 10/24/2006 10:57 AM
Second thought: There's no need for the loop at all. Structs(value types) are initialized at the moment they're created.


Cheers,
Wes
  
Gravatar # re: What's Wrong With This Code? (#8)
by Skup at 10/24/2006 11:07 AM
Arrays are automaticaly filled whith 0's when initialized. Calling new Point() on each element of the array just fill it again with 0's which is useless.

The developper did that because when initializing a struct on the stack, it's not automatically filled with 0's (contrary to reference types), so you have to call at least the parameterless constructor to initialize it.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Christiaan at 10/24/2006 11:20 AM
Well my gueuss is that Joe doens't need to initialize 10000 points as Point is a value type and must not be initialized.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Sahil Malik at 10/24/2006 12:54 PM
Joe should be worried because his lead cares more about performance than anything else.

:)
  
Gravatar # re: What's Wrong With This Code? (#8)
by Scott at 10/24/2006 1:44 PM
I apologize that I have to moderate comments. There were lots of good comments overnight but they all appear at once- in the morning when I catch up.

A lot of people pointed out that the for loop isn't needed. Since Point is a struct (a value type), creating the array is enough to give us 10,000 zero-initialized points.

The array is created on the heap, so it is ok to return a reference to the calling method.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Geoff Appleby at 10/24/2006 2:19 PM
Yeah, what's with the moderation dude? Maybe you need Gaptcha! :)
  
Gravatar # re: What's Wrong With This Code? (#8)
by scott at 10/24/2006 2:24 PM
Yeah Geoff - I need something. If I don't moderate this place would be filled with spam. I did have a captcha control installed previously but got some negative comments about it.

Perhaps Gaptcha is better than what I was using. I'll have to give it a look.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Geoff Appleby at 10/25/2006 11:54 PM
Well, the source is available for you now, although it looks like you're running CS 1.1 (i'm guessing) and I wrote it as a 2.1 addin for .net 2. Is an upgrade worth it? (Hell yeah! :)
  
Gravatar # re: What's Wrong With This Code? (#8)
by Tin Bui at 11/14/2006 1:07 PM
omg,
finally, I have finished reading the 33 comments above.
Guys, look at the hint from the author. Regardless of whether or not you want to do a loop for initialization, just say we want to do the loop, then 10,000 would make a performance nut really goes nut.
However, against the author's intention, I would not be thinking about making the looping any faster, but to asynchronously do the loop on another thread and go do something else.


Hi Geoff, how are you?
  
Gravatar # re: What's Wrong With This Code? (#8)
by AJ at 11/20/2006 2:11 PM
The (entry level example) author wants an array of 10,000 points, 2 integers each. The best way is to malloc the block, (initialized to 0) and use a pointer to a struct of type point to access the array.

It does however, make sense, especially on complex projects, to make the first write of the code as clear as possible (as this is), with little regard to performance, and then increase performance as needed.

Code "sevicablility" is a paramount issue to support people, however, lack of clarity as we all know, adds to R&D job security.

Performance, unfortunately, often goes hand-in-hand with lack of clarity.
  
Gravatar # re: What's Wrong With This Code? (#8)
by Gaurav at 12/13/2006 10:37 AM
I think calling points.Length 10000 times is not a good idea and it will have a performance hit. Rather than code should be something like this.

Point[] CreatePoints()
{
Point[] points = new Point[10000];
int length = points.Length;

for (int i = 0; i < length; i++)
{
points[i] = new Point();
}

return points;
}
  

Your comment:

Title:
Name:
Email:
Website:
 
Italic Underline Blockquote Hyperlink
 
 
Please add 3 and 5 and type the answer here: