-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Move observability-relevant structure fields to the top #105271
Move observability-relevant structure fields to the top #105271
Conversation
945c762
to
3f41f83
Compare
@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. |
3f41f83
to
60d2b16
Compare
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
60d2b16
to
f9af502
Compare
Unfortunately this is not a bugfix so technically we cannot backport this unless @Yhg1s gives us greenlight. |
@Yhg1s are you ok backporting this to 3.12? |
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 :) |
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 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. Also, what fields are important, and too whom? If we want to expose the offset of certain fields to out-of-process tools, then we could put the offsets in a table. |
Why is there no issue for this? |
Well, as I can see that this change is highly controversial, let's revert it until we all agree. |
)" This reverts commit 410c2f1.
#105512 for the revert |
This has been reverted |
@markshannon are there specific benchmark designed to measure the performance the structures involved in this change.
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.
This is not a request to make fields part of the A(B/P)I
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).
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.
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. |
@P403n1x87 |
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
I've shared my thoughts in this comment #100987 (comment). I think a symbol/section like |
Want to make an issue for adding that info? |
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