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

Remove the heapsize dependency. #115

Merged
merged 6 commits into from
Jan 8, 2019
Merged

Remove the heapsize dependency. #115

merged 6 commits into from
Jan 8, 2019

Conversation

zhzy0077
Copy link
Contributor

@zhzy0077 zhzy0077 commented Jan 7, 2019

I have removed the heapsize crate in order to build on Max OS X. Moving heap_size_of_children into individual structs' impl blocks.
Also, I have update the rust-toolchain to 2018-12-31 while rls on Mac is missing in nightly-2018-12-30 version.

https://mexus.github.io/rustup-components-history/x86_64-apple-darwin.html

Copy link
Owner

@cswinter cswinter left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for doing this! Just needs one small change before merging.

@@ -77,5 +78,9 @@ impl Buffer {
pub fn len(&self) -> usize {
self.length
}

pub fn heap_size_of_children(&self) -> usize {
mem::size_of::<Buffer>()
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't quite right because it will not account for any contents of the buffer HashMap, just the pointer.
I'm fine with just ignoring Buffer completely though (and maybe adding a comment to that extent in Table::heap_size_of_children) since it's not really used at the moment.

@zhzy0077
Copy link
Contributor Author

zhzy0077 commented Jan 8, 2019

@cswinter I have update the heap_size_of_children of Buffer by calculating the hashmap's size.

src/mem_store/raw_col.rs Outdated Show resolved Hide resolved
@zhzy0077
Copy link
Contributor Author

zhzy0077 commented Jan 8, 2019

@cswinter Missed this point. Updated.

src/ingest/raw_val.rs Outdated Show resolved Hide resolved
@zhzy0077
Copy link
Contributor Author

zhzy0077 commented Jan 8, 2019

@cswinter I think mem::size_of::<i64>() is required and another size_of:: is added to MixedCol struct.

BTW: The commit tree is a mess. Please squash merge to eliminate such commits.

Copy link
Owner

@cswinter cswinter left a comment

Choose a reason for hiding this comment

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

Squashing sounds good, just need one more small fix!

@@ -23,7 +23,7 @@ impl RawVal {
match *self {
RawVal::Int(_) => mem::size_of::<i64>(),
Copy link
Owner

Choose a reason for hiding this comment

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

The i64 would be part of the RawVal, since it does not cause a heap allocation this should just return 0.

@zhzy0077
Copy link
Contributor Author

zhzy0077 commented Jan 8, 2019

@cswinter Sorry for that. Previously I am a Java developer. It's not so intuitive for me to distinguish whether a object is on heap or stack.

@cswinter
Copy link
Owner

cswinter commented Jan 8, 2019

No worries, there's always more to learn 😄
Diff looks great now, will merge once travis completes 🎉

@cswinter cswinter merged commit cd25ab5 into cswinter:master Jan 8, 2019
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.

None yet

2 participants