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 HeapSizeOf trait and from_slice method #42

Merged
merged 4 commits into from Mar 16, 2017
Merged

Conversation

@nipunn1313
Copy link
Contributor

nipunn1313 commented Mar 14, 2017

Also added some tests for the new methods. The rationale

  • from_slice is an ergonomic way to convert a Copy-able slice to a SmallVec (instead of using the From<'a slice> which uses an iterator)
  • HeapSizeOf is handy for measuring heap allocations. Especially useful for monitoring something like this. Seems to be part of the servo project anyway.
  • Added size 36 to smallvec sizes (was useful to us)

This change is Reviewable

@nipunn1313 nipunn1313 force-pushed the nipunn1313:heapsize branch from 9ae3d69 to 3483d3b Mar 14, 2017
Because it seems like good practice.
@@ -478,6 +482,12 @@ impl<A: Array> SmallVec<A> {
}

impl<A: Array> SmallVec<A> where A::Item: Copy {
pub fn from_slice(slice: &[A::Item]) -> Self {

This comment has been minimized.

@rphmeier

rphmeier Mar 14, 2017

Contributor

Could it just be impl<A: Array, T: AsRef<[A::Item]>> From<T> for SmallVec<A>?

This comment has been minimized.

@nipunn1313

nipunn1313 Mar 15, 2017

Author Contributor

Also need to make sure A::Item: Copy which is what made me make it more specific (from_slice() rather than impl From).

I'd imagine that a generic From implementation should work regardless of whether it's Copy or not. Without specialization, it's hard to make an efficient version for the Copy type, so I went with a different "from_slice" function.

@tomaka
Copy link
Contributor

tomaka commented Mar 15, 2017

For the record heapsize is a public external dependency and may cause a dependency hell later.

@rphmeier
Copy link
Contributor

rphmeier commented Mar 15, 2017

@tomaka That can be addressed by putting heapsize stuff under a feature flag

@jdm
Copy link
Member

jdm commented Mar 16, 2017

Let's have an optional feature flag for heapsize, as mentioned in previous comments. Otherwise these changes look unobjectionable to me.

lib.rs Outdated
@@ -6,6 +6,9 @@
//! to the heap for larger allocations. This can be a useful optimization for improving cache
//! locality and reducing allocator traffic for workloads that fit within the inline buffer.

#![feature(specialization)]

This comment has been minimized.

@jdm

jdm Mar 16, 2017

Member

This looks like a mistake?

This comment has been minimized.

@nipunn1313

nipunn1313 Mar 16, 2017

Author Contributor

Good catch. I was playing around with specialization and accidentally rolled it into this. Fixed!

@jdm
Copy link
Member

jdm commented Mar 16, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2017

📌 Commit 010df90 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2017

Testing commit 010df90 with merge d389f3a...

bors-servo added a commit that referenced this pull request Mar 16, 2017
Add HeapSizeOf trait and from_slice method

Also added some tests for the new methods. The rationale
- from_slice is an ergonomic way to convert a Copy-able slice to a SmallVec (instead of using the From<'a slice> which uses an iterator)
- HeapSizeOf is handy for measuring heap allocations. Especially useful for monitoring something like this. Seems to be part of the servo project anyway.
- Added size 36 to smallvec sizes (was useful to us)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/42)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2017

☀️ Test successful - status-travis
Approved by: jdm
Pushing d389f3a to master...

@bors-servo bors-servo merged commit 010df90 into servo:master Mar 16, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nipunn1313
Copy link
Contributor Author

nipunn1313 commented Mar 16, 2017

Thanks yo!

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.