JackieTien97 commented on code in PR #18454:
URL: https://github.com/apache/iotdb/pull/18454#discussion_r3772297733
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -236,13 +211,19 @@ public synchronized AlignedTVList clone() {
public synchronized PartialClonePlan preparePartialClone(Set<Integer>
columnsToClone) {
Set<Integer> retainedColumns =
new HashSet<>(Objects.requireNonNull(columnsToClone, "columnsToClone
cannot be null"));
- AlignedTVList cloneList = AlignedTVList.newAlignedList(new
ArrayList<>(dataTypes));
+ AlignedTVList cloneList = AlignedTVList.newAlignedList(new
ArrayList<>(dataTypes), false);
Review Comment:
**[P1] Refresh the clone's per-block memory cache after moving the lazy
columns**
With `initializeValueColumns=false`, every non-retained value slot is still
null when `prepareMovePlan` evaluates
`cloneList.calculateArrayMemCostWithoutIndex(null)`. `commitPartialClone` then
moves those column lists into the clone but installs that pre-move value into
`cloneList.arrayMemCostWithoutIndex`, so the replacement working TVList
undercounts one object reference per moved column per block. A focused case
with 3 INT64 columns, 2 blocks, and 1 retained column returns 4592 bytes
instead of 4608; for 500 columns, 150 rows, and 1 retained column this is 5,988
bytes immediately and 1,996 bytes for every later block (with 4-byte
references). The write estimator and its snapshot both consume the same stale
cache, so reconciliation does not repair the gap. Please recompute/refresh both
lists after the move, as the final master implementation does, and add a
clone-RAM invariant plus a subsequent-write test.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -1347,7 +1330,7 @@ public static long alignedTvListArrayMemCost(TSDataType[]
types) {
/** Initial list-container memory before the first aligned row is written. */
public static long alignedTvListInitialMemCost(int measurementColumnCount) {
- long arrayListShallowSize =
RamUsageEstimator.shallowSizeOfInstance(ArrayList.class);
+ long arrayListShallowSize =
MemoryEstimationHelper.ARRAY_LIST_INSTANCE_SIZE;
Review Comment:
**[P1] Account for `materializedValueArrayCounts` in every dev/1.3 memory
estimator**
This initial-cost calculation still omits the `int[measurementColumnCount]`
allocated by every `AlignedTVList`. The final master fix also charges this
N-wide container in `calculateContainerRamCost`; for dev/1.3 the
branch-specific write accounting additionally requires adding
`sizeOfIntArray(measurementColumnCount)` here and the corresponding old-to-new
size delta in `columnExtensionMemCost`. At 500 columns the omission is about
2,016 bytes per TVList. Because `getRamSize()`, write snapshots, query
reservation, and cleanup all share the same incomplete model, their values can
agree while real heap usage remains silently under-accounted. This was present
in the base branch, but this PR explicitly promises to complete the review
fixes from #18409, so the master container fix still needs to be adapted and
backported here.
--
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]
