Pages:
Author

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

hero member
Activity: 784
Merit: 501
January 03, 2014, 08:44:49 AM
#15
This proves u r not BCNext. Ur and his logic r completely different!
I am from generation "640k is enough" Smiley So I dislike such memory copies.
hero member
Activity: 910
Merit: 1000
January 03, 2014, 08:44:27 AM
#14
You little nerds.  Kiss
sr. member
Activity: 392
Merit: 250
January 03, 2014, 08:43:43 AM
#13
Okey, let me rewrite something Smiley

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);

                }

            }

hero member
Activity: 784
Merit: 501
January 03, 2014, 08:42:34 AM
#12
Keep digging, I am still a step ahead. Smiley
Two step ahead (0.4.8 and 0.4.9), and I hope you start to do third step already Smiley
legendary
Activity: 2142
Merit: 1010
Newbie
January 03, 2014, 08:39:58 AM
#11
Okey, let me rewrite something Smiley

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 Smiley)
sr. member
Activity: 392
Merit: 250
January 03, 2014, 08:38:44 AM
#10
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. Smiley
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:38:15 AM
#9
Relax, I am also using IntelliJ. And the above has been fixed in 0.4.9e.

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...
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:37:00 AM
#8
Okey, let me rewrite something Smiley

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.
sr. member
Activity: 392
Merit: 250
January 03, 2014, 08:33:49 AM
#7
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.
legendary
Activity: 2142
Merit: 1010
Newbie
January 03, 2014, 08:26:54 AM
#6
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
sr. member
Activity: 392
Merit: 250
January 03, 2014, 08:26:42 AM
#5
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.
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:22:21 AM
#4
One more thanks to IDEA: line 6533, peer can be null. At least there's check for null above and below, but not here.
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:14:05 AM
#3
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()])
legendary
Activity: 2142
Merit: 1010
Newbie
January 03, 2014, 08:04:48 AM
#2
Watching
hero member
Activity: 784
Merit: 501
January 03, 2014, 08:01:42 AM
#1
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 Smiley ), I'm starting this thread. Not for bounty, but for better quality (but donations welcome, as usual Smiley ).

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...
Pages:
Jump to: