Move observability-relevant structure fields to the top#105271
Conversation
945c762 to
3f41f83
Compare
June 3, 2023 15:41
|
@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. |
Sorry, something went wrong.
3f41f83 to
60d2b16
Compare
June 3, 2023 15:43
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
June 6, 2023 22:43
|
Unfortunately this is not a bugfix so technically we cannot backport this unless @Yhg1s gives us greenlight. |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Why is there no issue for this? |
Sorry, something went wrong.
|
Well, as I can see that this change is highly controversial, let's revert it until we all agree. |
Sorry, something went wrong.
)" This reverts commit 410c2f1.
|
This has been reverted |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
Adding a new symbol might be more acceptable than moving fields around, regarding ABI compatibility. |
Sorry, something went wrong.
|
@P403n1x87 |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
Want to make an issue for adding that info? |
Sorry, something went wrong.
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