Nxt source code analysis (QA) - page 10.

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: Come-from-Beyond on January 03, 2014, 08:39:58 AM

This proves u r not BCNext. Ur and his logic r completely different!

I am from generation "640k is enough"

So I dislike such memory copies.

bitcoinpaul

hero member

Activity: 910

Merit: 1000

You little nerds. Kiss

Jean-Luc

sr. member

Activity: 392

Merit: 250

Quote from: ImmortAlex on January 03, 2014, 08:37:00 AM

Okey, let me rewrite something

For memory usage I prefer to code like this:

Code:

                List selectedPeers = new ArrayList<>();
                for (Map.Entry entry : peers.entrySet()) {
                    Peer peer = entry.getValue();
                    if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() != 0 && ((!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold))) {
                        selectedPeers.add(peer);
                    }
                }

And even more, List selectedPeers can be reused, if used only in this method.

You are reading my mind. My tinfoil hat has been leaking!
My replacement of the same lines in getAnyPeer, already in 0.4.9e:

Code:

            List selectedPeers = new ArrayList();

            for (Peer peer : Nxt.peers.values()) {

                if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() > 0
                        && (!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold)) {

                    selectedPeers.add(peer);

                }

            }

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: Jean-Luc on January 03, 2014, 08:38:44 AM

Keep digging, I am still a step ahead.

Two step ahead (0.4.8 and 0.4.9), and I hope you start to do third step already

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: ImmortAlex on January 03, 2014, 08:37:00 AM

Okey, let me rewrite something

Code:

        static Peer getAnyPeer(int state, boolean applyPullThreshold) {

            synchronized (peers) {

                Collection peers = ((HashMap)Nxt.peers.clone()).values();
                Iterator iterator = peers.iterator();
                while (iterator.hasNext()) {

                    Peer peer = iterator.next();
                    if (peer.blacklistingTime > 0 || peer.state != state || peer.announcedAddress.length() == 0 || (applyPullThreshold && enableHallmarkProtection && peer.getWeight() < pullThreshold)) {

                        iterator.remove();

                    }

                }

                if (peers.size() > 0) {

                    Peer[] selectedPeers = peers.toArray(new Peer[0]);
...

For memory usage I prefer to code like this:

Code:

                List selectedPeers = new ArrayList<>();
                for (Map.Entry entry : peers.entrySet()) {
                    Peer peer = entry.getValue();
                    if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() != 0 && ((!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold))) {
                        selectedPeers.add(peer);
                    }
                }

And even more, List selectedPeers can be reused, if used only in this method.

This proves u r not BCNext. Ur and his logic r completely different!

(Or u did this intentionally

)

Jean-Luc

sr. member

Activity: 392

Merit: 250

Quote from: ImmortAlex on January 03, 2014, 08:14:05 AM

Small suggestion on usage of Collection.toArray().
I see a lot of calls like

Code:

peers.toArray(new Peer[0])

1. It is completely useless and equal to simple toArray() call.
2. It is better in terms of speed and memory usage to code like this:

Code:

peers.toArray(new Peer[peers.size()])

A search for toArray in 0.4.9e finds only two instances in old, commented out code. Keep digging, I am still a step ahead.

ImmortAlex

hero member

Activity: 784

Merit: 501

Quote from: Jean-Luc on January 03, 2014, 08:26:42 AM

Relax, I am also using IntelliJ. And the above has been fixed in 0.4.9e.

Quote from: Jean-Luc on January 03, 2014, 08:33:49 AM

I have fixed, I hope, the synchronization issues related to Blocks and Transactions in 0.4.9e. The synchronization in Accounts is on my TODO list for the next version.

Good news!
Okey, let me find something really interesting...

ImmortAlex

hero member

Activity: 784

Merit: 501

Okey, let me rewrite something

Code:

        static Peer getAnyPeer(int state, boolean applyPullThreshold) {

            synchronized (peers) {

                Collection peers = ((HashMap)Nxt.peers.clone()).values();
                Iterator iterator = peers.iterator();
                while (iterator.hasNext()) {

                    Peer peer = iterator.next();
                    if (peer.blacklistingTime > 0 || peer.state != state || peer.announcedAddress.length() == 0 || (applyPullThreshold && enableHallmarkProtection && peer.getWeight() < pullThreshold)) {

                        iterator.remove();

                    }

                }

                if (peers.size() > 0) {

                    Peer[] selectedPeers = peers.toArray(new Peer[0]);
...

For memory usage I prefer to code like this:

Code:

                List selectedPeers = new ArrayList<>();
                for (Map.Entry entry : peers.entrySet()) {
                    Peer peer = entry.getValue();
                    if (peer.blacklistingTime <= 0 && peer.state == state && peer.announcedAddress.length() != 0 && ((!applyPullThreshold || !enableHallmarkProtection || peer.getWeight() >= pullThreshold))) {
                        selectedPeers.add(peer);
                    }
                }

And even more, List selectedPeers can be reused, if used only in this method.

Jean-Luc

sr. member

Activity: 392

Merit: 250

Quote from: ImmortAlex on January 03, 2014, 08:01:42 AM

Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.

In 0.4.9e, they are. :-)

Quote

When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

I have fixed, I hope, the synchronization issues related to Blocks and Transactions in 0.4.9e. The synchronization in Accounts is on my TODO list for the next version.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Quote from: ImmortAlex on January 03, 2014, 08:22:21 AM

One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.

Hey, who is checking the code, u or robot? Grin

Jean-Luc

sr. member

Activity: 392

Merit: 250

Quote from: ImmortAlex on January 03, 2014, 08:22:21 AM

One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.

Relax, I am also using IntelliJ. And the above has been fixed in 0.4.9e.

ImmortAlex

hero member

Activity: 784

Merit: 501

One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.

ImmortAlex

hero member

Activity: 784

Merit: 501

Small suggestion on usage of Collection.toArray().
I see a lot of calls like

Code:

peers.toArray(new Peer[0])

1. It is completely useless and equal to simple toArray() call.
2. It is better in terms of speed and memory usage to code like this:

Code:

peers.toArray(new Peer[peers.size()])

Come-from-Beyond

legendary

Activity: 2142

Merit: 1010

Newbie

Watching

ImmortAlex

hero member

Activity: 784

Merit: 501

While lurking for injected bugs, I found that some parts of Nxt code makes my eyes bleed. With hope that I have enough programming skills (probably better, than my runglish

), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual

).

First of all, thanks to IntelliJ IDEA code analysis, I see a lot of syncronized with potentially wrong usage.
Static variables like peers, blocks, accounts, users are not final. They are initialized at start, so it is not a big flaw right now, but who knows about future.
When I look more deeply in syncronizations, I see a lot of places, where author get content of such global collection and then sync on it, but not sync on collection itself. Example:

Code:

Block.analyse() method:

Account generatorAccount = accounts.get(Account.getId(generatorPublicKey));
synchronized (generatorAccount) {
    generatorAccount.setBalance(generatorAccount.balance + totalFee * 100L);
    generatorAccount.setUnconfirmedBalance(generatorAccount.unconfirmedBalance + totalFee * 100L);
}

While in some places such usage may be safe, I think it's very bad practice anyway.

More to come...

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