GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Move observability-relevant structure fields to the top by P403n1x87 · Pull Request #105271 · python/cpython

Conversation

@P403n1x87

@P403n1x87 P403n1x87 commented Jun 3, 2023

Copy link
Copy Markdown
Contributor

Some structures have fields that are used by out-of-process tools, like Austin. Having these fields defined after some more complex structures makes it hard to maintain these tools. With this change, we move the declaration of the most useful fields to the top of the structure definition. This reduces the amount of irrelevant information that the mentioned tools have to replicate to retrieve the actually useful data

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kakkoyun reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added the awaiting review label Jun 3, 2023
@P403n1x87
P403n1x87 force-pushed the refactor/observability-fields-first branch from 945c762 to 3f41f83 Compare June 3, 2023 15:41
@P403n1x87

P403n1x87 commented Jun 3, 2023

Copy link
Copy Markdown
Contributor Author

@markshannon @pablogsal I hope you don't mind if I tag you on this PR. I was looking into the changes required to support 3.12 in Austin and thought of proposing this change to make my life (and presumably that of those who maintain similar tools) easier.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@P403n1x87
P403n1x87 force-pushed the refactor/observability-fields-first branch from 3f41f83 to 60d2b16 Compare June 3, 2023 15:43
@P403n1x87
Move observability-relevant structure fields to top
f9af502
Some structures have fields that are used by out-of-process tools, like
Austin. Having these fields defined after some more complex structures
makes it hard to maintain these tools. With this change, we move the
declaration of the most useful fields to the top of the structure
definition. This reduces the amount of irrelevant information that the
mentioned tools have to replicate to retrieve the actually useful data
@P403n1x87
P403n1x87 force-pushed the refactor/observability-fields-first branch from 60d2b16 to f9af502 Compare June 6, 2023 22:43
@pablogsal pablogsal added skip news skip issue labels Jun 8, 2023
@pablogsal

pablogsal commented Jun 8, 2023

Copy link
Copy Markdown
Member

Unfortunately this is not a bugfix so technically we cannot backport this unless @Yhg1s gives us greenlight.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pablogsal
pablogsal approved these changes Jun 8, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Jun 8, 2023
@pablogsal pablogsal changed the title Move observability-relevant structure fields to top Move observability-relevant structure fields to the top Jun 8, 2023
@pablogsal
pablogsal merged commit 410c2f1 into python:main Jun 8, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 8, 2023
@pablogsal

pablogsal commented Jun 8, 2023

Copy link
Copy Markdown
Member

@Yhg1s are you ok backporting this to 3.12?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Yhg1s

Yhg1s commented Jun 8, 2023

Copy link
Copy Markdown
Member

Hrrm, not really, partly because I'm really not sold on the usefulness of the change, and partly because I'm aware of software poking at these things so this is an ABI change. I'd rather not backport, but if you want to try and sell me on the usefulness feel free :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Jun 8, 2023

Copy link
Copy Markdown
Member

Did anyone benchmark this?

The interpreter frame is the hottest piece of memory in the VM, so care needs to be taken rearranging the fields.
The eval_breaker field is also quite hot, so moving it away from the front of the of the interpreter state may well harm performance. This PR is undoing the layout change in #104581.

The order of fields should not be part of the API or ABI (apart from not moving them during a release). They should (IMO) be arranged for performance.
In fact there is no guarantee that the fields will even continue to exist. We have added and removed fields freely in the past, and want the freedom to continue to do so.

Also, what fields are important, and too whom?
What if tool A wants field X and tool B wants field Y, then what?
The order of fields needs to be an internal implementation detail. Things are just too fragile otherwise.

If we want to expose the offset of certain fields to out-of-process tools, then we could put the offsets in a table.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Jun 8, 2023

Copy link
Copy Markdown
Member

Why is there no issue for this?
Which fields "are used by of out process tools"?
This PR doesn't make it clear what is being moved before what and why.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Jun 8, 2023

Copy link
Copy Markdown
Member

Well, as I can see that this change is highly controversial, let's revert it until we all agree.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal pablogsal mentioned this pull request Jun 8, 2023
Revert "Move observability-relevant structure fields to the top" #105512
Merged
pablogsal added a commit that referenced this pull request Jun 8, 2023
@pablogsal
Revert "Move observability-relevant structure fields to the top (#105271
72cecf1
)"

This reverts commit 410c2f1.
@pablogsal

pablogsal commented Jun 8, 2023

Copy link
Copy Markdown
Member

#105512 for the revert

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Jun 8, 2023

Copy link
Copy Markdown
Member

This has been reverted

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@P403n1x87

P403n1x87 commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

@markshannon are there specific benchmark designed to measure the performance the structures involved in this change.

Did anyone benchmark this?

I did not benchmark this, nor I have an idea of why fields are where they currently are, since there are no comments in the source code. Without that information available, I took the liberty to move them at will. I appreciate one might want to invoke the principle of locality here, but without extra information I had to take a guess.

The order of fields should not be part of the API or ABI (apart from not moving them during a release). They should (IMO) be arranged for performance.

This is not a request to make fields part of the A(B/P)I

In fact there is no guarantee that the fields will even continue to exist. We have added and removed fields freely in the past, and want the freedom to continue to do so.

It is quite likely that the most interesting fields will always be there (they might change name, as they have done in the past, but they are essentially still there).

Also, what fields are important, and too whom? What if tool A wants field X and tool B wants field Y, then what? The order of fields needs to be an internal implementation detail. Things are just too fragile otherwise.

It would be hard to take opinions out of this. As the maintainer of Austin, I have moved fields in the order that I makes the more sense to me (but hopefully to others as well). However I don't have a scientific answer to give for the ordering.

If we want to expose the offset of certain fields to out-of-process tools, then we could put the offsets in a table.

That would be great, but as far as I understand this is something that will perhaps come in the future. Here I'm looking for something that can go in 3.12.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Jun 8, 2023

Copy link
Copy Markdown
Member

That would be great, but as far as I understand this is something that will perhaps come in the future. Here I'm looking for something that can go in 3.12.

Adding a new symbol might be more acceptable than moving fields around, regarding ABI compatibility.
But that's @Yhg1s decision.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Jun 9, 2023

Copy link
Copy Markdown
Member

@P403n1x87
Want to make a list of the fields you need to access?
What's the best way to make information about offsets and the like visible to out-of-process tools?
We discussed a similar problem in #100987

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@P403n1x87

P403n1x87 commented Jun 9, 2023

Copy link
Copy Markdown
Contributor Author

@P403n1x87 Want to make a list of the fields you need to access?

The list of fields that I'm currently relying on can be inferred from https://github.com/P403n1x87/austin/blob/b48d980f6f3d0808d9221ee611a9c3094e02bb50/src/version.h#L196-L306

What's the best way to make information about offsets and the like visible to out-of-process tools? We discussed a similar problem in #100987

I've shared my thoughts in this comment #100987 (comment). I think a symbol/section like _PyRuntime pointing to an offset structure would be enough.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Jun 9, 2023

Copy link
Copy Markdown
Member

Want to make an issue for adding that info?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal pablogsal mentioned this pull request Jul 10, 2023
Add a collection of offsets to facilitate the work of out-of-process debuggers #106597
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@P403n1x87 @pablogsal @Yhg1s @markshannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.