----------------------------------------------------------- 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 > >
