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

rename pool back to compact_mem and implement here #235

Merged
merged 2 commits into from
Apr 10, 2019
Merged

Conversation

piever
Copy link
Collaborator

@piever piever commented Apr 9, 2019

In a discussion on making DataFrames more lightweight (ref: JuliaData/DataFrames.jl#1764), it was pointed out that StructArrays also got a bit of bloat (which feels unnecessary for people using it for things other than data analysis). I have a plan to get rid of the PooledArrays dependency and Requires use for WeakRefStrings, but it requires moving the "pooling" functionality back here, so I've renamed pool (that we took from StructArrays) to the old name of compact_mem (pool is wrong anyway as we are only pooling some columns) and moved the implementation here (in the future I will deprecate the implementation in StructArrays when I get rid of the dependencies).

@piever piever merged commit 97adceb into master Apr 10, 2019
@piever piever deleted the pv/decouple branch April 10, 2019 13:05
@KristofferC
Copy link

KristofferC commented Apr 10, 2019

What's the point of Requires for stuff like WeakRefStrings? It loads extremely fast anyway, I would bet the overhead from Requires is larger than just loading it unconditionally.

@piever
Copy link
Collaborator Author

piever commented Apr 10, 2019

I agree. I would like to remove the dependency completely by adding a 2 line interface package - or two lines to Base julia - that encompasses all these packages (PooledArrays, CategoricalArrays, WeakRefStrings) so that row comparison can be made efficient for custom array storage X without depending on package X (see JuliaLang/julia#31606 for details). In the meantime I agree with you that I may be better off depending on WeakRefStrings then putting it in Requires.

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