----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72666/#review221333 -----------------------------------------------------------
intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java Lines 51 (patched) <https://reviews.apache.org/r/72666/#comment310221> this assumes that the elements are accessed in ascending oeder - since getAndIncrementLength() simply increments the length by 1. I suggest to get rid of getAndIncrementLength() and update getForUpdate() as below: public T getForUpdate(int index) { ensureCapacity(index + 1); T ret = buffer.get(index); // update length only if the accessed index is beyond the current length if (this.length <= index) { this.length = index + 1; } return ret; } intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java Lines 55 (patched) <https://reviews.apache.org/r/72666/#comment310223> To make it easier to read, I suggest to get rid of method resetCurrentLength() and update toList() as below: public List<T> toList(boolean resetList) { List<T> ret = this.buffer.subList(0, this.length); if (resetList) { this.length = 0; } } intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java Lines 60 (patched) <https://reviews.apache.org/r/72666/#comment310226> - is 'incrementCapacityBy' argument needed, given this is available in instance memeber this.incrementCapacityBy? - consider renaming request() => ensureCapacity() - consider folding adjustCapacity(), instantiateItems() into this method, as below: private void ensureCapacity(int capacity) { if (capacity > this.buffer.size()) { int currCapacity = this.buffer.size(); int newCapacity = currCapacity + incrementCapacityBy; while (newCapacity < capacity) { newCapacity += incrementCapacityBy; } this.buffer.ensureCapacity(newCapacity); // initialize new entries for (int i = currCapacity; i < newCapacity; i++) { this.buffer.add(itemClass.newInstance()); } } } intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java Lines 71 (patched) <https://reviews.apache.org/r/72666/#comment310225> adjustCapacity() => ensureCapacity() intg/src/main/java/org/apache/atlas/utils/FixedBufferList.java Lines 87 (patched) <https://reviews.apache.org/r/72666/#comment310227> Consider moving instantiateItems() into adjustCapacity() implementation; this will avoid callers of adjustCapacity() to call instantiateItems() as well . intg/src/main/java/org/apache/atlas/utils/FixedBufferListAccessor.java Lines 44 (patched) <https://reviews.apache.org/r/72666/#comment310228> Why does this block need to be under synchronized? No state is updated in this block. intg/src/test/java/org/apache/atlas/utils/FixedBufferListAccessorTest.java Lines 33 (patched) <https://reviews.apache.org/r/72666/#comment310229> verifyPeriodicPurge() - the method name doesn't seem to related the implementation. Please review and update. repository/src/main/java/org/apache/atlas/repository/audit/EntityAuditListenerV2.java Lines 90 (patched) <https://reviews.apache.org/r/72666/#comment310230> FixedBufferListAccessor class doesn't seem to add much value. Consider the following simper usage: private static final ThreadLocal<FixedBufferList<EntityAuditEventV2>> AUDIT_EVENTS_BUFFER = new ThreadLocal.withInitial(() -> new FixedBufferList<EntityAuditEventV2>(FIXED_BUFFER_INITIAL_SIZE_DEFAULT, FIXED_BUFFER_INCREMENT_DEFAULT)); Note the use of 'static' above; fixedBufferListAccessor can also be eliminated. You might consider a simple accesser method in EntityAuditListenerV2: private FixedBufferList<EntityAuditEventV2> getAuditEventsBuffer() { return AUDIT_EVENTS_BUFFER.get(); } Same applies for EntityNotificationListenerV2 as well. - Madhan Neethiraj On July 23, 2020, 6:48 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72666/ > ----------------------------------------------------------- > > (Updated July 23, 2020, 6:48 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/AtlasConfiguration.java 2c007ca01 > 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/7/ > > > 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. Without out of memory error. > > **Pre-commit** > https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/2035/ > > > Thanks, > > Ashutosh Mestry > >
