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

Introduce liballoc::actual_usable_size #32075

Closed
nox opened this issue Mar 6, 2016 · 25 comments
Closed

Introduce liballoc::actual_usable_size #32075

nox opened this issue Mar 6, 2016 · 25 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@nox
Copy link
Contributor

nox commented Mar 6, 2016

In Servo we use heapsize to be able to report about heap memory usage. It would be way more robust if heapsize's function heap_size_of were in liballoc itself.

Probably that function should be parameterised by T instead of taking a *mut c_void, to handle alignment without relying on HeapValidate on Windows.

@SimonSapin
Copy link
Contributor

CC @pnkfelix

@pnkfelix
Copy link
Member

... I don't know if we can reliably provide this for all potential allocators ...

Would you be willing to make the API more flexible, e.g. have it return Option<usize>, with a best-effort expectation?

@SimonSapin
Copy link
Contributor

je_malloc_usable_size only takes a pointer and somehow that’s enough for jemalloc to return a meaningful value. But we don’t need all allocators to be that magical, we can provide them with more information:

// In the alloc::heap module
pub unsafe fn usable_size(ptr: *mut u8, size: usize, align: usize) -> usize { ... }

Namely, the same information we need to keep track of anyway for alloc::heap::deallocate. Simply returning the size argument would be a valid implementation of usable_size.

@retep998
Copy link
Member

Since the size returned by HeapSize on Windows is always exactly the size you asked it to allocate, without any rounding up or extra space, simply returning the size argument would be just as accurate on Windows.

@pnkfelix
Copy link
Member

Okay I'm fine with this as long as it meets the same input requirements as deallocate

@nox
Copy link
Contributor Author

nox commented Mar 26, 2016

Since the size returned by HeapSize on Windows is always exactly the size you asked it to allocate, without any rounding up or extra space, simply returning the size argument would be just as accurate on Windows.

@retep998 You mean return size + align, right?

@nox
Copy link
Contributor Author

nox commented Mar 26, 2016

The point of this function isn't to return how much space can be used in a give pointer, but about how much space did I need to actually allocate.

@retep998
Copy link
Member

@nox Well in the case of overaligned types it is size + align. if align <= MIN_ALIGN { size } else { size + align }.

@nox
Copy link
Contributor Author

nox commented Mar 27, 2016

Yeah ok, just checking that we were talking about the same thing. :)

@SimonSapin
Copy link
Contributor

The point of this function isn't to return how much space can be used in a give pointer

FWIW early versions of https://github.com/servo/tendril did use je_malloc_usable_size that way. (I removed that when porting it to stable Rust.)

@nox
Copy link
Contributor Author

nox commented Mar 4, 2017

What can we do to get some progress about this?

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum
Copy link
Member

Nominating for @rust-lang/libs discussion so that we can move forward, it seems Servo wants this.

@alexcrichton
Copy link
Member

The libs team discussed this during triage today and now that we have global allocators on the horizon such a feature is likely best implemented through a known allocator which can then be used to, for example, codify the implementation above as "correct". In light of that, I'm going to close this issue as we're unlikely to add such a method like this to the Allocator trait.

@SimonSapin
Copy link
Contributor

a known allocator which can then be used to, for example, codify the implementation above as "correct".

The implementation above is a very fragile hack. The point of this request is that it would not be needed anymore. I may be missing a step in the reasoning how global allocators help here.

@nox
Copy link
Contributor Author

nox commented Jun 7, 2017

I second what @SimonSapin said, this hack is fragile and makes me wary for the future.

This issue and the wish for such a function is only a preliminary step in our quest to correct memory reporting. Once it is in libstd, I would like HashMap and whatnot to be able to be introspected for their memory consumption.

If global allocators don't help on that, I don't see why I was pinged into that RFC at all.

@alexcrichton
Copy link
Member

Global allocators allow you to define what the global allocator is, turning this from a "hack" into a correct implementation.

@SimonSapin
Copy link
Contributor

The fragility of this hack is not only "are we using jemalloc at all", it also depends on details of how exactly std links to jemalloc. At some point the symbol name changed from je_malloc_usable_size to malloc_usable_size (or was it the opposite?) but only on some platforms.

@nox
Copy link
Contributor Author

nox commented Jun 7, 2017

And there is still the elephant in the room, which is that if the function isn't in the standard library, then obviously the standard library won't be able to use it for the types it defines.

@alexcrichton
Copy link
Member

Er, ok, let me be clearer.

  • We are adding support for customizing allocators in programs
  • This means Servo can define its own allocator, whatever that is.
  • When using a custom global allocator, jemalloc and the system allocator are not used
  • As Servo is now in complete control of the allocator, it can decide symbol names and what functionality is available, it's all your problem at that point not libstd.

The point of global allocators is that this is a local problem, not a libstd problem. This code goes in Servo, not in libstd.

@nox
Copy link
Contributor Author

nox commented Jun 7, 2017

How does defining our own allocator means we can inspect how much memory was allocated for a HashMap? That's still an internal implementation detail of the collection type, and the measurement function cannot be exact from Servo code.

@nox
Copy link
Contributor Author

nox commented Jun 7, 2017

This is how we currently measure how much space a HashMap is taking: https://github.com/servo/heapsize/blob/master/src/lib.rs#L269-L278

This cannot be done in an exact way from outside the standard library, which means that if we want this feature, we need some sort of malloc_usable_size in there.

@alexcrichton
Copy link
Member

@nox I fear that there's a lack of communication here. This issue, as stated, I assume is for a function like mentioned above. That function is not necessary to put in liballoc if you can have a custom global allocator.

The problem you're mentioning, tracking the precise memory usage of collections in libstd, sounds unrelated to this issue itself and may want its own dedicated issue if it's required.

@aturon
Copy link
Member

aturon commented Jun 7, 2017

So to further clarify:

  • I believe at this point @alexcrichton has explained how to implement the precise functionality mentioned in this issue, in a robust way, by fully controlling and talking directly to the system allocator. @SimonSapin, do you agree/does that make sense to you?

  • Separately, there's a question of getting information about std collections, which seems to require more than simply providing this function (so as @alexcrichton said probably needs a separate issue/some level of design).

Does that seem right?

@nox
Copy link
Contributor Author

nox commented Jun 28, 2017

This is right. I just don't see how getting information about std collections will be doable without such a function directly in liballoc, but I'll file a different issue with the whole story instead.

@nox
Copy link
Contributor Author

nox commented Jul 18, 2017

@aturon Should I file an issue for getting information about std collections? A Servo contributor just mentioned Sender<T> and Receiver<T>, which are even more opaque than the collections and thus we really have no way to inspect those for their memory usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants