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

Add performance documentation for WeightedIndex #594

Merged
merged 1 commit into from
Sep 3, 2018

Conversation

sicking
Copy link
Contributor

@sicking sicking commented Aug 24, 2018

Also includes a small documentation improvement for Uniform

@sicking
Copy link
Contributor Author

sicking commented Aug 24, 2018

Fixes the WeightedIndex-specific parts of #116

@sicking
Copy link
Contributor Author

sicking commented Aug 24, 2018

I had to revert the changes to Uniform in order to pass tests. The problem is that we create two struct.Uniform.html files, in different directories, so getting both to link to external files correctly is tricky. And while only one of the two files is linked is used (I think), the tests will complain about broken links in the other one.

This is probably fixable somehow, but since this changed was tangential to this PR anyway, I'll leave it for a separate PR.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Thanks.

What was the missing changes?

/// type of those weights. However, since `Vec` doesn't guarantee a particular
/// growth strategy, additional memory might be allocated but not used.
/// The `WeightedIndex` object also contains a [`Uniform<X>`] object, though
/// for primitive types, this doesn't allocate any memory.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure which part you're saying is is incorrect. Uniform<X> certainly has size, but it doesn't allocate memory (from the heap).

I'm also not counting the "inline" part of Vec<X> as allocation, which is why those bytes are not mentioned. Nor things like alingment bytes.

Generally the easiest way to account for the "inline" parts of objects is to use mem::size_of<WeightedIndex<X>>(). That'll account for all those bits, but won't account for memory allocations, which is why I focused more on talking about memory allocations.

Do you have any suggestions for wording to make that more clear?

Copy link
Member

Choose a reason for hiding this comment

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

True; I skim read. However #116 asks for documentation on memory usage, not allocations, and as far as I'm aware this is what is usually more interesting (though one can separate heap and stack usage).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess both are useful in different situations. I pushed an updated version, let me know if it matches what you had in mind.

@sicking
Copy link
Contributor Author

sicking commented Aug 24, 2018

What was the missing changes?

Not sure what you mean here?

@dhardy
Copy link
Member

dhardy commented Aug 24, 2018

You mentioned you had to revert some changes but those aren't in the commit, so I wondered what they are?

@sicking
Copy link
Contributor Author

sicking commented Aug 25, 2018

You mentioned you had to revert some changes but those aren't in the commit, so I wondered what they are?

Ah. I wanted to add the "See the [module documentation] on how to implement [Uniform] range sampling for a custom type." text, which we currently have on the traits, also to the documentation of the Uniform struct.

@dhardy dhardy merged commit 4163daa into rust-random:master Sep 3, 2018
@sicking sicking deleted the weighteddoc branch September 4, 2018 06:16
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.

2 participants