Skip to content
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 data recorded with yk_promote into MTThread. #976

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Feb 16, 2024

I've slowly come to realise that calls to yk_promote could (but don't currently!) end up being in compiled in two different ways:

  1. A literal call to yk_promote_*.
  2. An inlined call to a side-band recording mechanism (e.g. PTWRITE).

Because I hadn't teased these two apart in my mind, my previous attempt had kind-of tried to merge the two together in one API. It's now clear that was entirely misguided on my part.

This commit can be seen as partly undoing my previous attempt: literal calls to yk_promote_* now record the data in MTThread which is a thread local. However, when tracing stops, the promotion data is now extracted from MTThread so it can safely be moved to another thread.

That means we can now handle #1 above: but we don't yet have an API that can handle #2.

let (thrdtrcr, promotions) = THREAD_MTTHREAD.with(|mtt| {
(
mtt.thread_tracer.take().unwrap(),
mtt.promotions.take().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

we should say why the unwraps are safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing comment on line 296 explains.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right. I thought that was referring to something else because it's singular. Maybe wack and s on the end?

(Top-notch pedantry right here!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ef0b3e1.

@vext01
Copy link
Contributor

vext01 commented Feb 16, 2024

If I understand correctly, you move the promotions from a trace recorder (local to a thread by definition) into a thread local mtthread?

At the risk of sounding like a dummy, what is the advantage of this? Just more convenient, or something more profound?

@ltratt
Copy link
Contributor Author

ltratt commented Feb 16, 2024

If I understand correctly, you move the promotions from a trace recorder (local to a thread by definition) into a thread local mtthread?

I originally thought that all tracing backends could work out when yk_promote_* was called and handle those in the same way as in-band (e.g. PTWRITE) data. It's now clear to me that this is possible for some backends (e.g. swt) but not others (hwt). Thus there has to be a generic outside-the-tracer mechanism for handling yk_promote_*.

@vext01
Copy link
Contributor

vext01 commented Feb 16, 2024

OK

I've slowly come to realise that calls to `yk_promote` could (but don't
currently!) end up being in compiled in two different ways:

  1. A literal call to `yk_promote_*`.
  2. An inlined call to a side-band recording mechanism (e.g.
     `PTWRITE`).

Because I hadn't teased these two apart in my mind, my previous attempt
had kind-of tried to merge the two together in one API. It's now clear
that was entirely misguided on my part.

This commit can be seen as partly undoing my previous attempt: literal
calls to `yk_promote_*` now record the data in `MTThread` which is a
thread local. However, when tracing stops, the promotion data is now
extracted from `MTThread` so it can safely be moved to another thread.

That means we can now handle ykjit#1 above: but we don't yet have an API that
can handle ykjit#2.
@vext01
Copy link
Contributor

vext01 commented Feb 16, 2024

Please squash.

@ltratt ltratt force-pushed the move_yk_promote_to_mtthread branch from ef0b3e1 to 7eff562 Compare February 16, 2024 14:00
@ltratt
Copy link
Contributor Author

ltratt commented Feb 16, 2024

Squashed.

@vext01 vext01 added this pull request to the merge queue Feb 16, 2024
Merged via the queue into ykjit:master with commit 96dae12 Feb 16, 2024
2 checks passed
@ltratt ltratt mentioned this pull request Feb 17, 2024
@ltratt ltratt deleted the move_yk_promote_to_mtthread branch March 30, 2024 09:13
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.

2 participants