Skip to content

refactor(profiling): drop bytecode-lib dependency from _asyncio.py - #17995

Closed
vlad-scherbich wants to merge 19 commits into
mainfrom
vlad/prof-replace-bytecode-dependency
Closed

refactor(profiling): drop bytecode-lib dependency from _asyncio.py#17995
vlad-scherbich wants to merge 19 commits into
mainfrom
vlad/prof-replace-bytecode-dependency

Conversation

@vlad-scherbich

@vlad-scherbich vlad-scherbich commented May 10, 2026

Copy link
Copy Markdown
Contributor

Description

ddtrace/profiling/_asyncio.py previously hooked into asyncio internals via ddtrace.internal.wrapping.wrap, which rewrites the target's __code__ using the third-party bytecode library.

This PR replaces that with a small in-module _wrap() helper that clones a template trampoline's __code__ via CodeType.replace() and grafts it onto the original function. It is the same identity-preserving trick the previous bytecode.wrap() used, but without the dependency!

Why

  • We plan to remove the bytecode dependency during migration to v3.15 (@gab is working on the tracing-side implementation), because it blocks the entire minor version release until it become compatible
  • All 9 wrap sites in this file target plain non-async, non-generator callables. They do not need bytecode-level wrapping.
  • Eliminate a direct dependency on a fragile external lib that has to be upgraded to a new CPython version, making as dependent on its release schedule.

Scope

This PR affects profiling consumers only. ddtrace.internal.wrapping.wrap itself is unchanged — the tracing path still genuinely needs bytecode for async/generator function wrapping with stack-effect preservation. The other (legitimate) bytecode consumers (ddtrace/debugging/_expressions.py, ddtrace/internal/assembly.py, ddtrace/internal/bytecode_injection/__init__.py) are untouched.

Testing

  • New behavioral test suite at tests/profiling/collector/test_asyncio_wrapping.py (16 tests).
  • Passes on main and branch

Performance

TL;DR:

  • this branch adds very little overhead, and it is basically buried in the noise between bencmark runs

Microbenchmarked on Python 3.14.3 / macOS arm64.
Three-way:

  • unwrapped baseline
  • main (bytecode wrap)
  • branch

N = 12 trials per variant; each trial:

  • create_task: 200,000 iterations per timing
  • gather3 / shield: 2,000 iterations per timing
op baseline mean main mean branch mean branch − main 95 % CI verdict
create_task 673 ns 799 ns 724 ns −75 ns [−162, +13] ns indistinguishable
gather3 76.3 µs 78.9 µs 77.5 µs −1.4 µs [−5.3, +2.5] µs indistinguishable
shield 72.2 µs 78.0 µs 73.9 µs −4.2 µs [−9.3, +1.0] µs indistinguishable

Risks

  • Unwrapping not implemented: as before, the wrappers are installed for the lifetime of the process; nothing in the existing code unwraps them. This is the same as with current bytecode-dependency implementation.

@vlad-scherbich vlad-scherbich added changelog/no-changelog A changelog entry is not required for this PR. Profiling Continous Profling labels May 10, 2026
@datadog-prod-us1-3

This comment has been minimized.

@pr-commenter

pr-commenter Bot commented May 10, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-05-10 20:21:06

Comparing candidate commit a982199 in PR branch vlad/prof-replace-bytecode-dependency with baseline commit d1a7113 in branch main.

Found 0 performance improvements and 5 performance regressions! Performance is the same for 592 metrics, 4 unstable metrics.

scenario:iast_aspects-re_expand_aspect

  • 🟥 execution_time [+257.868µs; +287.952µs] or [+7.645%; +8.537%]

scenario:iastaspects-lower_aspect

  • 🟥 execution_time [+57.183µs; +63.855µs] or [+19.453%; +21.723%]

scenario:iastaspectsospath-ospathbasename_aspect

  • 🟥 execution_time [+90.950µs; +99.087µs] or [+22.394%; +24.398%]

scenario:span-start

  • 🟥 execution_time [+1.141ms; +1.313ms] or [+7.307%; +8.412%]

scenario:telemetryaddmetric-1-count-metric-1-times

  • 🟥 execution_time [+199.065ns; +233.654ns] or [+9.545%; +11.204%]

@vlad-scherbich
vlad-scherbich force-pushed the vlad/prof-replace-bytecode-dependency branch from a982199 to 87bd294 Compare May 10, 2026 20:27
@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented May 10, 2026

Copy link
Copy Markdown

Codeowners resolved as

ddtrace/profiling/_asyncio.py                                           @DataDog/profiling-python
ddtrace/profiling/_wrap.py                                              @DataDog/profiling-python

@vlad-scherbich

Copy link
Copy Markdown
Contributor Author

@hatgpt-codex-connector please review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the asyncio profiling integration to avoid ddtrace.internal.wrapping.wrap() (and its third-party bytecode dependency) by introducing a local _wrap() helper that uses functools.wraps + setattr, including explicit alias mirroring for asyncio.* / asyncio.tasks.* bindings.

Changes:

  • Replaced bytecode-based wrapping in ddtrace/profiling/_asyncio.py with an in-module _wrap() helper and added alias mirroring where needed.
  • Added a comprehensive new subprocess-based test suite covering alias identity, metadata preservation, callback firing, and argument/kwarg substitution behaviors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
