apache / apache/lucene

feat(util): clarify `Accountable` ownership semantics and fix inconsistent `ramBytesUsed()`

Open
#16,113 1 comment 0 reactions 0 assignees View on GitHub
type:enhancement
Dominant language
Java
Stars
3.6k
Forks
1.4k
Avg merge
2d 11h
Merged PRs (30d)
88

Description

## Goal

Clarify the `Accountable.ramBytesUsed()` ownership semantics first, so that any subsequent work on missing or inconsistent implementations has a single rule to apply. The contract is the prerequisite; revisiting implementations is downstream and out of scope here.

Java has no synchronous release event for memory; `ramBytesUsed()` is a snapshot of currently held bytes, with `close()` as the one synchronous release signal the language offers for resources that have one.

## Problem

`Accountable.ramBytesUsed()` is specified as "an estimate of the bytes used by this object" without saying whether bytes from referenced objects are included. In practice, implementations in the Lucene codebase vary: some count everything reachable from the object, including borrowed/shared state passed in via the constructor; some count only what they allocated; a few have explicit comments noting that a referenced object "should be accounted separately."

When external code sums `ramBytesUsed()` across multiple `Accountable` instances that share state, the same memory can be counted more than once.

`getChildResources()` is conventionally a diagnostic accessor for printing and inspection. It does not advertise itself as a deduplicated ownership tree, and consumers should not assume that summing over it yields a well-defined per-node total.

## Examples of divergence

These are not bug reports. They show that today's contract admits multiple defensible choices, which is what the proposal is meant to resolve.

**`GlobalOrdinalsQuery`** counts a shared `OrdinalMap` received through its constructor:

```
// lucene/join/src/java/org/apache/lucene/search/join/GlobalOrdinalsQuery.java
this.ramBytesUsed = BASE_RAM_BYTES + ... + RamUsageEstimator.sizeOfObject(this.globalOrds) + ...
```

The `OrdinalMap` is cached on the top reader context and shared across queries that target the same join field on the same index. Two such queries each include its full size, so a consumer summing across queries sees the same bytes more than once. This is a defensible local choice under today's contract; it just isn't the only one.

**`GlobalOrdinalsWithScoreQuery`** in `lucene/join/src/java/org/apache/lucene/search/join/GlobalOrdinalsWithScoreQuery.java` has the same pattern at the equivalent site.

**`Lucene99HnswVectorsWriter.FieldWriter`** includes `flatFieldVectorsWriter.ramBytesUsed()` in its own count. The outer writer's per-field loop comments that the field tracks the delegate's usage and the outer omits the delegate from its own count to compensate. The pair sums to the right total today; the inner and outer stay in step through a comment rather than through the contract. A clarified contract would make where the bytes are attributed explicit without changing the total.

## Downstream impact

Embedders (Elasticsearch, Solr, OpenSearch, custom indexers) build memory inventories by summing `ramBytesUsed()` across the Accountables they hold open. When two Accountables that share state both report the shared size, the totals inflate, and downstream consumers cannot tell from the API which value reflects actual heap. Capacity planning and circuit breaker thresholds drift, diagnostics become forensic since the API alone cannot answer why a reader reports N GB on an M GB heap, and per shard or per segment inventories cannot produce useful totals without a deduplication layer.

## Proposal

1. **Clarify `Accountable`'s javadoc** with an ownership-oriented convention. Define "owned" as memory allocated for this object's lifetime, or released by its `close()` if applicable. Inputs received via constructors or factories — borrowed, wrapped, sliced, or shared — are reported only to the extent they are owned, not by their deep content. The reference slot itself is already covered by the holder's shallow size. State that `getChildResources()` is a diagnostic accessor and not a deduplicated ownership tree; summing over it is not sum-safe in general. With each implementation reporting only its owned slice, summing `ramBytesUsed()` across `Accountable` instances becomes a defined operation: each byte is reported by at most one holder, so the sum is meaningful without a deduplication layer. The convention text in the companion PR makes three precision points explicit: `ramBytesUsed()` reports owned heap bytes (mmap regions and direct buffers are out of scope); you own what you `close()`, regardless of who allocated it; a clone reports the bytes it allocated for its own iteration state, not the master's.

2. **Revisit individual implementations only as follow-up.** Once the convention is settled, classes that diverge can be evaluated case-by-case in separate PRs, weighing local design intent against the shared rule. This issue does not propose specific changes; the examples above are illustrative.

## Backward compatibility

The proposal does not change the `Accountable` interface contract or signature. Fixes to specific classes will change the numbers those classes report, and ordering matters: undercounting is operationally more dangerous than overcounting because it lets capacity planners and circuit breakers assume headroom that does not exist. The codebase already undercounts on average since many classes do not implement `Accountable`, so adding the interface to classes that report nothing today is always safe. The real risk is migrating a class that currently includes borrowed state before that state's owning `Accountable` exists, which turns a known overcount into a fresh undercount; such migration should wait.

`Accountable.getChildResources()` is unchanged. Existing patterns where a parent aggregates child `ramBytesUsed()` while also exposing those children via `getChildResources()` (`DocumentsWriterPerThread`, `IndexingChain`, similar tests) continue to work as today.

## Related issues

- [LUCENE-8855](https://issues.apache.org/jira/browse/LUCENE-8855) (2019, fixed in 8.2): "Add Accountable to some Query implementations". Direct prior evidence that the same ambiguity this issue addresses already shaped API decisions: Adrien Grand declined to add `Accountable` to `BytesRef` and `IntsRef` because they can represent slices of arrays and the contract did not say whether borrowed deep content should be counted. The community also chose `UNKNOWN_DEFAULT_RAM_BYTES_USED = 256` to overcount unknown query types, codifying the operational asymmetry this issue's Backward compatibility section names.
- [apache/lucene#15097](https://github.com/apache/lucene/issues/15097) (fixed by [#15124](https://github.com/apache/lucene/pull/15124)): "Incorrect size estimation of BooleanQuery in LRUQueryCache causing heap exhaustion". Operational evidence that query size estimation affects cache budgets in production. The fix replaced a 1024 byte default with `RamUsageEstimator.sizeOf(query)`, a generic overcounting walker, consistent with the LUCENE-8855 preference; `BooleanQuery` still does not implement `Accountable`.

Contributor guide

Open the contributing guide

Research direction

Start at the Accountable interface javadoc and review the ownership examples in GlobalOrdinalsQuery, GlobalOrdinalsWithScoreQuery, and Lucene99HnswVectorsWriter.FieldWriter. Done means the contract explicitly defines owned heap bytes, treatment of borrowed or shared state, and the diagnostic-only nature of getChildResources(), without changing individual implementations.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend-api-design, performance
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.