I write comments rarely, as fas as the sole code of mine is concerned. But I do use a comment from time to time to appoint a command to myself that is some spooky-action-at-a-distance dependant and can couse me trouble to get oriented myself in shape of it.

I tend to comment things that I know wont be redone in a while, or things that simply take me too much time to re-understand once I re-visit them:

	@Override
	public void render ( final Bag<ShaderProgram> programs )
	{
		if ( renderQueue.isEmpty() )
		{
			return;
		}

		final UniformBlockUpdater<RenderTask>[] updaters = blockUpdaters.data();
		final int upSize = blockUpdaters.size();

		final RenderTask[] queue = renderQueue.data();
		final int quSize = renderQueue.size();

		final ShaderProgram[] progs = programs.data();
		final int prSize = programs.size();

		final int mbs = maxBatchSize;

		for ( int p = 0; p < prSize; ++p )
		{
			progs[p].bind();

			int taskOffset = 0;
			int instOffset = 0;

			do
			{
				int maxItem = quSize;
				int maxInstance = queue[quSize - 1].instanceCount;

				for ( int u = upSize; u-- > 0; )
				{
					final long encOffsets = updaters[u].batchUpdateInstanced
							( queue, quSize, taskOffset, instOffset, mbs );

					maxItem = unpackHigh( encOffsets );
					maxInstance = unpackLow( encOffsets );
				}

				final int lastItem = maxItem - 1;

				// If the updater couldn't advance past the current item.
				int baseInstOffset = (lastItem == taskOffset) ?
						maxInstance : queue[taskOffset].instanceCount;
				baseInstOffset -= instOffset;

				// Special case: First task draw.
				draw( queue[taskOffset], 0, baseInstOffset );

				for ( int i = (taskOffset + 1); i < lastItem; ++i )
				{
					final int ic = queue[i].instanceCount;
					draw( queue[i], baseInstOffset, ic );
					// Offset for next task.
					baseInstOffset += ic;
				}

				// Special case: Last task draw.
				if ( (taskOffset + 1) <= lastItem )
				{
					draw( queue[lastItem], baseInstOffset, maxInstance );
				}

				final int lastItemCount = queue[lastItem].instanceCount;
				// Completed drawing/uploading instances of last task.
				final int completedLastTask = maxInstance < lastItemCount ? 1 : 0;
				// If yes advance to next item, keep it as it is otherwise.
				taskOffset = maxItem - completedLastTask;
				// Wrap over offset so next item starts from zero instances.
				instOffset = (maxInstance % lastItemCount);

				// While we dont run out of tasks.
			} while ( taskOffset < quSize );
		}
	}

Honestly I never know what the fuck that loop is doing after I write it (in the various incarnations it had over time).

I tend to comment things that I know wont be redone in a while, or things that simply take me too much time to re-understand once I re-visit them:

	@Override
	public void render ( final Bag<ShaderProgram> programs )
	{
		if ( renderQueue.isEmpty() )
		{
			return;
		}

		final UniformBlockUpdater<RenderTask>[] updaters = blockUpdaters.data();
		final int upSize = blockUpdaters.size();

		final RenderTask[] queue = renderQueue.data();
		final int quSize = renderQueue.size();

		final ShaderProgram[] progs = programs.data();
		final int prSize = programs.size();

		final int mbs = maxBatchSize;

		for ( int p = 0; p < prSize; ++p )
		{
			progs[p].bind();

			int taskOffset = 0;
			int instOffset = 0;

			do
			{
				int maxItem = quSize;
				int maxInstance = queue[quSize - 1].instanceCount;

				for ( int u = upSize; u-- > 0; )
				{
					final long encOffsets = updaters[u].batchUpdateInstanced
							( queue, quSize, taskOffset, instOffset, mbs );

					maxItem = unpackHigh( encOffsets );
					maxInstance = unpackLow( encOffsets );
				}

				final int lastItem = maxItem - 1;

				// If the updater couldn't advance past the current item.
				int baseInstOffset = (lastItem == taskOffset) ?
						maxInstance : queue[taskOffset].instanceCount;
				baseInstOffset -= instOffset;

				// Special case: First task draw.
				draw( queue[taskOffset], 0, baseInstOffset );

				for ( int i = (taskOffset + 1); i < lastItem; ++i )
				{
					final int ic = queue[i].instanceCount;
					draw( queue[i], baseInstOffset, ic );
					// Offset for next task.
					baseInstOffset += ic;
				}

				// Special case: Last task draw.
				if ( (taskOffset + 1) <= lastItem )
				{
					draw( queue[lastItem], baseInstOffset, maxInstance );
				}

				final int lastItemCount = queue[lastItem].instanceCount;
				// Completed drawing/uploading instances of last task.
				final int completedLastTask = maxInstance < lastItemCount ? 1 : 0;
				// If yes advance to next item, keep it as it is otherwise.
				taskOffset = maxItem - completedLastTask;
				// Wrap over offset so next item starts from zero instances.
				instOffset = (maxInstance % lastItemCount);

				// While we dont run out of tasks.
			} while ( taskOffset < quSize );
		}
	}

Honestly I never know what the fuck that loop is doing after I write it (in the various incarnations it had over time).