ddtrace/profiling/_asyncio.py Introduces _wrap() and migrates asyncio/uvloop hook points off bytecode-based wrapping, with alias mirroring for selected APIs.
tests/profiling/collector/test_asyncio_wrapping.py Adds subprocess-isolated behavioral tests validating wrapping invariants and callback firing across asyncio APIs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ddtrace/profiling/_asyncio.py Outdated
Comment thread tests/profiling/collector/test_asyncio_wrapping.py Outdated
Comment thread tests/profiling/collector/test_asyncio_wrapping.py Outdated
@vlad-scherbich vlad-scherbich changed the title Vlad/prof replace bytecode dependency refactor(profiling): remove bytecode dependency by implementing a custom wrapper May 11, 2026
@vlad-scherbich
vlad-scherbich force-pushed the vlad/prof-replace-bytecode-dependency branch 2 times, most recently from 02c593b to 9133537 Compare May 11, 2026 19:33
@vlad-scherbich
vlad-scherbich requested a review from Copilot May 11, 2026 19:37
@vlad-scherbich

Copy link
Copy Markdown
Contributor Author

@chatgpt-codex-connector please review

@vlad-scherbich vlad-scherbich changed the title refactor(profiling): remove bytecode dependency by implementing a custom wrapper chore(profiling): remove bytecode dependency by implementing a custom wrapper May 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread ddtrace/profiling/_asyncio.py Outdated
Comment thread tests/profiling/collector/_asyncio_wrap_helpers.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9133537072

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread ddtrace/profiling/_asyncio.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@vlad-scherbich
vlad-scherbich force-pushed the vlad/prof-replace-bytecode-dependency branch from 2e404db to 348bb66 Compare May 11, 2026 20:48
@vlad-scherbich vlad-scherbich changed the title chore(profiling): remove bytecode dependency by implementing a custom wrapper refactor(profiling): drop bytecode-lib dependency from _asyncio.py May 11, 2026
@vlad-scherbich
vlad-scherbich marked this pull request as ready for review May 11, 2026 21:29
@vlad-scherbich
vlad-scherbich requested a review from a team as a code owner May 11, 2026 21:29
@vlad-scherbich
vlad-scherbich requested a review from taegyunkim May 11, 2026 21:29
@vlad-scherbich
vlad-scherbich force-pushed the vlad/prof-replace-bytecode-dependency branch from ebb1828 to 7211d79 Compare May 12, 2026 02:00
Comment thread ddtrace/profiling/_asyncio.py Outdated
Comment thread ddtrace/profiling/_asyncio.py Outdated
return await wrapper(original_copy, args, kwargs)


def _wrap(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we need all this extra machinery here, instead of simply defining a wrapper function, e.g.:

original_tasks_wait = asyncio.tasks._wait

def tasks_wait(*args, **kwargs):
     ...
     return original_tasks_wait(*args, **kwargs)

asyncio.tasks._wait = tasks_wait

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've actually tried something like that at first. it breaks for callers that captured a reference via from asyncio.tasks import _wait before the profiler started. Those references still point at the un-wrapped original.

For example: uvloop's Cython modules do exactly that, which caused all 6 uvloop CI jobs to fail. This PR does basically what the bytecode.wrap machinery does under the hood, which is mutating __code__ in place so identity is preserved.

This regression test: test_pre_cached_reference_still_triggers_callback should fail with the simple approach.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Have we considered using the wrapping context instead then? That's what the tracer would need to migrate to anyway to get rid of bytecode wrapping

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we did in a very general way, so I would need to better understand how that would work.

What are the pros / cons of "plain" wrap vs wrapping context, why is context preferable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the tracing's WrappingContext be used here as you suggest? If so, wouldn't it still depend on bytecode? Or is that the future WrappingContext that's rewritten in your sys.monitoring API? (forgive my n00b questions).

If it's actually the latter, where bytecode dependency is removed, then I would be all for adopting it.

What do you think of merging this change for profiling only, which would fully unblock testing a 3.15 demo service and proving the migration will work? And then replace this implementation with your, better, approach?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is that the future WrappingContext that's rewritten in your sys.monitoring API?

Yes. This is the wrapping machinery that is getting migrated to the monitoring API. So if we really need a more "advanced" wrapping mechanism than just a plain Python wrapper, we should consider using this if possible.

@vlad-scherbich
vlad-scherbich force-pushed the vlad/prof-replace-bytecode-dependency branch from 06190ee to 7c9f453 Compare May 12, 2026 17:18
@vlad-scherbich

Copy link
Copy Markdown
Contributor Author

Blocked on #17849, which will replace the need for custom wrapper created here.

@vlad-scherbich
vlad-scherbich marked this pull request as draft May 16, 2026 14:58
@vlad-scherbich

Copy link
Copy Markdown
Contributor Author

Closing in favor of #18389

@vlad-scherbich
vlad-scherbich deleted the vlad/prof-replace-bytecode-dependency branch July 24, 2026 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/no-changelog A changelog entry is not required for this PR. Profiling Continous Profling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants