Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Mar 10, 2025

No description provided.

@github-actions
Copy link

Failed to generate code suggestions for PR

@misrasaurabh1
Copy link
Contributor

reviewed this async - main feedback being that tracer has to not create new sqlite db for each therad. write to the single sqlite db concurrently. Sqlite should handle the locking etc, thats what it is meant for.
Also avoid modifying the profiler code as we want to minimize the difference wrt the original python profile implementation.

KRRT7 added 5 commits March 11, 2025 15:16
with the added import, and the moved func def, the coverage lines change, inspected manually
@misrasaurabh1 misrasaurabh1 merged commit 42317cf into main Mar 13, 2025
15 checks passed
@KRRT7 KRRT7 deleted the tracer_tracker branch March 13, 2025 02:13
@KRRT7
Copy link
Contributor Author

KRRT7 commented Mar 13, 2025

/review

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Robustness

Ensure that the new ThreadPoolExecutor-based test (test_threadpool) covers edge cases and that thread interleavings are sufficiently simulated.

def test_threadpool() -> None:
    pool = ThreadPoolExecutor(max_workers=3)
    args = list(range(10, 31, 10))
    result = pool.map(funcA, args)

    for r in result:
        print(r)
Multi-thread Handling

Review the enhanced multi-thread support in tracing methods (e.g. trace_dispatch_call and related functions) to ensure that frame comparisons and context switching are handled robustly in concurrent environments.

def trace_dispatch_call(self, frame: FrameType, t: int) -> int:
    """Handle call events in the profiler."""
    try:
        # In multi-threaded contexts, we need to be more careful about frame comparisons
        if self.cur and frame.f_back is not self.cur[-2]:
            # This happens when we're in a different thread
            rpt, rit, ret, rfn, rframe, rcur = self.cur

            # Only attempt to handle the frame mismatch if we have a valid rframe
            if (
                not isinstance(rframe, FakeFrame)
                and hasattr(rframe, "f_back")
                and hasattr(frame, "f_back")
                and rframe.f_back is frame.f_back
            ):
                self.trace_dispatch_return(rframe, 0)

        # Get function information
        fcode = frame.f_code
        arguments = frame.f_locals
        class_name = None
        try:
            if (
                "self" in arguments
                and hasattr(arguments["self"], "__class__")
                and hasattr(arguments["self"].__class__, "__name__")
            ):
                class_name = arguments["self"].__class__.__name__
            elif "cls" in arguments and hasattr(arguments["cls"], "__name__"):
                class_name = arguments["cls"].__name__
        except Exception:  # noqa: BLE001, S110
            pass

        fn = (fcode.co_filename, fcode.co_firstlineno, fcode.co_name, class_name)
        self.cur = (t, 0, 0, fn, frame, self.cur)
        timings = self.timings
        if fn in timings:
            cc, ns, tt, ct, callers = timings[fn]
            timings[fn] = cc, ns + 1, tt, ct, callers
        else:
            timings[fn] = 0, 0, 0, 0, {}
        return 1  # noqa: TRY300
    except Exception:  # noqa: BLE001
        # Handle any errors gracefully
        return 0
Test Expectation

Verify that the updated expected traced function count (from 3 to 5) aligns with the changes in tracer behavior and that the tests properly validate the multi-threaded tracing improvements.

if not functions_traced or int(functions_traced.group(1)) != 5:
    logging.error("Expected 5 traced functions")
    return False

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants