Skip to content
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.

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

Automatically calculate piece length #55

Merged
merged 7 commits into from
Oct 23, 2020
Merged

Automatically calculate piece length #55

merged 7 commits into from
Oct 23, 2020

Conversation

codeclem
Copy link
Contributor

Ported from https://github.com/cmclark65/mktorrent

This would close issue #30

Thanks to @cmclark65 for the code and permission to port it!

@cmclark65
Copy link

7 years ago I was happy with 24 bit as the largest piece size for a torrent target larger than 12.5 GiB. I don't know what size the largest common torrents are today, but maybe it would make sense to add a few more "maxes" to the array? Is 24 enough to keep the torrent size down for today's largest files/file collections being torrented? It is nearly trivial to add a few more sizes.

Obviously 24 is still a lot better than the old default of 18 for very large targets, and possibly good enough.

@cmclark65
Copy link

Arguably it would be good to just get rid of the skanky array method I used, specify a maximum piece size such as 28 for the for loop, and specify a constant for the max number of pieces (2000 if the issue #30 is to be trusted; I seem to have targeted a max of more like 800) and then just have the for loop be "n == 1 << i; if ((m->size + n - 1) / n) <= PIECE_MAX m->piece_length = n;", the if following the loop would need to change i to 1 << i, ditch the later conversion from bit size to integer since it is already now done in the loop.

Does this make sense? I would do it this way now; they way i did it 7 years ago with the pre-defined maxes and the array kind of sucks.

@codeclem
Copy link
Contributor Author

codeclem commented Oct 13, 2020

Common recommendations for piece size vary some. A 24 bit length would be a piece size of 16 MiB. A lot of people say you should never even go over 1 MiB, which I think is kind of unreasonably strict, and I have never heard anyone recommend a size larger than 16 MiB. Supposedly some clients don't handle 32 MiB+ pieces well, or even at all.

Looking through my currently seeded torrents, the largest torrent I have with a 1 MiB piece size is 49 GiB. I have many torrents in the 300-800 GiB range that all use 16 MiB pieces, and there are two torrents that are each 1.5 TiB and they use 64MiB pieces. I think that's the only time I have encountered pieces that large (qBitorrent doesn't even offer it as an option when creating torrents) and I can't find any torrents right now that use 32 MiB.

All that to say, I think 16 MiB is a very reasonable max default. This commit does not remove the ability for the user to specify their own piece length, and I think anyone who feels they need 32 or 64 MiB pieces would totally understand having to explictly say so.

As far as the implementation, I think it's fine as it is but I would be happy to refactor it if @Rudde has an opinion. For now I would wait to see if he's even interested in merging the feature at all.

@FranciscoPombal
Copy link

Piece sizes over 16 MiB can make sense for some very large torrents. Don't listen to the brain dead zealots claiming 16 MiB should be the max, that's just because their precious uT 2.2.1 doesn't support piece sizes over that.

@cmclark65
Copy link

cmclark65 commented Oct 14, 2020

If a 24 bit piece size is the largest that a commonly used client supports, it seems like a good max for the auto-size code. Then this port of my code should be good to go; it is proven at least. I tested it well on 32 and 64 bit linux and freebsd at the time I first wrote it.

My suggestion for rewriting using a formula would require re-testing; I'd recommend doing it if you wanted to raise the max piece size from 2^24 but if not you probably have better things to do with your time. I hope the current maintainer buys these changes if only so that someone other than me gets some benefit out of my work. :)

init.c Show resolved Hide resolved
init.c Outdated
Comment on lines 414 to 421
uintmax_t piece_len_maxes[] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
(uintmax_t) BIT15MAX * ONEMEG, (uintmax_t) BIT16MAX * ONEMEG,
(uintmax_t) BIT17MAX * ONEMEG, (uintmax_t) BIT18MAX * ONEMEG,
(uintmax_t) BIT19MAX * ONEMEG, (uintmax_t) BIT20MAX * ONEMEG,
(uintmax_t) BIT21MAX * ONEMEG, (uintmax_t) BIT22MAX * ONEMEG,
(uintmax_t) BIT23MAX * ONEMEG
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be const in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this and num_piece_len_maxes to const

init.c Outdated
Comment on lines 423 to 424
int num_piece_len_maxes = sizeof(piece_len_maxes) /
sizeof(piece_len_maxes[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

init.c Outdated
Comment on lines 577 to 578
if (m->piece_length == 0)
m->piece_length = i;
Copy link
Contributor

Choose a reason for hiding this comment

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

i = num_piece_len_maxes in this case, so I think it'd be better to write m->piece_length = num_piece_len_maxes.

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, that is more clear. I've pushed a new commit with this change.

init.c Outdated
Comment on lines 569 to 570
/* determine the piece length based on the torrent size if
it was not user specified. */
Copy link
Owner

Choose a reason for hiding this comment

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

Please fix the indentation here. Use tabs.

init.c Outdated
Comment on lines 580 to 583
/* if user did specify a piece length, verify its validity */
FATAL_IF0(m->piece_length < 15 || m->piece_length > 28,
"the piece length must be a number between 15 and 28.\n");
}
Copy link
Owner

Choose a reason for hiding this comment

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

Please fix the indentation here as well.

mktorrent.h Outdated
Comment on lines 13 to 25
#define ONEMEG 1048576

/* max torrent size in MB for a given piece length in bits */
/* where an X bit piece length equals a 2^X byte piece size */
#define BIT23MAX 12800
#define BIT22MAX 6400
#define BIT21MAX 3200
#define BIT20MAX 1600
#define BIT19MAX 800
#define BIT18MAX 400
#define BIT17MAX 200
#define BIT16MAX 100
#define BIT15MAX 50
Copy link
Owner

Choose a reason for hiding this comment

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

Please replace the tabs with single spaces.

@pobrn pobrn merged commit ea1fbf2 into pobrn:master Oct 23, 2020
@pobrn
Copy link
Owner

pobrn commented Oct 23, 2020

Thanks.

@codeclem
Copy link
Contributor Author

Thank you!

pobrn added a commit that referenced this pull request Jan 30, 2021
* Add exclude command line option. Excludes files based on a glob string (#56)
* Automatically calculate piece length (#55)
@ghost
Copy link

ghost commented Feb 28, 2022

Is there a new release immediately planned with the merged pull requests?

@edgeworn
Copy link

The last release was over 5 years ago. Will there be a new release soon with this and related changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants