System out in 3.0 beta branch

When I updated the 3.0 beta branch to the trunk merge of last weekend a system out was added.

Can you change line 356 of DesktopAssetManager.java from a system.out to an info log?



Besides this minor issue, I see a lot of improvement! (e.g. the new asset key and the speed in the motiontracker)



Regards,

Maxim

1 Like

Thanks. I committed the change to trunk. It should make it over to the stable beta branch update (not sure when that will be, but hopefully sooner rather than later).

@Sploreg said:
not sure when that will be, but hopefully sooner rather than later

I guess so, the update push provoked a few nice fixes already ^^

I reported that some weeks ago. Of course I was ignored. :stuck_out_tongue:



mutters something about black sheep

I think I just heard something… maybe it was the wind.

cough

Now on page 22

you should get some medication for that cough madjack

@madjack said:
*cough*
Now on page 22


From that message buried on the second page of a thread no one was reading I guess. :) :) :)

logger.log(Level.INFO, "Loaded {0} with {1}", tex + " - " + tex.getMinFilter());
I can commit if needs be.


"I can commit if needs be." And we can revert it if we don't like it. :) Though I think the log statement is broken since it references two args and only has one. But a minor thing.

I do think I had tested it though… shrug



I’ll retest and commit (since it’s already modified).

You were right.



Changed it to:



[java]

logger.log(Level.INFO, “Loaded {0} with {1}”, new Object[]{tex, tex.getMinFilter()});

[/java]



I’ll commit.

Pfffffffff. I hate Ninjas. :stuck_out_tongue:

@madjack said:
You were right.

Changed it to:

[java]
logger.log(Level.INFO, "Loaded {0} with {1}", new Object[]{tex, tex.getMinFilter()});
[/java]

I'll commit.


Cool. This why you almost never see the two arg form. It's going to create a new object every time whether INFO is on or not, in this case. So most would just opt to concatenate the string and forgo the {} arg stuff. The idea behind the one arg "{0}" style ones is that you avoid a string creation if INFO level is not being logged. No way to avoid it with a "{1}" unfortunately.
@Sploreg said:
Thanks. I committed the change to trunk. It should make it over to the stable beta branch update (not sure when that will be, but hopefully sooner rather than later).


*shakes head* Why didn't I see this in the first place? :P

Thanks Paul. Noted.

@pspeed said:
Cool. This why you almost never see the two arg form. It's going to create a new object every time whether INFO is on or not, in this case. So most would just opt to concatenate the string and forgo the {} arg stuff. The idea behind the one arg "{0}" style ones is that you avoid a string creation if INFO level is not being logged. No way to avoid it with a "{1}" unfortunately.


Well the string concat will create a new String object, the Object[] format will create a new Object[] so you lose either way. The advantage of Object[] is that the string creation and concat are only done if actually needed (and considering that toString() could itself be potentially quite an expensive operation and involve yet more object creations in the form of stringbuilders/whatever) that can be a nice saving.

If really worried (for example doing a lower-than-info level logging in a frequently called location) then you should check if logging is enabled for that level before doing the logging line at all.
@zarch said:
Well the string concat will create a new String object, the Object[] format will create a new Object[] so you lose either way. The advantage of Object[] is that the string creation and concat are only done if actually needed (and considering that toString() could itself be potentially quite an expensive operation and involve yet more object creations in the form of stringbuilders/whatever) that can be a nice saving.

If really worried (for example doing a lower-than-info level logging in a frequently called location) then you should check if logging is enabled for that level before doing the logging line at all.


My point was that for log( INFO, "Message {0}", someArg ) then no additional creation will be done if INFO is off.

For either log( INFO, "Message {0} {1}, new Object[] { someArg1, someArg2 } ) or log( INFO, "Message " + someArg1 + " " + someArg2 )... an extra object is created whether INFO is on or not. But I think the string concatenation case looks clearer or is less messy.

Otherwise, as you said, wrap it in an isEnabled() call or when reasonable, split it into two calls.
@pspeed said:
My point was that for log( INFO, "Message {0}", someArg ) then no additional creation will be done if INFO is off.

For either log( INFO, "Message {0} {1}, new Object[] { someArg1, someArg2 } ) or log( INFO, "Message " + someArg1 + " " + someArg2 )... an extra object is created whether INFO is on or not. But I think the string concatenation case looks clearer or is less messy.

Otherwise, as you said, wrap it in an isEnabled() call or when reasonable, split it into two calls.


Ahh, I see what you mean. Yes for the single argument form passing an object will avoid the toString() and string concatenation in the same way as Object[] will. It doesn't actually avoid the object creation though - it just delays it:

[java]
public void log(Level level, String msg, Object param1) {
if (level.intValue() < levelValue || levelValue == offValue) {
return;
}
LogRecord lr = new LogRecord(level, msg);
Object params[] = { param1 };
lr.setParameters(params);
doLog(lr);
}
[/java]

[java]
public void log(Level level, String msg, Object params[]) {
if (level.intValue() < levelValue || levelValue == offValue) {
return;
}
LogRecord lr = new LogRecord(level, msg);
lr.setParameters(params);
doLog(lr);
}
[/java]

In one case, if logging is off you get no extra creation. In the other case, if logging is off then you do get creation. (one arg versus two I mean).



And actually, I was wrong in the string + string case because two objects are created. The StringBuilder and then the String that it is converted to. But by the time you worry about that, better to wrap it in an isEnabled() anyway.



Edit: plus all the toString() calls in the string + string case. I’m too tired to be answering forum posts. :slight_smile:

Hmm, when I posted this minor sys.out bug I did not expect to get 17 reactions next time I checked the forum! But thanks for fixing it, although I have to wait for the next update :wink:

1 Like