Fix parallel external source forwarding recycled output buffers by mzient · Pull Request #6413 · NVIDIA/DALI

mzient · 2026-06-30T07:35:49Z

Category:

Bug fix (non-breaking change which fixes an issue)

Description:

In parallel mode ExternalSource could forward a worker's shared-memory buffer straight to the pipeline output without copying it. The worker was then free to recycle that buffer for a later sample, silently overwriting outputs the user still held (observed as garbage or stale values).

ForwardCurrentData now reports whether it copied the data. When it only swapped the buffer in (no copy), the operator records the output range in the iteration's UnshareableData. The pipeline-output MakeContiguous runs in a new PipelineOutput mode that, before passing a contiguous input through, checks it against UnshareableData and forces a copy if the buffer is one that may be recycled.

Add a regression test that holds every output of an epoch without copying and verifies them only after the epoch has run, which fails if a held buffer was recycled.

See #6027

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

dali-automaton · 2026-06-30T07:37:39Z

CI MESSAGE: [56318520]: BUILD STARTED

greptile-apps · 2026-06-30T07:51:29Z

Greptile Summary

This PR fixes a data-corruption bug (#6027) in the executor2 parallel ExternalSource: when a worker's shared-memory buffer was swapped directly into the pipeline output without copying, the worker could later recycle that buffer, silently overwriting outputs the caller still held.

ForwardCurrentData now returns bool indicating whether a copy was made; when it only swapped (no copy), ExternalSource::RunImpl calls MakeOutputUnshareable, registering the output buffer's address range in a new per-iteration UnshareableData map.
A new PipelineOutput mode for MakeContiguous (set by graph analysis on all executor2 output nodes) checks the input against UnshareableData before deciding to pass through, forcing an independent copy when the buffer may be recycled.
A regression test verifies that all outputs of an epoch remain uncorrupted when held without copying until after the epoch completes.

Confidence Score: 4/5

The parallel-mode fix is mechanically sound, but the copy_performed semantic change in CPU ShareUserData inadvertently defeats no_copy=True for non-parallel executor2 pipelines.

Changing copy_performed from true to false in ShareUserData causes MakeOutputUnshareable to fire for every non-parallel no_copy=True CPU ExternalSource in executor2, forcing a copy and silently breaking the no_copy=True contract.

dali/pipeline/operator/builtin/input_operator.h - copy_performed change in ShareUserData and MakeOutputUnshareable helper need a mechanism to distinguish parallel-worker shared memory from user-owned no_copy buffers.

Important Files Changed

Filename	Overview
dali/pipeline/operator/builtin/input_operator.h	ForwardCurrentData now returns bool; copy_performed changed from true to false in ShareUserData — semantically correct but inadvertently triggers MakeOutputUnshareable for non-parallel no_copy=True CPU pipelines in executor2.
dali/pipeline/workspace/iteration_data.h	New UnshareableData class: interval map (end->start) for tracking unshareable memory ranges. Contains/Add correct for non-overlapping use case; Add does not perform three-way merge but is functionally correct for Contains.
dali/pipeline/operator/builtin/make_contiguous.h	New PipelineOutput mode added. SetPassthrough helper checks unshareable_data under lock and forces copy if buffer is flagged. Logic is correct.
dali/pipeline/operator/builtin/external_source.cc	Both CPU and GPU RunImpl capture bool from ForwardCurrentData and call MakeOutputUnshareable on swap. CPU path correct for parallel workers.
dali/pipeline/executor/executor2/exec_graph_analysis.cc	SetMakeContiguousMode sets PipelineOutput mode on MakeContiguous nodes feeding the output node.
dali/pipeline/executor/executor2/exec_graph.h	AddOutputNode simplified: dead name-registration code removed, replaced with duplicate-output guard.
dali/pipeline/operator/builtin/input_operator.cc	All four ForwardCurrentData specializations changed from void to bool, returning copy_performed or true when a copy was performed.
dali/test/python/test_external_source_parallel.py	New regression test holds all epoch outputs without copying and verifies them after the full epoch to catch recycled shared-memory buffers.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant W as Parallel Worker
    participant ES as ExternalSource RunImpl
    participant IQ as InputQueue shared-mem
    participant MC as MakeContiguous PipelineOutput
    participant UD as UnshareableData
    participant Out as Pipeline Output
    W->>IQ: "place buffer in shared memory copy_performed=false"
    ES->>IQ: ForwardCurrentData swap returns false
    ES->>UD: MakeOutputUnshareable buffer range
    ES->>MC: pass swapped TensorList
    MC->>UD: SetPassthrough Lock Contains raw_tensor 0
    UD-->>MC: found pass_through_ false
    MC->>Out: copy to independent buffer
    W->>IQ: recycle shared-memory buffer safe
    Out-->>Out: data intact

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant W as Parallel Worker
    participant ES as ExternalSource RunImpl
    participant IQ as InputQueue shared-mem
    participant MC as MakeContiguous PipelineOutput
    participant UD as UnshareableData
    participant Out as Pipeline Output
    W->>IQ: "place buffer in shared memory copy_performed=false"
    ES->>IQ: ForwardCurrentData swap returns false
    ES->>UD: MakeOutputUnshareable buffer range
    ES->>MC: pass swapped TensorList
    MC->>UD: SetPassthrough Lock Contains raw_tensor 0
    UD-->>MC: found pass_through_ false
    MC->>Out: copy to independent buffer
    W->>IQ: recycle shared-memory buffer safe
    Out-->>Out: data intact

Comments Outside Diff (1)

dali/pipeline/operator/builtin/input_operator.h, line 383-393 (link)

[Bug] no_copy=True copy forced for non-parallel CPU pipelines in executor2

Changing copy_performed from true to false in the CPU ShareUserData path is semantically correct (no copy was performed here), but it has an unintended side effect for non-parallel pipelines. When a user calls feed_input with no_copy=True, ShareUserData is the code path taken. After this change, ForwardCurrentData returns false, which triggers MakeOutputUnshareable in ExternalSource::RunImpl, registering the user's buffer in unshareable_data. The downstream MakeContiguous in PipelineOutput mode then finds the buffer in unshareable_data and forces a copy, silently undoing the no_copy=True contract for all executor2 pipelines — regardless of whether parallel workers are involved.

Parallel workers can recycle shared-memory buffers (the real bug); user-provided no_copy buffers cannot be recycled by a worker. A separate flag on InputQueueItem (e.g., may_be_recycled) could distinguish the two cases, allowing MakeOutputUnshareable to be skipped for user-owned buffers while still protecting parallel-worker shared memory.

_{Reviews (6): Last reviewed commit: "Bug fix." | Re-trigger Greptile}

dali-automaton · 2026-06-30T09:53:37Z

CI MESSAGE: [56318520]: BUILD FAILED

dali-automaton · 2026-06-30T10:44:22Z

CI MESSAGE: [56337382]: BUILD STARTED

mzient · 2026-06-30T10:50:23Z

-    if (!node->instance_name.empty()) {
-      if (!name2node_.emplace(node->instance_name, node).second) {
-        nodes_.pop_back();
-        throw std::invalid_argument(
-            make_string("Duplicate node name: \"", node->instance_name, "\""));
-      }
-    }


This code was dead.

dali-automaton · 2026-06-30T10:50:39Z

CI MESSAGE: [56338022]: BUILD STARTED

dali-automaton · 2026-06-30T12:02:44Z

CI MESSAGE: [56343673]: BUILD STARTED

dali-automaton · 2026-06-30T12:29:50Z

CI MESSAGE: [56346138]: BUILD STARTED

dali-automaton · 2026-06-30T12:48:49Z

CI MESSAGE: [56338022]: BUILD PASSED

dali-automaton · 2026-06-30T15:07:44Z

CI MESSAGE: [56346138]: BUILD PASSED

In parallel mode ExternalSource could forward a worker's shared-memory buffer straight to the pipeline output without copying it. The worker was then free to recycle that buffer for a later sample, silently overwriting outputs the user still held (observed as garbage or stale values). ForwardCurrentData now reports whether it copied the data. When it only swapped the buffer in (no copy), the operator records the output range in the iteration's UnshareableData. The pipeline-output MakeContiguous runs in a new PipelineOutput mode that, before passing a contiguous input through, checks it against UnshareableData and forces a copy if the buffer is one that may be recycled. Add a regression test that holds every output of an epoch without copying and verifies them only after the epoch has run, which fails if a held buffer was recycled. Tests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Fix Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2026-07-01T09:30:09Z

CI MESSAGE: [56467605]: BUILD STARTED

dali-automaton · 2026-07-01T11:13:47Z

CI MESSAGE: [56467605]: BUILD FAILED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2026-07-01T16:18:13Z

CI MESSAGE: [56504565]: BUILD STARTED

mzient · 2026-07-01T17:03:02Z

@greptileai

Changing copy_performed from true to false in ShareUserData causes MakeOutputUnshareable to fire for every non-parallel no_copy=True CPU ExternalSource in executor2, forcing a copy and silently breaking the no_copy=True contract.

The old behavior wasn't a contract - it was a bug. The old executor always performed a copy at the output; the new one sometimes avoided it, resulting in unwanted buffer recycling in case of parallel external source - and possibly whenever the user passed data with NoCopy.

greptile-apps · 2026-07-01T17:04:16Z

That's a fair correction. The old copy_performed = true in the CPU ShareUserData was factually incorrect — data was being shared, not copied — and it was only masking the issue by accidentally preventing the unshareable tag in executor2. The old executor always copied at the output stage regardless, so users of non-parallel no_copy=True CPU sources were never getting zero-copy end-to-end to begin with.

Setting copy_performed = false here accurately reflects reality and consistently delegates the output-copy decision to MakeContiguous/PipelineOutput, which is the right place for it. My original concern was overstated.

Acknowledged — the change is correct.

dali-automaton · 2026-07-01T18:05:55Z

CI MESSAGE: [56504565]: BUILD PASSED

banasraf · 2026-07-02T13:22:37Z

+      auto lock = unshareable.Lock();
+      if (!unshareable.Empty()) {
+        assert(is_contiguous);
+        if (unshareable.Contains(input.raw_tensor(0)))


Are we guarded against empty input (for .raw_tensor(0))?

Executor skips operators when all inputs have 0 samples.

It's impossible to have 0 samples at the pipeline output.

mzient mentioned this pull request Jun 30, 2026

pytorch loader convert number into nan values #6027

Closed

1 task done

greptile-apps Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread dali/pipeline/operator/builtin/make_contiguous.h Outdated

Comment thread dali/pipeline/operator/builtin/make_contiguous.h

Comment thread dali/pipeline/executor/executor2/exec_graph_analysis.cc Outdated

Comment thread dali/pipeline/workspace/iteration_data.h Outdated

dali-automaton assigned banasraf and rostan-t Jun 30, 2026

mzient force-pushed the fix_parallel_external_source_short_circuit branch from ec7fb74 to 6afb3d3 Compare June 30, 2026 10:42

mzient commented Jun 30, 2026

View reviewed changes

rostan-t approved these changes Jun 30, 2026

View reviewed changes

Comment thread dali/pipeline/operator/builtin/make_contiguous.h Outdated

mzient and others added 6 commits July 1, 2026 11:28

Fix multi-output usage. Simplify logic in graph analysis.

99c08fc

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Remove assertion which doesn't hold in tests.

6862921

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Handle empty ranges as no-op.

88a2993

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Remove unused variable.

0a8c3c4

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Make MakeContiguous::SetPassthrough private.

5794ac4

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the fix_parallel_external_source_short_circuit branch from 63c8f82 to 5794ac4 Compare July 1, 2026 09:29

greptile-apps Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread dali/pipeline/operator/builtin/input_operator.cc Outdated

Bug fix.

58b61ff

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

banasraf reviewed Jul 2, 2026

View reviewed changes

banasraf approved these changes Jul 2, 2026

View reviewed changes

mzient merged commit 653cdaf into NVIDIA:main Jul 2, 2026
7 checks passed

Conversation

mzient commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

greptile-apps Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Comments Outside Diff (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

mzient Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

Uh oh!

dali-automaton commented Jun 30, 2026

Uh oh!

dali-automaton commented Jul 1, 2026

Uh oh!

Uh oh!

dali-automaton commented Jul 1, 2026

Uh oh!

dali-automaton commented Jul 1, 2026

Uh oh!

mzient commented Jul 1, 2026

Uh oh!

greptile-apps Bot commented Jul 1, 2026

Uh oh!

dali-automaton commented Jul 1, 2026

Uh oh!

banasraf Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

mzient Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mzient commented Jun 30, 2026 •

edited

Loading

greptile-apps Bot commented Jun 30, 2026 •

edited

Loading