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

Memory manager prototype #541

Closed
wants to merge 1 commit into from
Closed

Conversation

richardstartin
Copy link
Member

No description provided.

@richardstartin richardstartin force-pushed the memory-manager-prototype branch 3 times, most recently from 26aa44c to 1871685 Compare November 25, 2021 22:57
@richardstartin
Copy link
Member Author

To make this work so allocated memory is always freed, every single temporary container allocation which doesn't escape a method needs to be tracked down so the container can be closed. This is hard and incredibly tedious to get right.

@lemire
Copy link
Member

lemire commented Nov 26, 2021

The C version of Roaring does manual memory management, so it is certainly possible.

I suspect the Rust implementation has its own approach since it does not have GC.

@richardstartin
Copy link
Member Author

The C version of Roaring does manual memory management, so it is certainly possible.

Yes, but it was built that way from the start and has to be that way. It wasn’t retrofitted into code built with years of assumptions that the GC does the memory management.

I suspect the Rust implementation has its own approach since it does not have GC.

Rust has automatic reference counting unless I’m very much mistaken.

@lemire
Copy link
Member

lemire commented Nov 26, 2021

Do not trust me when it comes to Rust.

@richardstartin
Copy link
Member Author

If you look at some of the places you have to put container closes this starts to look like a bad idea, especially if Allocator.free has a non negligible cost.

@richardstartin richardstartin force-pushed the memory-manager-prototype branch 2 times, most recently from 6b17a94 to a498ad0 Compare November 26, 2021 12:16
@lemire
Copy link
Member

lemire commented Nov 26, 2021

If you look at some of the places you have to put container closes this starts to look like a bad idea, especially if Allocator.free has a non negligible cost.

I'll come back to my earlier point: you are effectively competing against the Java memory manager which has far more flexibility than you'll ever have.

It is possible that one can end up winning, but it is not obvious. The only way to tell for sure is to fully implement a correct solution and then run benchmarks.

@richardstartin
Copy link
Member Author

I'll come back to my earlier point: you are effectively competing against the Java memory manager which has far more flexibility than you'll ever have.

It is possible that one can end up winning, but it is not obvious. The only way to tell for sure is to fully implement a correct solution and then run benchmarks.

I completely agree, but if there were to be manual memory management, I would prefer for it to have the less disruptive nature on this branch. Unless the user does something, all of the memory management will fold away to allocations and array copies after inlining.

@richardstartin
Copy link
Member Author

I don’t have much enthusiasm for finding every case where containers need to be closed and it kind of ruins the code - things having a GC allows like chaining can’t be used, for example. I also don’t expect much or even any benefits. But this is less invasive than adding a reference to the containers, so if anyone wants to finish or copy this, can demonstrate there is no regression in the default case and that it is worthwhile (in terms of a real metric like system load or max GC pause time, rather than measured allocation rate) then I will leave the branch here.

@alimans3
Copy link

I don’t have much enthusiasm for finding every case where containers need to be closed and it kind of ruins the code - things having a GC allows like chaining can’t be used, for example. I also don’t expect much or even any benefits. But this is less invasive than adding a reference to the containers, so if anyone wants to finish or copy this, can demonstrate there is no regression in the default case and that it is worthwhile (in terms of a real metric like system load or max GC pause time, rather than measured allocation rate) then I will leave the branch here.

I’ll take it from here, and add a custom allocator with local thread pools and show the difference in GC pressure and temporary objects when done.

@richardstartin
Copy link
Member Author

I’ll take it from here, and add a custom allocator with local thread pools and show the difference in GC pressure and temporary objects when done.

“GC pressure” is not a metric I think you should care about. The GC does its job, allows for fast allocations, and by not allocating objects you will achieve a reduction in allocation rate by construction. If you can demonstrate a reduction in max pause time or system load, that would be interesting.

@lemire
Copy link
Member

lemire commented Nov 26, 2021

I concur. The goal cannot be to reduce GC pressure.

You can avoid triggering the GC entirely and make things worse.

@alimans3
Copy link

I concur. The goal cannot be to reduce GC pressure.

You can avoid triggering the GC entirely and make things worse.

What I mean by GC Pressure is reducing creation of temporary objects and therefore less GC pause times in applications where bitmaps are heavily used and with large sizes.

@blacelle
Copy link
Member

blacelle commented Nov 26, 2021

What I mean by GC Pressure is reducing creation of temporary objects and therefore less GC pause times in applications where bitmaps are heavily used and with large sizes.

@alimans3 You are correct in the spirit, as it is true one may observe better overall performances by reducing GC pauses. However, one may also observe worse overall performance (e.g. by preventing the JVM doing various other tricks (@richardstartin wrote articles specifically to such stuff )).

Again, the only way to demonstrate clearly the benefit of this approach would be to provide a concrete benchmark, preferably through a dedicated JMH benchmark. I definitely trust you observe lower GC-pressure in your project, hence I suppose you observes smaller GC pauses, which may be crucial if your application is realtime. However, other non-realtime workload may observe worst overall performances (e.g. Druid, Pinot, Kylin ,...).

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

4 participants