To me that's exactly an example of code that should be rewriten:

You're declaring a lot of local variables that are only used once, why? For example how is this

final int prSize = programs.size();

for ( int p = 0; p < prSize; ++p )

Any simpler than this?

for ( int p = 0; p < programs.size(); ++p )

The later is shorter by 1 line, doesn't add extra code that does nothing and actually conveys the intent better as programs.size is clear while "prSize" could be anything, what is "pr", out of context you can't know.

Sames goes for "progs", "upSize" ,"updaters" and "mbs", seriously why create mbs, you're not even saving a call to find the size there you're just using mbs as is later, how is mbs ever going to be clearer than "MaxBatchSize"? The others are reused but i'm fairly sure their advantages are pretty meaningless over getting rid of the variable.

Why do you have a local variable named queue (and nothing else), hungarian notation is bad, yet it gives you the type and intent, the type is too much but here you have "only" the type, and you even have the wrong one as it's an array and not a queue, i can't think of any worse variable name. What's wrong with "renderTasks"? Isn't that clearer than "queue"?

To me that's a perfect example of cryptic code!

Just changing those few tiny things already help readability, the rest of the code is also pretty bad as it seems you're trying to use the arrays as queues, why aren't you simply actually using a queue then instead of all the cryptic special case looping code?

Overall this is exactly an example of what i mentioned, code that shouldn't be commented, but rewritten.

You're declaring a lot of local variables that are only used once, why? For example how is this



final int prSize = programs.size();

for ( int p = 0; p < prSize; ++p )



Any simpler than this?



for ( int p = 0; p < programs.size(); ++p )



The later is shorter by 1 line, doesn't add extra code that does nothing and actually conveys the intent better as programs.size is clear while "prSize" could be anything, what is "pr", out of context you can't know

What is wrong on cashing calls into local variables?! What if he decided out of nowhere to write this :

final int prSize = programs.size()*2;

for ( int p = 0; p < prSize; ++p )

In your solution you would use :

for ( int p = 0; p < programs.size()*2; ++p ) reevaluating at every cycle iteration? Even without that, your naive suggestion of -for ( int p = 0; p < programs.size(); ++p )- will rape cache at every iteration.

What is wrong on cashing calls into local variables?!

The compiler will do it for you. Write code for legibility, and only modify it for speed after you've profiled to determine that it's actually a bottleneck.

Your notions of what will happen to the cache are completely wrong, by the way.

What is wrong on cashing calls into local variables?!

The compiler will do it for you. Write code for legibility, and only modify it for speed after you've profiled to determine that it's actually a bottleneck.

Your notions of what will happen to the cache are completely wrong, by the way.

Is it by the way (does not get optimized and stored on stack) and still visiting a property on a distant object is cache ok?

Do you also expect following code to get optimized ...

var a=array[100]+arrayB[100]; // 2 page breaks

var b=array[101]+arrayB[101];//2 page breaks

.... into this ?

var tmpa10=array[100]; // page break

var tmpa11=array[101];

var tmpb10=arrayB[100]; // page break

var tmpb11=arrayB[101];

var a=tmpa10+tmpb10;

var b=tmpa11+tmpb11;

from my experience this could not happen becouse it would assume changing the order of [] operator instructions as programed, unless explicit , compiler would not dare to perform it.

What is wrong on cashing calls into local variables?!

The compiler will do it for you. Write code for legibility, and only modify it for speed after you've profiled to determine that it's actually a bottleneck.

Your notions of what will happen to the cache are completely wrong, by the way.

Is that guaranteed to be optimized by the compiler, but more importantly is that style actually safe to use in a modern multi-threaded environment?

The compiler would actually be forbidden from optimizing it in many cases, as C functions are not guaranteed to be pure. Only if the function is visible and can be inlined and it can be proven there is no aliasing there is a reasonable chance for it to be optimized.

You're declaring a lot of local variables that are only used once, why? For example how is this



final int prSize = programs.size();

for ( int p = 0; p < prSize; ++p )



Any simpler than this?



for ( int p = 0; p < programs.size(); ++p )



The later is shorter by 1 line, doesn't add extra code that does nothing and actually conveys the intent better as programs.size is clear while "prSize" could be anything, what is "pr", out of context you can't know

What is wrong on cashing calls into local variables?! What if he decided out of nowhere to write this :

final int prSize = programs.size()*2;

for ( int p = 0; p < prSize; ++p )

In your solution you would use :

for ( int p = 0; p < programs.size()*2; ++p ) reevaluating at every cycle iteration? Even without that, your naive suggestion of -for ( int p = 0; p < programs.size(); ++p )- will rape cache at every iteration.

Because this is premature optimisation leading to bad code, unless you think that the bottleneck of his problem is getting the size out of a list, but i quite doubt so.

Because this is premature optimisation leading to bad code

Bad code? Quite overwhelming statement about less compressed "readable" code, while it is not true either, becouse a local variable of native type like int -is more understandable when used than a some-function invoking. One is supposed to navigate to the invoked strangness all-over the place instead of being stacked int, to find out wheather it is intentional or "throw me optimize, readibility altar" case