Skip to content

Prevent TieredPGO inlining from breaking stack trace walking logic #2620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 26, 2023

Conversation

slang25
Copy link
Contributor

@slang25 slang25 commented Jun 18, 2023

Fixes #2617

I've recreated the issue locally on macOS on my M2 MBP, this fixes the exception, I need to check that the traces have the right API names captured still.

@@ -536,6 +538,7 @@ private static bool IsPlaywrightInternalNamespace(string namespaceName)
namespaceName.StartsWith("Microsoft.Playwright.Helpers", StringComparison.InvariantCultureIgnoreCase));
}

[MethodImpl(MethodImplOptions.NoInlining)] // This method is also a stacktrace marker.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While not strictly necessary, it seems a good idea to make this explicit

@slang25 slang25 marked this pull request as draft June 18, 2023 09:13
@slang25
Copy link
Contributor Author

slang25 commented Jun 18, 2023

Unfortunately I've done some further testing and it looks like a lot of the method names captured in the trace will change to base types, which isn't desired, I'll look to improve this PR.

@slang25
Copy link
Contributor Author

slang25 commented Jun 19, 2023

Update:

So this PR fixes the NullReferenceException, but in order to get the right API name captured like this:

Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Page.GotoAsync").Count(), 1);
Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Page.SetContentAsync").Count(), 1);
Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Page.ClickAsync").Count(), 1);
Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Mouse.MoveAsync").Count(), 1);
Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Mouse.DblClickAsync").Count(), 1);
Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Keyboard.InsertTextAsync").Count(), 1);
Assert.GreaterOrEqual(events.Where(e => e?.ApiName == "Page.CloseAsync").Count(), 1);

we're going to need to sprinkle NoInlining over all of the public APIs. I've been thinking about other approaches that don't do stack frame walking, however some public APIs flow through other public APIs , for example Page.GotoAsync → Frame.GotoAsync (see here), which makes that harder to do in a non-breaking way.

@slang25 slang25 marked this pull request as ready for review June 23, 2023 22:59
@slang25
Copy link
Contributor Author

slang25 commented Jun 23, 2023

So this is now working and tested with some traces. Unfortunately the cleanest way I could think to do this was add [MethodImpl(MethodImplOptions.NoInlining)] absolutely everywhere on the public APIs of anything that inherits ChannelOwnerBase, this is enforced with a convention test.

Copy link
Member

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

Amazing work!

{
if (!method.MethodImplementationFlags.HasFlag(MethodImplAttributes.NoInlining))
{
#pragma warning disable CA1305
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to get rid of that.

@mxschmitt mxschmitt merged commit fba5017 into microsoft:main Jun 26, 2023
@mxschmitt
Copy link
Member

@slang25 thank you so much for looking into this and @AndyAyersMS for filing it inside this repo with all the details!

@slang25 slang25 deleted the fix-stacktrace-logic branch June 26, 2023 21:09
@slang25
Copy link
Contributor Author

slang25 commented Jun 27, 2023

My pleasure @mxschmitt 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Stack inspection done by playwright is fragile and breaks with Dynamic PGO enabled
3 participants