Alloc methods should take layout by reference

[joshlf]

Currently, the methods of the Alloc trait that take a Layout take it by value. This is unnecessary, and makes clippy unhappy. Instead, they should take their Layout arguments by reference.

[nagisa]

I feel that Layout being passed by-value is very intended. It is 2-usize worth of data, which will usually be split over registers for the call and end up being more efficient than going through a reference.

[nagisa]

clippy warning about this seems like a false positive though.

[joshlf]

Fair enough. @Manishearth do you have thoughts on clippy's behavior here? Is this just one of those situations in which the right move is to silence clippy with an allow(needless_pass_by_value)?

[Manishearth]

Hmm, Layout is Copy, Clippy shouldn't be warning on Copy period. Will have a look

[joshlf]

Doesn't look like it's Copy: https://doc.rust-lang.org/src/alloc/allocator.rs.html#46

[Manishearth]

That seems incorrect?

In this case the clippy lint is working as it should; taking non-copy types by-move isn't really good. If you're doing this for perf reasons slap on an allow.

[joshlf]

Incorrect in that Layout ought to be Copy or incorrect in that I shouldn't be complaining about it if clippy is in the right? :)

[Manishearth]

Layout should be Copy.

If it's not because it's considered a hazard (much like how Range isn't Copy) then it should be passed by reference because otherwise it gets consumed each time and needs explicit cloning.

[joshlf]

Also, IIRC, the reason that it's not Copy is that somebody (I believe it was @SimonSapin ?) wanted to leave the door open to having Layouts be able to contain a chain of other Layouts from which they were derived, which would preclude them from being Copy. As I see it:

• If we go that route, then Layouts are expensive to clone, and so should be passed by reference
• If we don't go that route, then Layouts should be Copy

Layout should be Copy.

Given the rest of this comment, I tend to agree.

[Manishearth]

Yeah, that reasoning makes sense. In that case these APIs definitely should operate by-ref.

[Manishearth]

cc @gankro

[SimonSapin]

@joshlf This is the first I hear of Layouts being (possibly) chained.

(FWIW I care less about the details of the Alloc trait than I care about stabilizing it within months rather than years.)

[vitalyd]

Hmm, I don't see a compelling reason to make Layout Copy right now and carry that contract. The comment earlier about it being 2 words and thus splittable across registers can also looked at as:

• it might be a downside on register starved arch
• it might not matter if callee is inlined and compiler can shuffle the code as it sees fit

All that to say, I don't think how the optimizer is going to pass it around should be the criteria to decide on Copy (or move) or not.

[joshlf]

@SimonSapin weird, I could have sworn that I'd brought this issue up before and that was the answer. I couldn't find it by Googling, though, so maybe I was just dreaming.

Edit: Found it!. It was @pnkfelix .

[SimonSapin]

@joshlf It’s quite possible that someone said that, but it wasn’t me so I can’t tell you about reasoning and such.

[joshlf]

I found it; see my previous comment.

[SimonSapin]

I chatted with @pnkfelix about this today. It’s been several years now since the allocation APIs was considered as hypothetically enabling GC integration, but nothing concrete has happened in that space since. In particular, the idea was that users would have to go through the allocator to create a Layout, which could then contain allocator-specific data about the type being allocated.

But now we have a Layout::from_size_align constructor and it seems very unlikely that we’ll remove it, so at this point we can probably give up on that idea and make Layout: Copy. PR: #50109