[Performance] Improve performance of compiled ReplayBuffer

[kurtamohler]

Description

Performance improvements for compiled ReplayBuffer.extend/sample with LazyTensorStorage. For the cases tested in the benchmark, compiled gives the same or better performance than eager.

This uses the changes from #2426 to avoid using multiprocessing features, but also adds some other changes to avoid graph breaks in other places.

The benchmark and test are updated to include both cases where the storage's max size is reached and not reached.

Motivation and Context

Part of #2501

• I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds core functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (update in the documentation)
• Example (update in the folder of examples)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

• I have read the CONTRIBUTION guide (required)
• My change requires a change to the documentation.
• I have updated the tests accordingly (required for a bug fix or a new feature).
• I have updated the documentation accordingly.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/2529

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 3 New Failures, 5 Unrelated Failures

As of commit b44d8f0 with merge base edbf3de ():

NEW FAILURES - The following jobs have failed:

• Continuous Benchmark (PR) / CPU Pytest benchmark (gh)
  test_cql_speed[False-None] 34,257.6030 (>1000.0) 41,447.0390 (>1000.0) 36,076.9334 (>1000.0) 2,046.7982 (>1000.0) 35,152.8870 (>1000.0) 2,408.0992 (>1000.0) 5;2 27.7185 (0.00) 29 1
• Continuous Benchmark (PR) / GPU Pytest benchmark (gh)
  ##[error]Workflow failed! Resource not accessible by integration
• Habitat Tests on Linux / tests (3.9, 12.1) / linux-job (gh)
  RuntimeError: Command docker exec -t a021d39b5e6bcb254530270ae0baa4f363fa6e14981668e34d448ba76861d5e9 /exec failed with exit code 1

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• Build Windows Wheels / pytorch/rl / upload / wheel-py3_9-cpu (gh) (trunk failure)
  ##[error]Unable to find any artifacts for the associated workflow
• Build Windows Wheels / pytorch/rl / upload / wheel-py3_9-cuda11_8 (gh) (trunk failure)
  ##[error]Unable to find any artifacts for the associated workflow
• Build Windows Wheels / pytorch/rl / upload / wheel-py3_9-cuda12_1 (gh) (trunk failure)
  ##[error]Unable to find any artifacts for the associated workflow
• Build Windows Wheels / pytorch/rl / upload / wheel-py3_9-cuda12_4 (gh) (trunk failure)
  ##[error]Unable to find any artifacts for the associated workflow
• Unit-tests on Windows / unittests-cpu / windows-job (gh) (trunk failure)
  ##[error]Process completed with exit code 1.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[kurtamohler]

I haven't been able to figure this one out yet, unfortunately. Even if I remove the len(storage) and use self._len_value directly, it still puts a guard around _len_value. I even tried completely removing any code that seems special about _len_value (the property decorator and anything related to mp.Value), and it still puts a guard around it.

I can't seem to find anything in the output from TORCH_TRACE that tells exactly why there's a guard on it. I've attached a trace of running the benchmarks (python benchmarks/test_replaybuffer_benchmark.py -k test_rb_extend_sample) after commenting out the @torch._dynamo.disable().

tl_out.zip

[vmoens]

that's ok, let's deactivate like you do here.
Perhaps we could isolate this and put a test around that graph break to make sure that if the compiler ends up hadling this properly we're aware of it and we can remove the disable() decorator?

[kurtamohler]

Good idea, I've added a test for that

[vmoens]

LGTM thanks a mil.
Just a couple of quick fixes here and there and we're good to go.
Do you think we should add a benchmark for the E2E replay buffer interactions?
Another next step is to make sure compile works with MemoryMappedTensor from tensordict - I'm pretty sure it won't like it

[vmoens]

that's ok, let's deactivate like you do here.
Perhaps we could isolate this and put a test around that graph break to make sure that if the compiler ends up hadling this properly we're aware of it and we can remove the disable() decorator?

[kurtamohler]

Do you think we should add a benchmark for the E2E replay buffer interactions?

Yeah I think that's a good idea, but I thought the benchmark I added was exercising the end-to-end interface, so I'm not entirely sure what you mean. Do you have a specific case in mind? Maybe something that actually uses the data for something other than just expand/sample?