Review Board 2.0.15


config: support outputing a pickle of the configuration tree

Review Request #930 - Created Dec. 16, 2011 and submitted

Information
Ali Saidi
gem5
Reviewers
Default 
config: support outputing a pickle of the configuration tree

This is useful as an input potential input to power models and visualization tools.

   

  


 


 

  

  









  
 


  

 


















 

  


 


  




   
Ship it!



   

   
Nathan Binkert

   
Posted  (Jan. 4, 2012, 7:57 a.m.)


  


  


  

   
Sorry to be pedantic.  I just hate duplicated code. :)  It's not so bad that I require the change and I don't need to review it again if you choose to make the changes I suggest.

Finally, why do you want the pickle format?  I ask because json might be more generally useful.  basically replace pickle.dump with json.dump and then a lot more stuff could suck in the json.  (Someone could build a pretty javascript thing with canvas for displaying the configuration :)



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I looked at it's actually pretty hard to get these into one change. In theory config.ini could be created from it, so I'll think about that but no promises. json doesn't work because right now there are types in the tree that aren't json compatible (E.g. timestruct for rtc devices). 
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Can't you simply add a recurse parameter to get_dict?  For children with recurse, it will do it, for children without recurse, it will just output the string.  Reasonable?
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    Ok.. The time struct was the only place where the dict isn't json compatible. So it can be pickle or json. Any opinions?
    
    
    
   
    4. 

   


  




   







    

     
     

      

       
        

         
          src/python/m5/SimObject.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
This seems to be a massive repeat of the print_ini code.  Could you build the ini_file code on top of this code?

Also, the name ini_str is pretty outdated, is it still necessary?  does str() work?  We could also separate __str__ and __repr__ (though that could certainly be a separate change.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    It's different enough from print_ini that it seems to me it might be hard to fold these together.  One thing that would help is if we added a sorted_keys() method to multidict, then all the code like:

    child_names = self._children.keys()
    child_names.sort()
    for n in child_names:

could be changed to:

    for n in _children.sorted_keys():

both here and in print_ini (and probably elsewhere).

As far as ini_str(), you can look in params.py to see where that differs from str(); since the default for ini_str() is str(), it's only overridden when different, with one exception :).
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    for n in sorted(self._children.keys()) not good enough?

Also, I agree that ini_str and str can be different, but I don't see why that is useful.  We can always just use __str__ instead of ini_str and not care.  ini_str only needed to be special when we were parsing ini files to suck in the configuration.  Now, we can be much more flexible with how we output them.  If we need different representations, then we can use __repr__ and __str__ for them.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    Heh, good point, guess I'm still stuck in pre-2.4 mode.  So we should do that.

For ports (or specifically PortRefs), ini_str() print's the peer's name, while __str__ prints the name of the port itself.  Probably a lot of the other overrides could be changed to just overriding __str__, but ports are the only one that Ali uses.  Though ports are different from params, so we could just call it peer_name() or something like that.
    
    
    
   
    4. 


   
  4. 
    
    
     
    
      
     
    
     
    I guess I'd like to see this, but if ali doesn't want to do it now, I'd understand.  It is a separate problem.
    
    
    
   
    5. 


   
  5. 
    
    
     
    
      
     
    
     
    I don't think I'm going to do the ini_str bit at this point. I'll clean up the storted(...) though.
    
    
    
   
    6. 

   


  


    


    

     
     

      

       
        

         
          src/python/m5/SimObject.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
remove newline :P




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    k
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          src/python/m5/main.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
how about --pickle-config or --pickle-conf.  Not sure if people know what pkl would mean




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    agreed
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    k
    
    
    
   
    3. 

   


  


    


    

     
     

      

       
        

         
          src/python/m5/simulate.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Seems that you could use the get_dict() class here and keep the ini specific code all right here.




  

   

   
    
    
   


  


    


   



  

 


  

 

















 



 

  


 


  



   

   
Steve Reinhardt

   
Posted  (Jan. 4, 2012, 8:38 a.m.)


  


  


  

   
Finally figured out why reviewboard confuses me sometimes... you can't comment on a patch and reply to a previous comment at the same time, you have to do it as two separate operations (even though you can have both the original comment and a reply in progress at the same time, leading to seemingly redundant sets of edit/discard/publish buttons).



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/python/m5/SimObject.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
I'm not fond of this name... it's pretty ambiguous.  I'd prefer something more descriptive like 'get_config_as_dict()'.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    k
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          src/python/m5/simulate.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
This copy-and-paste comment should be updated or deleted or something.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    already fixed
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          src/python/m5/simulate.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Doesn't this create a bunch of redundant data, since you're iterating through all the objects with root.descendants() but each object's dict recursively includes all its children too?

I was expecting you to just pickle root.get_dict() and that's it.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    you're right, I just didn't notice and root.system.blah worked fine; will fix
    
    
    
   
    2.