Nxt source code analysis (QA) - page 4.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Jean-Luc on January 05, 2014, 04:25:48 PM

Quote from: Jaguar0625 on January 05, 2014, 03:28:52 PM

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

In 0.5.0 this looks like:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
transaction.index = transactionCounter.incrementAndGet();

I am afraid CfB and BCNext may get mad at me, but 0.5.0 relies heavily on the Concurrent API...

in your dreams.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Jean-Luc

sr. member

Activity: 392

Merit: 250

Quote from: Jaguar0625 on January 05, 2014, 03:28:52 PM

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

In 0.5.0 this looks like:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
transaction.index = transactionCounter.incrementAndGet();

I am afraid CfB and BCNext may get mad at me, but 0.5.0 relies heavily on the Concurrent API...

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: tacotime on January 05, 2014, 04:20:37 PM

Quote from: User705 on January 05, 2014, 04:12:20 PM

Quote from: FrictionlessCoin on January 05, 2014, 04:03:08 PM

Quote from: Come-from-Beyond on January 05, 2014, 03:58:29 PM

Quote from: tacotime on January 05, 2014, 03:53:29 PM

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

No, it's more like, if I see some kind of error, let me keep quiet about it!

Can you explain how that would effect the actual functionality of the system. Isn't it the same as someone submitting improperly signed transactions so those should be ignored? You keep bringing up issues with the code but without actual explanations of how those issues are supposed to screw things up.

It depends where you put it... it's dangerous in most places though because it doesn't class or report the exception and prevent you from seeing any other bugs that might arise while a function is executing. Basically: "Regardless of the error that arises, keep running and do not report it."

http://stackoverflow.com/questions/2416316/why-is-the-catchexception-almost-always-a-bad-idea

It's what I write when I'm in a rush and don't really care when I get some kind of error so long as the program does more or less what I want it to; ideally you would address it properly, but if you're writing something like a program for something non-critical it doesn't really matter.

Yes, maybe NXT handling money is a non-critical application.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: User705 on January 05, 2014, 04:12:20 PM

Quote from: FrictionlessCoin on January 05, 2014, 04:03:08 PM

Quote from: Come-from-Beyond on January 05, 2014, 03:58:29 PM

Quote from: tacotime on January 05, 2014, 03:53:29 PM

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

No, it's more like, if I see some kind of error, let me keep quiet about it!

Can you explain how that would effect the actual functionality of the system. Isn't it the same as someone submitting improperly signed transactions so those should be ignored? You keep bringing up issues with the code but without actual explanations of how those issues are supposed to screw things up.

It's in over 18 places in the code.

Anyway, it is bad practice because if an unexpected error happens (which it always does), you do want to know that something happened. To assume that you can ignore this because you think you know what you are doing is just bad practice.

Folks, I don't need to explain to you guys who to write code properly!

Jean-Luc

sr. member

Activity: 392

Merit: 250

Quote from: Jaguar0625 on January 05, 2014, 03:16:03 PM

Since setBalance can throw, where you do the following, balance and unconfirmed balance can get out of sync:

Code:

generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);

Code:

void setBalance(long balance) throws Exception {
    this.balance = balance;
    for (Peer peer : peers.values()) {
    if (peer.accountId == id && peer.adjustedWeight > 0) {
        peer.updateWeight();
    }
}

Granted, this is unlikely since you are suppressing all exceptions around the network calls. But it might be more robust to calculate balance from unconfirmedbalance or vice-versa.

There is no setBalance() in 0.5.0:

Code:

        synchronized long getBalance() {
            return balance;
        }

        synchronized long getUnconfirmedBalance() {
            return unconfirmedBalance;
        }

        void addToBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;

            }

            updatePeerWeights();

        }
        
        void addToUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.unconfirmedBalance += amount;

            }

            updateUserUnconfirmedBalance();

        }

        void addToBalanceAndUnconfirmedBalance(long amount) throws Exception {

            synchronized (this) {

                this.balance += amount;
                this.unconfirmedBalance += amount;

            }

            updatePeerWeights();
            updateUserUnconfirmedBalance();

        }

tacotime

legendary

Activity: 1484

Merit: 1005

Quote from: User705 on January 05, 2014, 04:12:20 PM

Quote from: FrictionlessCoin on January 05, 2014, 04:03:08 PM

Quote from: Come-from-Beyond on January 05, 2014, 03:58:29 PM

Quote from: tacotime on January 05, 2014, 03:53:29 PM

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

No, it's more like, if I see some kind of error, let me keep quiet about it!

Can you explain how that would effect the actual functionality of the system. Isn't it the same as someone submitting improperly signed transactions so those should be ignored? You keep bringing up issues with the code but without actual explanations of how those issues are supposed to screw things up.

It depends where you put it... it's dangerous in most places though because it doesn't class or report the exception and prevent you from seeing any other bugs that might arise while a function is executing. Basically: "Regardless of the error that arises, keep running and do not report it."

http://stackoverflow.com/questions/2416316/why-is-the-catchexception-almost-always-a-bad-idea

It's what I write when I'm in a rush and don't really care when I get some kind of error so long as the program does more or less what I want it to; ideally you would address it properly, but if you're writing something like a program for something non-critical it doesn't really matter.

lwilliams777

newbie

Activity: 10

Merit: 0

@Frictionless. I see that you know what you're talking about with regard to programming. In fact, you may very well be a brilliant programmer, but the problem is, no one cares. Why? Because you have taken it upon yourself as a right to criticize but have not contributed anything productive. You haven't earned that right yet. In order to earn the right to be heard you should contribute, even in small amount, not with criticism, but with real productive help. Once you do that then you've earned the right to criticize and others will actually listen to you. As it stands, people just think you are a jerk, and using Simon Crowell, a known jerk, as an example doesn't help your cause. Simon Crowell may be rich and be a star power broker, but I believe he has very few "real" deep long lasting friendships, if any.

You have not gone about this in an effective way. If you had played this right you would have contributed in a huge way and others would have sung your praises and others would have gladly signed up for your next generation plan. In fact they would have become your best sales people and you would have had more folks sign up to help with your development and vision. But as it stands there are only a few willing to help you and your chances of developing a great system are slim to none.

User705

legendary

Activity: 896

Merit: 1006

First 100% Liquid Stablecoin Backed by Gold

Quote from: FrictionlessCoin on January 05, 2014, 04:03:08 PM

Quote from: Come-from-Beyond on January 05, 2014, 03:58:29 PM

Quote from: tacotime on January 05, 2014, 03:53:29 PM

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

No, it's more like, if I see some kind of error, let me keep quiet about it!

Can you explain how that would effect the actual functionality of the system. Isn't it the same as someone submitting improperly signed transactions so those should be ignored? You keep bringing up issues with the code but without actual explanations of how those issues are supposed to screw things up.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Come-from-Beyond on January 05, 2014, 03:58:29 PM

Quote from: tacotime on January 05, 2014, 03:53:29 PM

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

No, it's more like, if I see some kind of error, let me keep quiet about it!

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: tacotime on January 05, 2014, 03:53:29 PM

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

Actually not. This is a paradigm heavily used in Erlang. If u tried to recover after an exception or even logged it, u would just consume resources without real profit.

tacotime

legendary

Activity: 1484

Merit: 1005

Quote from: FrictionlessCoin on January 05, 2014, 03:43:53 PM

My favorite snippet of code:

Code:

catch (Exception e) { }

More than 18 times in the code.

Reminds me of all these coin holders who have their head in the sand.

For the people who don't code, this is essentially, "if you see some kind of error, just ignore it."

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Come-from-Beyond on January 05, 2014, 03:43:29 PM

Quote from: Jaguar0625 on January 05, 2014, 03:28:52 PM

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

Since all you are really doing is setting a transaction number, you should be using AtomicInteger. This would eliminate the lock completely

But, in general, you should consider minimizing the scopes of your locks. For example, here, you're locking the entire transactions array to update a single transaction. It would be better to lock around that specific transaction so as not to block other pieces of code attempting to inspect / edit other transactions.

Also, there are a number of places where reader-writer locks are more appropriate than using synchronized (e.g. when dealing users, which should change relatively infrequently).

This extra locking can help scalability down the road.

Valid points, but there was a special rule set - code must be readable by a novice programmer. Concurrent API is for more experienced programmers. I used Concurrent API in my code a couple of times but I was trying to follow the rule.

Maybe you should rephrase that "code must be writable by a novice programmer who has never understood Java concurrent api."

So you're saying you made up a rule to hide your ignorance. "Thou shall not use concurrent API because I don't understand it"

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

My favorite snippet of code:

Code:

catch (Exception e) { }

More than 18 times in the code.

Reminds me of all these coin holders who have their head in the sand.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: Jaguar0625 on January 05, 2014, 03:28:52 PM

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

Since all you are really doing is setting a transaction number, you should be using AtomicInteger. This would eliminate the lock completely

But, in general, you should consider minimizing the scopes of your locks. For example, here, you're locking the entire transactions array to update a single transaction. It would be better to lock around that specific transaction so as not to block other pieces of code attempting to inspect / edit other transactions.

Also, there are a number of places where reader-writer locks are more appropriate than using synchronized (e.g. when dealing users, which should change relatively infrequently).

This extra locking can help scalability down the road.

Valid points, but there was a special rule set - code must be readable by a novice programmer. Concurrent API is for more experienced programmers. I used Concurrent API in my code a couple of times but I was trying to follow the rule.

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Jaguar0625 on January 05, 2014, 03:28:52 PM

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

Since all you are really doing is setting a transaction number, you should be using AtomicInteger. This would eliminate the lock completely

But, in general, you should consider minimizing the scopes of your locks. For example, here, you're locking the entire transactions array to update a single transaction. It would be better to lock around that specific transaction so as not to block other pieces of code attempting to inspect / edit other transactions.

Also, there are a number of places where reader-writer locks are more appropriate than using synchronized (e.g. when dealing users, which should change relatively infrequently).

This extra locking can help scalability down the road.

Yeah, let's just throw as many synchronized blocks all over the place because we really don't know what we're doing.

I would have been impressed if the code didn't use a single synchronized block. That would be an indication of a very well designed system.

Sigh.

Jaguar0625

sr. member

Activity: 299

Merit: 250

There does seem to be a lot of excessive locking. For example:

Code:

Transaction transaction = Transaction.getTransaction(buffer);
synchronized (Nxt.transactions) {						
    transaction.index = ++transactionCounter;						
}

Since all you are really doing is setting a transaction number, you should be using AtomicInteger. This would eliminate the lock completely

But, in general, you should consider minimizing the scopes of your locks. For example, here, you're locking the entire transactions array to update a single transaction. It would be better to lock around that specific transaction so as not to block other pieces of code attempting to inspect / edit other transactions.

Also, there are a number of places where reader-writer locks are more appropriate than using synchronized (e.g. when dealing users, which should change relatively infrequently).

This extra locking can help scalability down the road.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: Jaguar0625 on January 05, 2014, 03:16:03 PM

Since setBalance can throw, where you do the following, balance and unconfirmed balance can get out of sync:

Code:

generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);

Code:

void setBalance(long balance) throws Exception {
    this.balance = balance;
    for (Peer peer : peers.values()) {
    if (peer.accountId == id && peer.adjustedWeight > 0) {
        peer.updateWeight();
    }
}

Granted, this is unlikely since you are suppressing all exceptions around the network calls. But it might be more robust to calculate balance from unconfirmedbalance or vice-versa.

Jean-Luc was going to fix this issue AFAIK.

Jaguar0625

sr. member

Activity: 299

Merit: 250

Since setBalance can throw, where you do the following, balance and unconfirmed balance can get out of sync:

Code:

generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);

Code:

void setBalance(long balance) throws Exception {
    this.balance = balance;
    for (Peer peer : peers.values()) {
    if (peer.accountId == id && peer.adjustedWeight > 0) {
        peer.updateWeight();
    }
}

Granted, this is unlikely since you are suppressing all exceptions around the network calls. But it might be more robust to calculate balance from unconfirmedbalance or vice-versa.

jl777

legendary

Activity: 1176

Merit: 1134

Topic: Nxt source code analysis (QA) - page 4. (Read 14115 times)