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

Improves Growth Factor #6523

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chris-huxtable
Copy link
Contributor

@chris-huxtable chris-huxtable commented Aug 11, 2018

Based on finding the growth coefficient of IO::Memory was 2.0 and the information referenced in this article.

src/io/memory.cr Outdated Show resolved Hide resolved
src/io/memory.cr Outdated Show resolved Hide resolved
src/io/memory.cr Outdated Show resolved Hide resolved
@Sija
Copy link
Contributor

Sija commented Aug 11, 2018

Btw, similar optimization could be also applied to Array.

@chris-huxtable
Copy link
Contributor Author

Yep :)

I want to sort it out here first then apply it anywhere else it could be useful.

src/io/memory.cr Show resolved Hide resolved
@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Aug 11, 2018

I am having 2 of my machines do every value 1.1 to 2.0 at steps of 0.01 just on the off chance there might be something better. The results I will put in Gists.

I will update them with the full results in the morning.

Edit: Based on the current results I think my results may be an artifact of my testing procedure. I will dig deeper.

Edit: My testing method was flawed. Please ignore it.

@ysbaddaden
Copy link
Contributor

Don't run benchmarks but analyze bdwgc to understand how it does allocate memory, and more specifically how much more memory does it allocate than required.

Problem is, once you're tied to a GC memory model, it will fall flat with another memory allocator. For example with my Immix GC the allocation is always rounded to a word size, that is 4 or 8 bytes depending on 32 or 64-bit arch. You may be lucky and have enough room after the current allocation, but it's random chance.

@chris-huxtable
Copy link
Contributor Author

I intend to go deeper when I have time. If it’s just a constant I don’t think it would be to difficult to set it based on the GC’s memory model.

@ysbaddaden
Copy link
Contributor

In fact, with Immix GC I always reallocate and will always (unless the size didn't change), because the current thread allocator probably doesn't own the memory block anymore.

So, to me, the growth factor is mostly about growing enough vs wasting space in common collection use cases.

@chris-huxtable
Copy link
Contributor Author

Looking further into this issue its largely about wasting space. That being said The growth coefficient should still be less then 2.0 so that reuse can potentially happen. I know @ysbaddaden has said that IMMIX doesn't reuse its memory but as far as I have been able to ascertain boehm does as do many others. If we switch from boehm I think this should be re-evaluated.

There is an interesting discussion on growth rates on stack overflow which also references the FBVector like I referenced above .

Until we switch from boehm I like the Idea of using Phi (φ) 1.618033987 as its less then 2.0 allowing reallocation, but will have an intermediate rate of growth keeping reallocations low. Also because its the golden ratio.

@jkthorne
Copy link
Contributor

@chris-huxtable are you still working on this I am interested if this can still be a performance improvement.

@chris-huxtable
Copy link
Contributor Author

I am not currently. But I could take another shot at it if there is interest.

@jkthorne
Copy link
Contributor

I mean I am interested in it. Maybe someone from the core team could weight in. I think some preliminary benchmarks might be compelling.

@chris-huxtable
Copy link
Contributor Author

It's kind of a difficult thing to test. If you don't inter-splice a number of mallocs as you grow the array when testing and manage to do so consistently you end up with no difference (realloc involves no copying) or inconstant results (realloc inconsistently copies.

@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Nov 12, 2019

Beyond performance improvements I think it makes sense to go to a more conservative growth factor just for memory efficiency.

@ysbaddaden, @straight-shoota, anyone else relevant - Would a PR which sets a more conservative growth factor of Phi (φ) 1.618033987 be accepted in principle, assuming it works and meets any other standards of quality?

Note: I like 1.618 as its an approximation of Phi (I realize this is aesthetic and a form of naturalistic fallacy). Apple uses 1.625 in NSMutableArray. Others have noted their use of 1.6. Really the factor should be able to be tuned (any PR I submit would use an easily adjusted constant).

@straight-shoota
Copy link
Member

Sure, performance optimizations are always welcome.

It's just optimizations can easily be added later, and I think at the moment we'd like to put a stronger focus on bug fixes and features. Feel free to go ahead, though.

@chris-huxtable
Copy link
Contributor Author

chris-huxtable commented Nov 12, 2019

@straight-shoota - Sounds good. Just don't want to waste my time and end up in a 9 mouth back and forth about it. Best to know if its acceptable before hand.

@rdp
Copy link
Contributor

rdp commented Dec 24, 2019

Are there any benchmarks showing performance improvement?

@chris-huxtable
Copy link
Contributor Author

It's hard to contrive a comparison. It's really more about efficiency.

@caspiano
Copy link
Contributor

I believe this can be closed in favour of #11482

@straight-shoota
Copy link
Member

They're definitely related. But this one is about IO::Memory, while #11482 only affected Array.

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

Successfully merging this pull request may close these issues.

None yet

7 participants