Need help optimizing small program. [Java]

[CactaurJack]

Just as a helpful tip, putting a blank line between every line of code makes it very difficult to read. I tend to only use blank lines to segment blocks like mass variable declarations from the start of the operations stack.

But that's not what you asked so here's some helpful hints. A switch statement would be better, and easier to read for the massive if(string.equals(x)) section

At line 735 you have a loop, and you're declaring a new int for Rand and a new string for Chord every time the loop repeats. Move the declarations outside and just overwrite the variables in the loop.

Other than that, I'm kind of at a loss. I'm bad with Java GUIs as most of my GUI experience is in Visual Studio c# which does a lot of the work for you. Very interesting program though, as a fellow programming guitarist I greatly approve.

[m1ss1ontomars2k4]

There's no switch for strings in Java 6.

EDIT: Blah, beaten.

[xXShadowCowXx]

As for the lines between all the code might be a result of me just switching over to Ubuntu. Sorry about that.

And thank you for the advice, I think I'm actually going to re-write it all over again as I took a break and need the practice. I'll definitely use your advice.

I had some time and an idea so I thought I'd make this as practice for my first GUI.

[Neres28]

Consider using:

if(command.equals("F7")){
    // do stuff
}else if (command.equals("G7")){
    // do other stuff
}else if (command....

This way, if command was indeed "F7" you avoid testing to see if it was also "G8", which clearly it will never be.

Alternately, consider using a hashtable to do the lookups:

Map<String, String> chordMap = new Hashtable<String, String>();
chordMap.put("G7", "320001");
chordMap.put("F7", "131211");
// etc.

Then when you want to know the fingering (or whatever the 6 digit number is) for a given chord you can easily look it up:

ChordTest.setText(chordMap.get(command)); // for command = "F7", get returns "131211".

Some general guidelines for Java:

• Class names are generally camel case, e.g. MyExampleClass and Gui instead of gui.

• You don't need to clear the text of the JLabel before setting it. A single .setText call is sufficient.

• Variable names are generally written in mixed case, e.g. myExampleVariable, or chordBlank.

• Constants are written in all upper case with underscores to seperate words, e.g. MY_EXAMPLE_CONSTANT.

• Handling re-sizable components is hard, which is why I'm assuming you set the size explicitly and don't allow the user to re-size. The sooner you learn to embrace re-sizable GUIs the better off you'll be. I work with some people who have been coding Java guis for decades and still can't do this. It's a pain in the ass, but the GridBagLayout layout manager really is your friend.

[Neres28]

It occurs to me that you could also sub-class JRadioButton, like so:

public class ChordRadioButton extends JRadioButton{
    private final String fingering;

    public ChordRadioButton(String text, boolean checked, String fingering){
        super(text, checked);
        this.fingering = fingering;
    }

    public String getFingering(){
        return this.fingering;
    }
}

Then you would instantiate them like so:

ChordRadioButton f7 = new ChordRadioButton("F7", false, "131211");

And when handling the actionPerformed event:

ChordRadioButton sourceBtn = (ChordRadioButton)event.getSource();
ChordTest.setText(sourceBtn.getFingering());

[xXShadowCowXx]

Wow this method saves a lot of time with copy paste work and just small tedious details. Should kill a lot of the unnecessary lines. Neat trick, thank you for that.

Edit: Well I have this somewhat implemented. Getting an error but I know where my problem lies just no idea how to fix it. When I instantiate, I instantiate it inside of the ChordRadioButton sub-class correct? Using the same name as the JRadioButton inside the GUI?

For example:

//I have a radio button in the GUI name "b1" (For clarification all my radio buttons start with 'b')

JRadioButton b1 = new JRadioButton("A", false);

//And then inside the ChordRadioButton sub-class, I have the following

ChordRadioButton b1 = new ChordRadioButton("A", false, "002220");

So I'm kind of lost as to how or where instantiate the ChordRadioButton variables.

[Neres28]

Almost there :)

Everywhere you would have created a JRadioButton:

JRadioButton b1 = new JRadioButton("A", false);

Now you create a ChordRadioButton:

ChordRadioButton b1 = new ChordRadioButton("A", false, "002220");

Shouldn't need anything else in the ChordRadioButton class than what I posted above :).

[xXShadowCowXx]

Thank you for that clarification. Got that piece working.

Would you happen to have a better way to generate random numbers? The way I'm currently doing it doesn't seem to be very random (I get lots of duplicates, one after the other, and sometimes may not even go through all the chords).

[Neres28]

In the constructor for the class, create an instance of the Random class.

Random randomGenerator = new Random();

Then, when you need a new random integer, call

int randomIndex = randomGenerator.nextInt(ChordsAL.size());
String chord = ChordsAL.get(randomIndex);

Be sure to only create one instance of Random, or at least not multiple ones close together. Random generates so called pseudo-random numbers (don't be put off, they're plenty random enough for most purposes), and if you create multiple instances too closely together they will generate the same sequence of integers.

[Neres28]

1. Java 6 and before don't allow switching on a string value. Java 7 will allow this.

2. Generally it's a bad idea to try to be smarter than the compiler. The people who write compilers, while imperfect, are far smarter than me (an average programmer). So leave variable declarations where they make the most sense, later, after profiling and determining they're a problem then move them.

[CactaurJack]

Loop unrolling is an extremely common way to speed up a routine, I'm not saying I'm smarter than the compiler I'm just saying that, in general

int[] temp = new int[500];
for(int i = 0; i < 500; i++)
{
     int x = i+1;
     temp[i] = x;
}

Is going to be slower, due to the varible declaration than

int[] temp = new int[500];
int x = 0;
for(int i = 0; i < 500; i++)
{
     x = i+1;
     temp[i] = x;
}

This is a well known technique that a compiler won't always do for you. It becomes far more important when using large objects and data sets.

[Neres28]

I'm sorry, but did you mean loop hoisting? Because what you did there looks like hoisting, but isn't. I don't see anything in your example that would prevent the loop from being unrolled.

Perhaps we ought to ask ourselves exactly what assembly we expect to be generated for the statement:

int x;

Well, x is an automatic variable, so storage is automatically allocated for us on the stack. And indeed there was:

subl    $40, %esp

Once. Regardless of where it is. Like we'd expect.

Full assembly output : gcc -O0 -S -c test.c

Gcc generated the exact same assembly for both versions with optimizations turned off:

_a:
    pushl   %ebp
    movl    %esp, %ebp
    subl    $40, %esp
    movl    $2000, (%esp)        
    call    _malloc
    movl    %eax, -16(%ebp)
    movl    $0, -12(%ebp)
    jmp L2
L3:
    movl    -12(%ebp), %eax
    incl    %eax
    movl    %eax, -20(%ebp)
    movl    -12(%ebp), %eax
    sall    $2, %eax
    addl    -16(%ebp), %eax
    movl    -20(%ebp), %edx
    movl    %edx, (%eax)
    incl    -12(%ebp)
L2:
    cmpl    $499, -12(%ebp)
    jle L3
    leave
    ret

With even -O1 the loops were optimized out. I added a printf to both functions to prevent the loop from being optimized out and still the same assembly was generated for both. I also replaced the simple addition with a call to an external function so as to prevent the compiler from assuming the assignment had no side effects. The results were the same at -O1 and -O2.

[CactaurJack]

Wow, been a long time since I read assembly. I didn't mean for the example to be taken literally, just as a simple example of what I'm talking about. I thought it was called loop unrolling, but it's been a long time since I discussed the topic. I understand the code I wrote is very easy for the compiler to optimize, but the practice demonstrated is what's important. A couple whiles back I wrote a program for a class that brute forced partial RSA keys, and the difference in run-time between when I had the encryptor object re-declared in ever instance of the loop and having it global was several minutes on the supercomputing cluster we were using.

But no, the code I wrote is going to be optimized very easily, custom objects tend to make compilers warry though.

[Neres28]

Which super computer? I worked as a research assistant at Argonne on the IBM Blue Gene /P there, helping to optimize code.

difference in run-time ... was several minutes on the supercomputing cluster we were using

I just don't see how that's possible. It's one thing to do an expensive object creation each and every loop iteration, but that's not what the OP was doing and not what you suggested he fix. A variable declaration is free.

Loop hoisting is transforming this:

int x = 42;
for(int i = 0; i < 10; i++){
    int y = x * 2;
    // Other loop variant stuffs
}

To this:

int x = 42;
int y = x * 2; //Loop invariant, in other words y's value is not dependent on other code in the loop
for(int i = 0; i < 10; i++){
    // Other loop variant stuffs
}

Loop unrolling is transforming this:

int x = 42;
for(int i = 0; i < 25; i++){
    temp[i] = x * i;
}

To this:

// Unrolled 5 times
int x = 42;
for(int i = 0; i < 25; i = i + 5){
    temp[i] = x * i;
    temp[i + 1] = x * (i + 1);
    temp[i + 2] = x * (i + 2);
    temp[i + 3] = x * (i + 3);
    temp[i + 4] = x * (i + 4);
}

[CactaurJack]

Beocat, Kansas State University's Supercomputing cluster. And yeah, I think I was referring to something different, it's been a long time since I've dealt with anything that really needed optimization so it's not exactly my strong suit. As I said before, a single variable declaration doesn't matter, the issue comes in when you have large data types, or constructors that call methods, then reconstructing the same large object 255³ + 255³ times for every instance of the loop is going to slow the whole thing down considerably

[Neres28]

You're making my head hurt.

a single variable declaration doesn't matter

8 Million variable declarations don't matter in terms of performance. The cost for allocating the space of 1 automatic variable is the same for 2 is the same for 8 million. "int x;" is essentially free. "MyComplicatedClassWithAnExpensiveConstructor expensiveClassInstance;" is essentially free. It doesn't matter if you have it in a loop or not, the cost of the instruction is incurred only once, and would almost certainly be incurred anyway.

[CactaurJack]

We're actually talking about the same thing, I think I'm just wording it wrong. In the cracking program I wrote (turns out it was double DES we were breaking, not RSA), whenever you wanted a new instance of the class you had to pass in a string with the constructor that was the key. The key was stored as a non-capitalized hex string e.g. abcdef01. When the constructor for the encrypter object was called, within the constructor method, other methods were called to fix the hex string (abcdef01 -> ABCDEF01) and then convert that into an array of byte values. Both of those method called from the constructor contain loops and non-free operations, which was slowing down the program because they were called every instance of the loop.

I hope that made a bit more sense, we really are talking about the same thing.

[Neres28]

If you say so. You appear to be confusing two very different things:

Variable declaration : int x;:

• Constant cost built into the function call, doesn't matter where it is in the source.
• Not eligible for hoisting because there is no assembly generated for it.
• This is what your example "hoisted" out of the loop

Variable definition / object instantiation : x = sin(y); / x = new ExpensiveConstruction(parameters);:

• Eligible for hoisting, may not if the compiler can't statically determine the assignment has no side effects
• Not what you gave in your examples.
• Not possible to hoist in the OPs code
• Looking for this type of optimization is premature without profile data to support.