deeppatel710 opened a new pull request, #18145:
URL: https://github.com/apache/pinot/pull/18145

   ## Summary                                                                   
                                                                                
                                                    
                                                                                
                                                                                
                                                      
     Fixes #17597                                                               
                                                                                
                                                      
                                                                                
                                                                                
                                                      
     `NullValueVectorReaderImpl.getNullBitmap()` was deserializing an 
`ImmutableRoaringBitmap` from the underlying `PinotDataBuffer` on **every 
call**, including every call to `isNull()`. Pyroscope profiling       
     identified this as consuming ~40% of CPU during 
`RealtimeToOfflineSegmentsTask` execution in Minion.                            
                                                                          
                                                                                
                         
     ## Root Cause
                                                                                
                                                                                
                                                      
     ```java
     // Before: constructs a new bitmap on every call                           
                                                                                
                                                      
     public ImmutableRoaringBitmap getNullBitmap() {           
       return new ImmutableRoaringBitmap(_dataBuffer.toDirectByteBuffer(0, 
(int) _dataBuffer.size()));
     }                                           
    ```                                             
   
     Fix                                     
                                                                                
                                                                                
                                                      
     Cache the result in a volatile field after the first construction. 
ImmutableRoaringBitmap is thread-safe once built, so volatile provides safe 
publication without lock overhead on the hot path.                
   
   ```                                                                          
                                                                                
                                                  
     private volatile ImmutableRoaringBitmap _nullBitmap;                       
                                                                                
                                                      
                                                                                
                                                                                
                                                      
     public ImmutableRoaringBitmap getNullBitmap() {
       ImmutableRoaringBitmap nullBitmap = _nullBitmap;                         
                                                                                
                                                      
       if (nullBitmap == null) {                               
         nullBitmap = new ImmutableRoaringBitmap(...);                          
                                                                                
                                                      
         _nullBitmap = nullBitmap;                             
       }                                                                        
                                                                                
                                                      
       return nullBitmap;                        
     }                                                                          
                                                                                
                                                      
   ```                                                      
     This caching existed previously (PR #7493) and was inadvertently removed.  
                                                                                
                                                      
                                             
   
   ## Test plan                                                                 
                                                                                
                                                       
                                                                                
                                                                                
                                                      
     [] Existing testNullValueVectorReader passes 
     [] New testNullBitmapIsCached asserts getNullBitmap() returns the same 
instance on repeated calls


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to