Profiling allocator for mobile.

[kimishpatel]

Stack from ghstack:

• Profiling allocator for mobile. #43951 Profiling allocator for mobile.

Summary:
AllocationPlan:
Stores the sequence of allocations, their sizes and liftime of the allocations. Along with this it also stores the total size of a single memory blob, total_size, required to satisfy all the allocations. It also stores the offsets in the blob, of size total_size, corresponding to each allocation.
Thus allocation plan contains:

• allocation sizes
• allocation lifetimes
• allocation offsets
• total size
  AllocationPlaner: Takes a pointer to the allocation plan and fills it ups with plan, i.e. sizes, lifetimes, offsets, total size. This is done via WithProfileAllocationsGuard which takes in AllocationPlan* and constructs AllocationPlanner* and set the thread local allocation_planner to it. MobileCPUAllocator profiles allocations via allocation_planner. In WithValidateAllocationsGuard, allocations profiled in the allocation plan are validated.

CPUProfilingAllocator:
Application owns CPUProfilingAllocator Using WithProfilingAllocatorGuard, it passes both CPUProfilingAllocator and AllocationPlan created earlier. Then CPUProfilingAllocator will manage allocations and frees according to the plan. Allocations that are not managed by CPUProfilingAllocator will be routed through c10::alloc_cpu, c10::free_cpu.

Test Plan:
cpu_profiling_allocator_test on mobile.

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D23451019

[dr-ci]

💊 CI failures summary and remediations

As of commit 1f0cb92 (more details on the Dr. CI page):

---

Commit 1f0cb92 was recently pushed. Waiting for builds...

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 12 times.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (gh/kimishpatel/24/base@45ddeb5). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                    @@
##             gh/kimishpatel/24/base   #43951   +/-   ##
=========================================================
  Coverage                          ?   67.91%           
=========================================================
  Files                             ?      384           
  Lines                             ?    49839           
  Branches                          ?        0           
=========================================================
  Hits                              ?    33849           
  Misses                            ?    15990           
  Partials                          ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45ddeb5...87e1d08. Read the comment docs.

[dreiss]

Does this mean that X can be used to satisfy Y, or not? From my read of the code, the answer is yes, so maybe we should document that Y is the id of the first allocation after X is freed.

[ezyang]

I read the comment after the addition of "Thus Y is the id of the first allocation after X is freed." I felt that dreiss's first statement, "X can be used to satisfy Y" was the clearest explanation of the lifetimes here.

[kimishpatel]

I can add that if it helps clarify but the reason I did not was because we are not really creating aliases of allocation ids.

[dreiss]

"success"

[dreiss]

"Guard"

[dreiss]

"success" seems unnecessary here.

Also, why not throw on failure, rather than returning a bool?

[kimishpatel]

Mainly so that user can decide note to use ProfilingAllocator based on the result of success. Other option is to have user run that in try.. catch block and make the same decision based on the outcome.

[ezyang]

Exceptions really slow, so if you expect this to be failing on a regular basis, bool return is much better.

[dreiss]

I think this last line of the comment is wrong.

[dreiss]

Why allocate this on the heap?

[kimishpatel]

I did not want AllocationPlanner default constructible so that it forces you to provide a valid plan pointer. Not sure if that is a good design, but thats what lead to this.

[dreiss]

it->second

[dreiss]

Might want to throw here with details of the mismatch.

[kimishpatel]

So for the usage I have in mind, I still want to return boolean. Let me add warning with details.

[dreiss]

Might want to extract this expression into a local variable.

[dreiss]

auto limit = allocation_plan_->allocation_offsets[i] + allocation_plan_->allocation_sizes[i];
allocation_plan_->total_size = std::max(allocation_plan_->total_size, limit);

[dreiss]

I didn't review this carefully since it's the lowest-risk part of this change.

I'd suggest following up with the Glow team to see if they have better algorithms for this.

[ezyang]

Is there any particular reason this lives as a separate concept from caching allocator? Given that there is never a situation where BOTH allocators are used, it seems like having two separate thread local allocators is unnecessary?

[ezyang]

