apache / apache/druid

Remove eager loading and on-heap caching of "internalFiles" map in SmooshedFileMapper

Open
#10,296 1 comment 0 reactions 0 assignees View on GitHub
Design Review Performance Proposal
Dominant language
Java
Stars
14.1k
Forks
3.8k
Avg merge
2d 58m
Merged PRs (30d)
233

Description

This is a not-so-straightforward follow-up to https://github.com/apache/druid/pull/10295

### Motivation

For each segment loaded, there is one instance of `SmooshedFileMapper` which contains a `Map` where each entry typically is for one column (in certain cases there could be more per column but that is not important for the discussion here). Depending upon number of segments loaded and number of columns, this wastes heap and there is no real reason to keep that map around in the heap during the full lifetime of Druid process.

### Proposed changes

Remove `private final Map internalFiles` from `SmooshedFileMapper` and have something like following instead...

```
private final File segmentBaseDir;
private static final ThreadLocal internalFileObjRef;

private static class InternalFilesObj
{
private final File segmentBaseDir;
private final Map internalFiles;

public InternalFilesObj(
File segmentBaseDir,
Map internalFiles
)
{
this.segmentBaseDir = segmentBaseDir;
this.internalFiles = internalFiles;
}
}

private Map getInternalFiles()
{
InternalFilesObj obj = internalFileObjRef.get();
if (obj != null && obj.segmentBaseDir.equals(segmentBaseDir)) {
return obj.internalFiles;
} else {
Map internalFilesMap = loadInternalFilesMap();
obj = new InternalFilesObj(segmentBaseDir, internalFilesMap);
internalFileObjRef.set(obj);
return obj;
}
}
```

That leads to one map cached per thread rather than per loaded segment on the Druid node.

### Rationale

Another approach considered was to store that metadata information as a sorted set of columns names , and an array of `Metadata` objects in the metadata file (both created/stored using `GenericIndexedWriter`). With that, we can read the information from file directly without ever building an on-heap map object. Lookup would be O(lg N) but everything will be lazy and totally off-heap.
However, current textual format of metadata.drd file is helpful while debugging and it is useful to be able to do `cat metadata.drd` and this alternative approach would make the format binary, Also a little more complex than the changes proposed above.

### Operational impact

None

### Test plan (optional)

Existing tests would cover the changes introduced.

Contributor guide

Open the contributing guide

Research direction

Search the Java sources for SmooshedFileMapper and inspect its internalFiles handling before reviewing the existing tests that cover this class. Done means metadata is no longer retained in an on-heap map for every loaded segment, the proposed per-thread behavior works, and the existing tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, performance
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.