Rework `TracedAOTBlock` enum. #978

ltratt · 2024-02-17T09:53:25Z

This PR reworks (and renames) the TracedAOTBlock enum. My initial aim was to make it plausible for this to record in-band promotion (e.g . PTWRITE) but as I looked in more depth I realised there were more things we could improve, including extra documentation (63589b9) and improving static guarantees (cb56456).

We will soon be recording not just "blocks", so rename the enum to something a little less inaccurate. This is a mechanical renaming with no effect on behaviour.

This is a mechanical renaming with no effect on behaviour.

This reflects the change of the enum element.

This is a mechanical renaming with no effect on behaviour.

This reflects the change of the enum element.

We don't use this currently, but we know that one day we might, so forcing us to think about this now will make it more likely that adding it later will be relatively simple.

These methods undermine the type safety of enums, turning what should be static checks into run-time panics.

vext01 · 2024-02-19T10:04:38Z

ykrt/src/compile/jitc_llvm.rs

@@ -79,7 +79,7 @@ impl JITCLLVM {
        Arc::new(JITCLLVM)
    }

-    fn encode_trace(&self, irtrace: &Vec<TracedAOTBlock>) -> (Vec<*const i8>, Vec<usize>, usize) {


ProcessedItem is such a generic name.

At least with TracedAOTBlock we know the granularity of what we are working with (blocks).

Honestly, I'd rather keep the old name and just know that when you trace an AOT block it can include promotions, although I'm guessing that you will object.

OK, I see that later ProcessedItem will become an enum. I this case I agree that TracedAOTBlock is no longer a good name, but I still think ProcessedItem is not a good name.

How about one of the following:

AOTObservation

AOTTraceEvent

AOTEvent

TraceEvent

Or maybe call the enum TraceAction or similar, and give the variants verb names, like ExecuteBlock and ObservePromotion?

The enum can't have "AOT" in the name. TraceAction could work with Block and Promotion? [Execute and in particular Observe seem like padding to me.]

I think that would be more descriptive than ProcessedItem, so if you are happy, so am I :)

a92f1fe changes the enum name (I think the enum variant names are probably OK for now as-is). If you do like this, it'll have to stay as a separate commit as rebasing this earlier will be a nightmare. [In other words: this PR could be merged as-is if -- and only if! -- you like the PR.]

vext01 · 2024-02-19T13:23:13Z

This LGTM.

Ready for merge I think then?

ltratt · 2024-02-19T22:11:22Z

Yes merge as-is please.

vext01 · 2024-02-20T10:08:51Z

10:00:37 warning: the item `libc` is imported redundantly
10:00:37  --> ykaddr/src/addr.rs:5:12
10:00:37   |
10:00:37 5 | use libc::{self, c_void, dlsym, Dl_info, RTLD_DEFAULT};
10:00:37   |            ^^^^ the item `libc` is already defined here
10:00:37   |
10:00:37   = note: `#[warn(unused_imports)]` on by default
10:00:37 
10:00:37 warning: the item `From` is imported redundantly
10:00:37  --> ykaddr/src/addr.rs:7:15
10:00:37   |
10:00:37 7 |     convert::{From, TryFrom},
10:00:37   |               ^^^^
10:00:37  --> /rustc/3246e79513cb89ddbfc0f21cb5a877e5b321dcc5/library/std/src/prelude/mod.rs:129:13
10:00:37   |
10:00:37   = note: the item `From` is already defined here
10:00:37 
10:00:37 warning: the item `TryFrom` is imported redundantly
10:00:37  --> ykaddr/src/addr.rs:7:21
10:00:37   |
10:00:37 7 |     convert::{From, TryFrom},
10:00:37   |                     ^^^^^^^
10:00:37  --> /rustc/3246e79513cb89ddbfc0f21cb5a877e5b321dcc5/library/std/src/prelude/mod.rs:129:13
10:00:37   |
10:00:37   = note: the item `TryFrom` is already defined here
10:00:37 
10:00:37 warning: the item `memmap2` is imported redundantly
10:00:37  --> ykaddr/src/obj.rs:9:5
10:00:37   |
10:00:37 9 | use memmap2;
10:00:37   |     ^^^^^^^ the item `memmap2` is already defined here
10:00:37 
10:00:37 warning: the item `phdrs` is imported redundantly
10:00:37   --> ykaddr/src/obj.rs:10:5
10:00:37    |
10:00:37 10 | use phdrs;
10:00:37    |     ^^^^^ the item `phdrs` is already defined here
10:00:37 
10:00:37 warning: `ykaddr` (lib) generated 5 warnings
...

@ptersilie this is what you were seeing yesterday?

I think maybe the import semantics changed and/or rustc now reports more unused imports than before? Is it just a matter of killing the imports?

ptersilie · 2024-02-20T10:18:24Z

Yes these are the same errors I got on my PR #975. I couldn't reproduce them locally but maybe that's because my rust version isn't up to date. Will try again with a newer rust.

ptersilie · 2024-02-20T11:29:28Z

Can confirm that after updating rust this fails on my local machine too.

vext01 · 2024-02-20T11:35:43Z

OK, so someone should raise a PR that fixes this in isolation. Any takers?

ptersilie · 2024-02-20T11:36:13Z

Working on it.

ptersilie · 2024-02-20T12:00:29Z

https://github.com/ykjit/yk/pull/979/files should fix this.

ltratt · 2024-02-20T18:45:36Z

Can someone kick this one?

ltratt added 8 commits February 17, 2024 08:28

Rename TracedAOTBlock to ProcessedItem.

a66575a

We will soon be recording not just "blocks", so rename the enum to something a little less inaccurate. This is a mechanical renaming with no effect on behaviour.

Rename Unmappable to UnmappableBlock.

8f911d4

This is a mechanical renaming with no effect on behaviour.

Rename new_unmappable to new_unmappable_block.

a3b98bc

This reflects the change of the enum element.

Rename Mapped to MappedAOTBlock.

8abaebf

This is a mechanical renaming with no effect on behaviour.

Rename new_mapped to new_mapped_aot_block.

7d131fa

This reflects the change of the enum element.

Document what the stages of tracing are.

63589b9

Add Promotion variant.

3cf3584

We don't use this currently, but we know that one day we might, so forcing us to think about this now will make it more likely that adding it later will be relatively simple.

Remove methods which only work for some enum variants.

cb56456

These methods undermine the type safety of enums, turning what should be static checks into run-time panics.

ltratt assigned vext01 Feb 17, 2024

vext01 reviewed Feb 19, 2024

View reviewed changes

Rename ProcessedItem to TraceAction.

a92f1fe

vext01 added this pull request to the merge queue Feb 20, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 20, 2024

vext01 added this pull request to the merge queue Feb 20, 2024

Merged via the queue into ykjit:master with commit df8e373 Feb 20, 2024
2 checks passed

ltratt deleted the rework_trace_enum branch March 30, 2024 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework `TracedAOTBlock` enum. #978

Rework `TracedAOTBlock` enum. #978

ltratt commented Feb 17, 2024

vext01 Feb 19, 2024

vext01 Feb 19, 2024 •

edited

Loading

vext01 Feb 19, 2024

ltratt Feb 19, 2024

vext01 Feb 19, 2024

ltratt Feb 19, 2024

vext01 commented Feb 19, 2024

ltratt commented Feb 19, 2024

vext01 commented Feb 20, 2024

ptersilie commented Feb 20, 2024

ptersilie commented Feb 20, 2024

vext01 commented Feb 20, 2024

ptersilie commented Feb 20, 2024

ptersilie commented Feb 20, 2024

ltratt commented Feb 20, 2024

Rework TracedAOTBlock enum. #978

Rework TracedAOTBlock enum. #978

Conversation

ltratt commented Feb 17, 2024

vext01 Feb 19, 2024

Choose a reason for hiding this comment

vext01 Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

vext01 Feb 19, 2024

Choose a reason for hiding this comment

ltratt Feb 19, 2024

Choose a reason for hiding this comment

vext01 Feb 19, 2024

Choose a reason for hiding this comment

ltratt Feb 19, 2024

Choose a reason for hiding this comment

vext01 commented Feb 19, 2024

ltratt commented Feb 19, 2024

vext01 commented Feb 20, 2024

ptersilie commented Feb 20, 2024

ptersilie commented Feb 20, 2024

vext01 commented Feb 20, 2024

ptersilie commented Feb 20, 2024

ptersilie commented Feb 20, 2024

ltratt commented Feb 20, 2024

Rework `TracedAOTBlock` enum. #978

Rework `TracedAOTBlock` enum. #978

vext01 Feb 19, 2024 •

edited

Loading