Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Bug or Feature: directly assign 'true' to delay_alloc of Chunk rather than using input param delay_alloc_? #20343

Open
ooooona opened this issue Jun 11, 2021 · 4 comments

Comments

@ooooona
Copy link

ooooona commented Jun 11, 2021

Description

I'm confuse about the coding here:

/*! \brief construct a new chunk */    
Chunk(mxnet::TShape shape, Context ctx_, bool delay_alloc_, int dtype)       
 : static_data(false), delay_alloc(true), ctx(ctx_),          
    storage_ref_(Storage::_GetSharedRef()),          
    engine_ref_(Engine::_GetSharedRef()) {      
  storage_shape = shape;      
  if (shape_is_known(storage_shape)) {        
    shandle.size = shape.Size() * mshadow::mshadow_sizeof(dtype);      
  } 
   var = Engine::Get()->NewVariable(); 
   shandle.ctx = ctx_; 
   if (!delay_alloc_) { 
     this->CheckAndAlloc(); 
   }
  }

here is source code

question

why not assign delay_alloc_ to delay_alloc?

@szha
Copy link
Member

szha commented Jun 11, 2021

Nice catch. Looks like a bug to me. @ooooona thanks for reporting. Would you like to open a pull request and fix the bug?

@szha
Copy link
Member

szha commented Jun 11, 2021

Seems to be introduced by #14223

@ooooona
Copy link
Author

ooooona commented Jun 15, 2021

Nice catch. Looks like a bug to me. @ooooona thanks for reporting. Would you like to open a pull request and fix the bug?

sure, i will try it later today :)

ooooona added a commit to ooooona/incubator-mxnet that referenced this issue Jun 15, 2021
hotfix: assign  input param `delay_alloc_` to `delay_alloc` rather than 'true'
pls refer to [issu#20343](apache#20343)
@wkcn
Copy link
Member

wkcn commented Jun 18, 2021

Thank you for pointing it out : )

It is a confusing variable name, but it is not a bug.

The meanings of decay_alloc_ and decay_alloc are different.
The former decay_alloc_ is an argument to decide whether the chunk need to decay-allocated.
But the later decay_alloc is the state whether the memory has not been allocated.

For the two cases decay_alloc_ is true or false,

CheckAndAlloc() will allocate memory when decay_alloc is true.

Therefore, decay_alloc should be initialized as true whenever decay_alloc_ is true or false.

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

No branches or pull requests

3 participants