Skip to content

Optimize Piece-s memory usage #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gzsombor wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Optimize Piece-s memory usage #35

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4017a57
Optimization: avoid creating too much buffers, use the inherited torr…
gzsombor Feb 24, 2013
1d4db41
pass SharedTorrent to the individual Piece objects, so a lot's of unn…
gzsombor Feb 24, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions src/main/java/com/turn/ttorrent/client/Piece.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class Piece implements Comparable<Piece> {
private final int index;
private final long offset;
private final long length;
private final byte[] hash;
private final ByteBuffer torrentHash;
private final boolean seeder;

private volatile boolean valid;
Expand All @@ -70,17 +70,17 @@ public class Piece implements Comparable<Piece> {
* @param index This piece index in the torrent.
* @param offset This piece offset, in bytes, in the storage.
* @param length This piece length, in bytes.
* @param hash This piece 20-byte SHA1 hash sum.
* @param torrentHash This full hash for every pieces.
* @param seeder Whether we're seeding this torrent or not (disables piece
* validation).
*/
public Piece(TorrentByteStorage bucket, int index, long offset,
long length, byte[] hash, boolean seeder) {
long length, ByteBuffer torrentHash, boolean seeder) {
this.bucket = bucket;
this.index = index;
this.offset = offset;
this.length = length;
this.hash = hash;
this.torrentHash = torrentHash;
this.seeder = seeder;

// Piece is considered invalid until first check.
Expand Down Expand Up @@ -160,18 +160,27 @@ public synchronized boolean validate() throws IOException {
this.valid = false;

try {
// TODO: remove cast to int when large ByteBuffer support is
// implemented in Java.
ByteBuffer buffer = this._read(0, this.length);
byte[] data = new byte[(int)this.length];
buffer.get(data);
this.valid = Arrays.equals(Torrent.hash(data), this.hash);
this.valid = checkHash(buffer);
} catch (NoSuchAlgorithmException nsae) {
logger.error("{}", nsae);
}

return this.isValid();
}

protected boolean checkHash(ByteBuffer data) throws NoSuchAlgorithmException {
byte[] calculatedHash = Torrent.hash(data);

int torrentHashPosition = getIndex() * Torrent.PIECE_HASH_SIZE;
for (int i = 0; i < Torrent.PIECE_HASH_SIZE; i++) {
byte value = this.torrentHash.get(torrentHashPosition + i);

@zanella zanella Nov 14, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fetching byte per byte on a direct buffer is faster than just fetching the whole thing once ?

if (value != calculatedHash[i]) {
return false;
}
}
return true;
}

/**
* Internal piece data read function.
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/com/turn/ttorrent/client/SharedTorrent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,6 @@ public synchronized void init() throws InterruptedException, IOException {
logger.info("Analyzing local data for {} with {} threads ({} pieces)...",
new Object[] { this.getName(), threads, nPieces });
for (int idx=0; idx<nPieces; idx++) {
byte[] hash = new byte[Torrent.PIECE_HASH_SIZE];
this.piecesHashes.get(hash);

// The last piece may be shorter than the torrent's global piece
// length. Let's make sure we get the right piece length in any
Expand All @@ -332,7 +330,7 @@ public synchronized void init() throws InterruptedException, IOException {
this.bucket.size() - off,
this.pieceLength);

this.pieces[idx] = new Piece(this.bucket, idx, off, len, hash,
this.pieces[idx] = new Piece(this.bucket, idx, off, len, piecesHashes,
this.isSeeder());

Callable<Piece> hasher = new Piece.CallableHasher(this.pieces[idx]);
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/turn/ttorrent/common/Torrent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,12 @@ public static byte[] hash(byte[] data) throws NoSuchAlgorithmException {
return md.digest();
}

public static byte[] hash(ByteBuffer data) throws NoSuchAlgorithmException {
MessageDigest md = MessageDigest.getInstance("SHA-1");
md.update(data);

@zanella zanella Nov 14, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create the byte array internally

@gzsombor gzsombor Nov 14, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if it's a ByteBuffer which doesn't backed by an accessible byte array, and in that case, only create a 4K buffer, which is more memory friendly solution, than to create a byte buffer at least 64K, to accommodate a torrent block just for hashing. I could run the torrent client more efficiently this way, YMMV.

@zanella zanella Nov 15, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buffer being passed is the data read from the "output" file (this._read(...)), which is a direct buffer, i.e. is not array backed, no ? The piecesHashes one is array backed.

I couldn't understand the rest of your answer, sorry. Upon reading the source of the MessageDigest I can see this optimization is there, reusing the byte array internally tempArray, but here md is created and discarded just as well, I think the solution lies more in the lines of #184, of having a "resource store" where you pick it up, use it and relinquish ownership...

@gzsombor gzsombor Nov 15, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look into here : https://github.com/mpetazzoni/ttorrent/pull/35/files#diff-454b627720e29935f25dab9a660670b1L166 you can see a big memory allocation. I don't know how big is it, if it's 64K or 512K - I've implemented it 3.5 years ago - but I'm fairly certain that one torrent piece can be pretty big, and it is much simpler to pass the ByteBuffer directly to the MessageDigest class, instead of allocating a new byte array, copy everything into it, calculate the hash.

Sorry being not too clear, English is not my native language :(

@zanella zanella Nov 16, 2016 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I think I can sum up the bytes duplication:

  • each byte piece is duplicated from the piecesHashes -> this one you got rid of by hitting the piecesHashes using the offset;
  • each file piece is read into a ByteBuffer then copied into a byte array -> this one you got rid of copying, by passing the ByteBuffer (which has a byte array internally) to the MessageDigest mehod;

I think that what remains is limiting the number of existing ByteBuffer buffer from file reading, like I tried in #195 , I'll try to limit it then profile it, with this done I think this patch is complete.

@gzsombor gzsombor Nov 18, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that happened :)

return md.digest();
}

/**
* Convert a byte string to a string containing an hexadecimal
* representation of the original data.
Expand Down