-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Update page size to 64k #50600
base: master
Are you sure you want to change the base?
Update page size to 64k #50600
Conversation
Are the max heap measurements measuring pages or bytes? It would be good to see if there is a substantial heap size impact. |
Is it possible to run these benchmarks on an old machine? |
This change shouldn't increase max heap by that much, you just lose a bit of granularity on the heap size (64k vs 16k). We have 48 size classes currently and that amounts to 3MB in total, which isn't that big anyway. |
Theoretically that's true, but the 64k pages increases the potential risks of fragmentation. |
|
I don't see how that's true? Isn't it always possible to have a worst case of many objects with only one live object per page, in which case bigger pages means more fragmentation? I still think this change is probably worth doing, just trying to understand this statement. |
I guess the way I phrased it wasn't the best. In a pathological case where you manage to leave one object alive per page this makes it potentially 4x worse. It does also make it harder for objects to span that many more pages because the pages are now bigger, and fortunately block sized heaps like ours tend to limit how much fragmentation you get simply because you don't get into a situation where the objects don't fit in the heap anymore. |
The impact is surprisingly large. It looks like the mange change is in sweep? Could you profile to determine whether the change is that we do 4x less madvise? |
It would also be interesting to see how it behaves with concurrent page sweeping enabled. |
I haven't done extensive profiling, it's just that the initial results were promising enough to open a PR. |
bump. should we merge this? |
Just want to note that, with a 16k page size, we're already seeing substantial amounts of memory lost to fragmentation in some of our workloads. This PR has the possibility of making things worse on that end. |
16k is a small page size (64k seems to be more usual).
master
PR
Should we change the size classes/include larger classes? Also this makes a bigger difference than what I had imagined.