Conversation
|
LGTM! Just one question: why can't we move the |
The point is that locking is not done every time @nalimilan - I will wait for your approval before merging. |
|
Thanks @bkamins . You are right. I did not realize that those broadcasts actually are calling the function many, many times. |
|
Instead of taking the lock everywhere, wouldn't it be almost as fast (and simpler) to use a vectorized |
This reverts commit 6685c73.
|
@nalimilan I have removed global state as you requested + improved inference a bit (so we have a shorter time to first table even in simple cases although we have a more complex logic; because of this I resolve your comments as they do not apply): Test 1Test 2Test 3Test 4Test 5 |
|
@nalimilan - I have additionally moved |
src/abstractdataframe/show.jl
Outdated
| end | ||
|
|
||
| """Return compact string representation of type T""" | ||
| function batch_compacttype(types::Vector{Any}, maxwidths::AbstractVector{Int}, |
There was a problem hiding this comment.
Choose between Vector and AbstractVector? :-) Same below.
There was a problem hiding this comment.
I opted to use Vector everywhere to signal we want to avoid specialization for different types.
| end | ||
| end | ||
|
|
||
| """Return compact string representation of type T""" |
There was a problem hiding this comment.
Add a comment to explain what's the point of having this function?
There was a problem hiding this comment.
ah - added. I was looking at another view on GitHub and thought you want me to expand the docstring of compacteltype (which I did)
|
I have also removed |
|
Do you know if the coverage misses are real? That's not the fault of this PR, but could be worth checking. |
|
Thank you! |
Fixes #2739
This change should make us faster in most cases (even narrow tables should be improved). I have not done much performance testing so some additional checks are welcome.
The one benchmark is:
This PR:
and DataFrames.jl 1.1: