Stream: git-wasmtime

Topic: wasmtime / issue #5493 Add `clif-util compile` option to ...

Wasmtime GitHub notifications bot (Dec 30 2022 at 01:32):

jameysharp commented on issue #5493:

I think this is a great idea! A bunch of people are out due to holidays, but if nobody else gets to it first I intend to review this next week. Feel free to comment again if you don't hear anything by the end of next week.

Wasmtime GitHub notifications bot (Jan 04 2023 at 02:08):

jameysharp commented on issue #5493:

I got partway through reviewing this today. Overall I think it's in good shape! A tip for anybody else who takes a look: turn on the "ignore whitespace changes" option when reviewing this diff.

I'm not entirely satisfied with the TargetIsa::cloned addition. All the TargetIsa methods take immutable borrows, so it should be safe to just pass borrows everywhere. Unfortunately the Module::isa trait method doesn't have a lifetime parameter so there isn't an immediately obvious way to make it stop requiring its own boxed copy of the TargetIsa. Still, I would be happier with fixing the Module trait somehow than with adding TargetIsa::cloned, if we can come up with a reasonable way to do it.

One option I think might work is a method like Module::with_isa(&self, f: impl FnOnce(&dyn TargetIsa)) that calls the given function with the borrowed TargetIsa, so that the borrow must be dropped before the callback returns. Then I don't think the trait or its callers care what the actual lifetime of the object is. That's a little more than I like for a public API change though so it's not great either. We don't make API stability promises about Cranelift but we do try not to make changes without good reasons, at least.

I want to think about this a little more, but I'm generally in favor of merging this PR, with or without changes. Thanks for proposing it!

Wasmtime GitHub notifications bot (Jan 04 2023 at 09:51):

bjorn3 commented on issue #5493:

Maybe we could give out Arc<dyn TargetIsa> instead of Box<dyn TargetIsa>?

Wasmtime GitHub notifications bot (Jan 05 2023 at 19:59):

samsartor commented on issue #5493:

I'm pushing another version of this where ObjectModule borrows the ISA, just so you can see what it looks like. Overall it works out very cleanly. But I personally liked the cloned ISA more, since:

1. lifetime params on structs can have wind up having unforeseen ergonomic costs
2. I can not imagine any situation where ISA flags are not trivially cloneable
3. the clone has negligible performance cost since it is far outside any tight loops
4. changing the Box to a borrow or Arc is a breaking change for anyone using the ObjectModule interface

Still, the cloned method was just a gross workaround for dyn Module + Clone not being a thing yet, and I'm not shedding any tears dropping it from this PR.

If you want me to revert or go with the Arc instead, just say the word.

Wasmtime GitHub notifications bot (Jan 05 2023 at 20:07):

samsartor commented on issue #5493:

Maybe we could give out Arc<dyn TargetIsa> instead of Box<dyn TargetIsa>?

I liked this in principal, but it infects a _lot_ of files. To be consistent we'd want to change most uses of &dyn TargetIsa to Arc<dyn TargetIsa>, and there are a lot of those. The nice thing about cloned was it left all the borrows of TargetIsa in place and let us upgrade at will.

Wasmtime GitHub notifications bot (Jan 05 2023 at 21:01):

bjorn3 commented on issue #5493:

lifetime params on structs can have wind up having unforeseen ergonomic costs

Yeah, cg_clif needs to pass ObjectModule to another thread, which needs it to be 'static.

Wasmtime GitHub notifications bot (Jan 05 2023 at 21:15):

jameysharp commented on issue #5493:

I'm not going to argue too vigorously about this, but if we can find a way to avoid cloning the TargetIsas that everyone's happy with, I think it'd be nice.

Barring that:

@samsartor wrote:

Still, the cloned method was just a gross workaround for dyn Module + Clone not being a thing yet, and I'm not shedding any tears dropping it from this PR.

I guess another alternative is to define trait TargetIsa: Clone, so that deriving Clone on each *Backend struct is sufficient, right?

I'm pushing another version of this where ObjectModule borrows the ISA, just so you can see what it looks like. Overall it works out very cleanly. But I personally liked the cloned ISA more, since:

1. lifetime params on structs can have wind up having unforeseen ergonomic costs

I totally agree with this point, although it looks like it might be okay in this case.

2. I can not imagine any situation where ISA flags are not trivially cloneable

I think the flags and Target are cheap to clone, but I'm not so sure about the MachineEnv: it has five vectors in it, and I'm not sure how big those are in practice. Usually proportional to the number of registers on the target machine I think, which doesn't sound too bad, but I'm not super familiar with that part.

3. the clone has negligible performance cost since it is far outside any tight loops
4. changing the Box to a borrow or Arc is a breaking change for anyone using the ObjectModule interface

Both fair points!

@bjorn3 wrote:

