-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72666/#review221193
-----------------------------------------------------------




intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 30 (patched)
<https://reviews.apache.org/r/72666/#comment309970>

    collectionName - doesn't seem to be used, except for the log statement at 
#79. Consider removing this, if its not needed.



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 31 (patched)
<https://reviews.apache.org/r/72666/#comment309973>

    bufferElementClass => elementClass



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 35 (patched)
<https://reviews.apache.org/r/72666/#comment309972>

    bufferMaxSize => capacity



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 36 (patched)
<https://reviews.apache.org/r/72666/#comment309971>

    currentIndex => length
      looks like currentIndex is used to determine number of valid entries in 
the buffer. Consider renaming to 'length'



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 38 (patched)
<https://reviews.apache.org/r/72666/#comment309969>

    'bufferElementClass' can be derived from the generic, as below. With this, 
bufferElementClass argument can be eliminated.
    
      ParameterizedType genericSuperclass = (ParameterizedType) 
getClass().getGenericSuperclass();
      Type              type              = 
genericSuperclass.getActualTypeArguments()[0];
    
      if (type instanceof ParameterizedType) {
        this.bufferElementClass = (Class<T>) ((ParameterizedType) 
type).getRawType();
      } else {
        this.bufferElementClass = (Class<T>) type;
      }



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 56 (patched)
<https://reviews.apache.org/r/72666/#comment309974>

    - set() => add() - to be consistent with ArrayList
    - better yet, consider collapsing both get() and set() with the following:
        public T getForUpdate(int idx) {
          if (this.length <= idx) {
            this.length = (idx + 1);
            
            request(this.length);
          }
    
          return buffer.get(idx);
        }



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 57 (patched)
<https://reviews.apache.org/r/72666/#comment309978>

    Given incrementByFactor is an instance member, is it necessary to send as 
argument?



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 62 (patched)
<https://reviews.apache.org/r/72666/#comment309975>

    Calling toList() effectively clears the buffer (since the 
currentIndex/length is reset). This is not intutive. Consider adding a separate 
method, reset(), for this.



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 67 (patched)
<https://reviews.apache.org/r/72666/#comment309976>

    Given incrementByFactor is specified in the constructor, why increment by 
'1' here? Consider merging #66 and #70 into a single method.



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 77 (patched)
<https://reviews.apache.org/r/72666/#comment309979>

    It might help to defer instantiation until an entry is accessed. Consider 
handling this in get(idx):
      public T get(int i) {
        request(i + 1);
        
        T ret = buffer.get(i);
        
        if (ret == null) {
          ret = elementClass.newInstance();
          
          buffer.set(i, ret);
        }
        
        return ret;
      }
    
    With this change, instantiateElements() will not be needed.



intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java
Lines 83 (patched)
<https://reviews.apache.org/r/72666/#comment309977>

    The intrepretation of 'incrementByFactor' doesn't look right. Shouldn't it 
be something like:
    
      // incrementByFactor should be a float, with value > 0
      // incrCapacity can be initialized in constructor as well
      int incrCapacity = (int) (this.capacity * incrementByFactor) + 1.0F;
    
      int newCapacity = this.capacity; 
    
      for (; newCapacity < requestedSize; newCapacity += incrCapacity);
      
      this.buffer.ensureCapacity(newCapacity);
      
      return newCapacity;



intg/src/main/java/org/apache/atlas/utils/FixedBufferListAccessor.java
Lines 25 (patched)
<https://reviews.apache.org/r/72666/#comment309980>

    Instead of class FixedBufferListAccessor, consider using the follownig:
    
      ThreadLocal<FixedBufferList<T>> buffer = ThreadLocal.withInitial(() -> 
new FixedBufferList<T>(FIXED_BUFFER_INITIAL_SIZE_DEFAULT, 
FIXED_BUFFER_INCREMENT_DEFAULT));


- Madhan Neethiraj


On July 10, 2020, 4:43 p.m., Ashutosh Mestry wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72666/
> -----------------------------------------------------------
> 
> (Updated July 10, 2020, 4:43 p.m.)
> 
> 
> Review request for atlas, Madhan Neethiraj, Nikhil Bonte, Nixon Rodrigues, 
> and Sarath Subramanian.
> 
> 
> Bugs: ATLAS-3878
>     https://issues.apache.org/jira/browse/ATLAS-3878
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> **Background**
> See JIRA for details.
> 
> *Analysis* Using memory profiling tools, it was observed that large number of 
> notification objects were created. These stayed in memory and later were 
> promoted to higher generation, thereby taking even longer to be collected.
> 
> **Approach**
> Using the fixed-buffer approach to address the problem of creating large 
> number of small objects.
> 
> New *FixedBufferList* This is an encapsulation over *ArrayList*. During 
> initial allocation, list is populated with default values. Features:
> - Setting of values to these pre-allocated objects is achieved by first doing 
> a *get* on the element and then assigning values to it.
> - *toList* fetches the sub-list from the encapsulating list. This uses the 
> state within the class to fetch the right length for the returning array.
> 
> New *NamedFixedBufferList* Maintains a per-thread *FixedBufferList*. This is 
> necessary since the list is now part class's state.
> Modified *EntityAuditListenerV2* Uses the new classes.
> Modifed *EntityNotificationListener* Uses the new classes.
> 
> **Verification**
> - Using the test setup, the memory usage was observed over a period of 24 
> hrs. 
> - Memory usage and object allocation was obvserved using memory profiler.
> 
> 
> Diffs
> -----
> 
>   intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java PRE-CREATION 
>   intg/src/main/java/org/apache/atlas/utils/FixedBufferListAccessor.java 
> PRE-CREATION 
>   intg/src/test/java/org/apache/atlas/utils/FixedBufferListAccessorTest.java 
> PRE-CREATION 
>   intg/src/test/java/org/apache/atlas/utils/FixedBufferListTest.java 
> PRE-CREATION 
>   
> repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditListenerV2.java
>  79527acfa 
>   
> webapp/src/main/java/org/apache/atlas/notification/EntityNotificationListenerV2.java
>  a677b315c 
> 
> 
> Diff: https://reviews.apache.org/r/72666/diff/3/
> 
> 
> Testing
> -------
> 
> **Unit testing**
> Unit tests added for the new classes.
> 
> **Volume testing**
> Setup:
> - Node: Threads 40, Core: 40, Allocated Memory: 12 GB
> - Multiple Kafka queues ingesting data.
> - Bulk entity creation using custom script ingesting 100M entities.
> 
> Memory usage stayed between 0 and 5% during the 24 hr period.
> 
> With:
> - Workers: 64
> - Batch size: 50 (fewer elements in batch improve commit time and audit write 
> time).
> - Throughput: ~1.2 M entities per hour.
> 
> **Pre-commit**
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/2035/
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Reply via email to