By the way, the use of thread local allocator here seems super questionable. If pointers escape from the thread local scope of the allocator, you will deallocate them with the wrong allocator here (which will probably cause a segfault). And we already have machinery for making sure you associate a correct deleter with each allocation (that's what DataPtr is for), so I'm not sure why you have to do it this way.

[kimishpatel]

Is there any particular reason this lives as a separate concept from caching allocator? Given that there is never a situation where BOTH allocators are used, it seems like having two separate thread local allocators is unnecessary?

Thanks Ed for the comment. Idea with profiling allocator is that you create a plan offline and use that plan at runtime, throwing exception when it fails. This PR introduces only the first part, where allocation plan does not have to be serialized with model but later we want to introduce those changes so as to not have to run profiling and allocation planning on mobile. Caching allocator on the other hand is model independent and it allows user to implements its own allocator by making caching allocator a virtual class.

[kimishpatel]

If pointers escape from the thread local scope of the allocator

Can you explain what do you mean by this?

If you mean that pointer is allocated within the thread local scope but deallocated outside, the correct behavior should be that such an allocation is not managed by profiling allocator. If there is a bug that does not ensure that behavior we need to fix that, but the design is that only allocations that are allocated and freed within this thread local scope are managed by profiling allocator.

[ezyang]

This PR introduces only the first part, where allocation plan does not have to be serialized with model but later we want to introduce those changes so as to not have to run profiling and allocation planning on mobile.

OK, I'll trust you on this. My general point was that if you only have one thread local allocator, you might be able to manage this at a higher level, in the selection of what allocator you register to the thread local at any given point. But maybe this is not so easy to do in the future.

[ezyang]

Continuing from https://github.com/pytorch/pytorch/pull/43951/files#r492446863 the way to fix this problem is to change the API of the allocate indirection here to return a DataPtr instead of a void* pointer. Then the thread local profiling allocator that was in scope at this time can provide an appropriate deleter (rather than having to do the lookup again upon deletion). You can then dispense with the deleter static method entirely.

[kimishpatel]

So as I said in my previous comments profiling allocator manages only those allocations which are allocated + freed within the scope. However, your point might still be valid, so I need to think if this is a better way to do the same thing. I will make changes accordingly.

[ezyang]

Yes, I can believe that under your intended usage pattern, the failure case that I had described would not happen. But if that's the precondition, you are at very least obligated to document the precondition (I don't think it is documented right now); and furthermore, the framework goes out of its way to make it easy for you to do things correctly even the more general situation when allocations escape the scope. (In some sense, the latter point makes it more important to handle this correctly, as most people will assume your described precondition doesn't need to hold for all allocators!)

[kimishpatel]

Yes, I can believe that under your intended usage pattern, the failure case that I had described would not happen.

To make sure we understand each other. What I mean is that if you use profiling allocator and utils to generate allocation plan and use that plan for allocation, it will not manage allocations whose lifetimes are outside the scope of the allocator. I will add comments to the effect, such that user would know that not all allocations made inside the scope are managed by profiling allocator.

In some sense, the latter point makes it more important to handle this correctly, as most people will assume your described precondition doesn't need to hold for all allocators.

I can agree with this. But when you say "handle this correctly" do you mean that, "handle correctly even when the precondition (that allocations with lifetime outside the scope are not managed by the allocator) does not need to be held true"?

[ezyang]

What I mean is that if you use profiling allocator and utils to generate allocation plan and use that plan for allocation, it will not manage allocations whose lifetimes are outside the scope of the allocator.

Yes. Though, I thought I saw some comments that implied that the profiling process can also detect allocations that escape the scope (you assign them int max ending, right?)

But when you say "handle this correctly" do you mean that, "handle correctly even when the precondition (that allocations with lifetime outside the scope are not managed by the allocator) does not need to be held true"?

Yes.

[kimishpatel]

Though, I thought I saw some comments that implied that the profiling process can also detect allocations that escape the scope (you assign them int max ending, right?)

That is correct.

[ezyang]

Shouldn't a block be considered non-overlapping if it is zero size, even if start_offset == other.start_offset? What sense is this < overload being used?

[ezyang]

If this is true, then the test here can be as simple as end_offset <= other.start_offset, right? (Assuming that start_offset <= end_offset)

[kimishpatel]

So this is used to make sure that allocation plan has not allocated overlapping regions. I am doing this by creating binary tree in which insertions are made. Two allocations are considered overlapping and thus will return false for < if when comparison is made for insertion.
I think your observation is correct. Thanks for commenting. I will fix that.

[dzhulgakov]

what exactly does this validation check? That not two allocations land in exactly the same spot? But they could if one of them got deallocated and reallocated, right? I'd imagine that you want to check actual alloc/dealloc across time instead

[kimishpatel]

Thanks for the catch @dzhulgakov. I thought I covered this but clearly not. I will fix it.

[dzhulgakov]

nit for future: in general vector of structs is usually more readable than a bunch of vectors

[facebook-github-bot]

This pull request has been merged in a09e109.