lifetime params on structs can have wind up having unforeseen ergonomic costs

Yeah, cg_clif needs to pass ObjectModule to another thread, which needs it to be 'static.

That's not that big of a deal though, is it? You can build a Box<dyn TargetIsa> and then call Box::leak on it to get a 'static borrow.

Wasmtime GitHub notifications bot (Jan 05 2023 at 21:50):

bjorn3 commented on issue #5493:

Box::leak leaks memory. This would make it unsuitable for running multiple compiler instances in the same process and would trip memory leak checkers.

guess another alternative is to define trait TargetIsa: Clone, so that deriving Clone on each *Backend struct is sufficient, right?

Clone is unfortunately not object safe.

Wasmtime GitHub notifications bot (Jan 05 2023 at 21:51):

bjorn3 commented on issue #5493:

I liked this in principal, but it infects a lot of files. To be consistent we'd want to change most uses of &dyn TargetIsa to Arc<dyn TargetIsa>, and there are a lot of those. The nice thing about cloned was it left all the borrows of TargetIsa in place and let us upgrade at will.

I don't think it is necessary to change most &dyn TargetIsa to Arc<dyn TargetIsa>. Most places won't clone it and as such wouldn't have any benefit, only the cost of requiring an extra clone.

Wasmtime GitHub notifications bot (Jan 05 2023 at 22:59):

samsartor commented on issue #5493:

Yeah, cg_clif needs to pass ObjectModule to another thread, which needs it to be 'static.

This sounds serious enough to me that I'll revert the lifetime parameter for now.

I guess another alternative is to define trait TargetIsa: Clone, so that deriving Clone on each *Backend struct is sufficient, right?

I originally did this, but the trait object still needs its own method and at that point adding the cloned requirement felt like more of a breaking change. TBH I doing this and then providing a default cloned method would have been better.

I don't think it is necessary to change most &dyn TargetIsa to Arc<dyn TargetIsa>. Most places won't clone it and as such wouldn't have any benefit, only the cost of requiring an extra clone.

if we can find a way to avoid cloning the TargetIsas that everyone's happy with, I think it'd be nice

What do you think of the Arc compromise I just pushed?

Also, should I squash my commits before you merge? Not sure what the policy is here.

Wasmtime GitHub notifications bot (Jan 05 2023 at 23:01):

jameysharp commented on issue #5493:

I'm reviewing the Arc version now, thanks!

We do squash-merges on PRs, so don't worry about cleaning up commit history.

Wasmtime GitHub notifications bot (Jan 05 2023 at 23:02):

samsartor edited a comment on issue #5493:

Yeah, cg_clif needs to pass ObjectModule to another thread, which needs it to be 'static.

This sounds serious enough to me that I'll revert the lifetime parameter for now.

I guess another alternative is to define trait TargetIsa: Clone, so that deriving Clone on each *Backend struct is sufficient, right?

Like @bjorn3 said, Clone itself doesn't help, but it would let the cloned method be default implemented. I considered it originally but didn't do that way for some reason. If you don't like the Arc solution, we might go back and revisit that.

I don't think it is necessary to change most &dyn TargetIsa to Arc<dyn TargetIsa>. Most places won't clone it and as such wouldn't have any benefit, only the cost of requiring an extra clone.

if we can find a way to avoid cloning the TargetIsas that everyone's happy with, I think it'd be nice

What do you think of the Arc compromise I just pushed?

Also, should I squash my commits before you merge? Not sure what the policy is here.

Wasmtime GitHub notifications bot (Jan 05 2023 at 23:02):

samsartor edited a comment on issue #5493:

Yeah, cg_clif needs to pass ObjectModule to another thread, which needs it to be 'static.

This sounds serious enough to me that I'll revert the lifetime parameter for now.

I guess another alternative is to define trait TargetIsa: Clone, so that deriving Clone on each *Backend struct is sufficient, right?

Like @bjorn3 said, Clone itself doesn't help, but it would let the cloned method be default implemented. I considered it originally but didn't do that way for some reason. If you don't like the Arc solution, we might go back and revisit that.

I don't think it is necessary to change most &dyn TargetIsa to Arc<dyn TargetIsa>. Most places won't clone it and as such wouldn't have any benefit, only the cost of requiring an extra clone.

if we can find a way to avoid cloning the TargetIsas that everyone's happy with, I think it'd be nice

What do you think of the Arc compromise I just pushed? It is a much bigger diff but most of the changes are just Box -> Arc.

Also, should I squash my commits before you merge? Not sure what the policy is here.

Wasmtime GitHub notifications bot (Jan 06 2023 at 00:00):

samsartor commented on issue #5493:

@jameysharp No worries. TBH I've missed this kinda code review stuff since I left industry :D

Last updated: Oct 23 2024 at 20:03 UTC