Skip to content

Infallible return values and more efficient out parameters #2248

Merged
kennykerr merged 3 commits intomasterfrom
ReturnValue
Dec 12, 2022
Merged

Infallible return values and more efficient out parameters #2248
kennykerr merged 3 commits intomasterfrom
ReturnValue

Conversation

@kennykerr
Copy link
Copy Markdown
Collaborator

@kennykerr kennykerr commented Dec 12, 2022 •
edited
Loading

Now that we're starting to get more accurate optional parameter metadata thanks to microsoft/win32metadata#1005 we can more accurately represent infallible return values. These are functions and COM interface methods that have a void return type in metadata but also have a trailing out parameter that may be treated as a logical return value. For example, GetLocalTime goes from:

let mut time = Default::default();
GetLocalTime(&mut time);

To this:

let time = GetLocalTime();

And ID2D1DeviceContext's GetTarget method goes from:

let mut previous = None;
target.GetTarget(&mut previous);

To this:

let previous = target.GetTarget()?;

A Result is used to allow detection of null COM interface pointer values.

I now also more carefully detect large structs and avoid transforming those to avoid large stack usage. For example, IDCompositionDevice2's GetFrameStatistics method now returns the DCOMPOSITION_FRAME_STATISTICS value as an output parameter. Any structure larger than 128 bits (the size of GUID) is passed by reference rather than by return value. This is especially useful for DirectX matrix structures.

Fixes #2246

@kennykerr kennykerr merged commit 0ee7be0 into master Dec 12, 2022
@kennykerr kennykerr deleted the ReturnValue branch December 12, 2022 21:25
@MarijnS95
Copy link
Copy Markdown
Contributor

MarijnS95 commented Jan 13, 2023 •
edited
Loading

@kennykerr we just noticed that IDXGIAdapter4::GetDesc3 went from GetDesc3() -> Result<DESC3> to GetDesc3(&mut desc) -> Result<()> in windows 0.44.0 since this PR. Now I'm not sure what the metadata contains or if the metadata is off, but https://learn.microsoft.com/en-us/windows/win32/api/dxgi1_6/nf-dxgi1_6-idxgiadapter4-getdesc3 states, for [out] pDesc:

This parameter must not be NULL

Any idea why this changed?

@kennykerr
Copy link
Copy Markdown
Collaborator Author

kennykerr commented Jan 13, 2023

I mentioned this in the PR description above. Testing this transformation revealed that it negatively affects certain methods that logically return very large structs, like DirectX matrixes, so I put a cap on the size of the structs that would be candidates for this transformation. Unfortunately, the metadata for return values is not very helpful at this point so I have to use a heuristic. The heuristic was just a little too generous before.

windows-rs/crates/libs/metadata/src/reader/mod.rs

Lines 1324 to 1349 in f5373bb

pub fn signature_param_is_retval(&self, param: &SignatureParam) -> bool {
// The Win32 metadata uses `RetValAttribute` to call out retval methods but it is employed
// very sparingly, so this heuristic is used to apply the transformation more uniformly.
if self.param_is_retval(param.def) {
return true;
}
if !param.ty.is_pointer() {
return false;
}
if param.ty.is_void() {
return false;
}
let flags = self.param_flags(param.def);
if flags.input() || !flags.output() || flags.optional() || param.kind.is_array() {
return false;
}
if self.param_kind(param.def).is_array() {
return false;
}
// If it's bigger than 128 bits, best to pass as a reference.
if self.type_size(&param.ty.deref()) > 16 {
return false;
}
// TODO: find a way to treat this like COM interface result values.
!self.type_is_callback(&param.ty.deref())
}

@MarijnS95
Copy link
Copy Markdown
Contributor

MarijnS95 commented Jan 13, 2023

Ack, DXGI_ADAPTER_DESC3 seems to be bigger than 128 bytes. Didn't notice this when originally scrubbing the description, thanks for pointing this out.

@MarijnS95
Copy link
Copy Markdown
Contributor

MarijnS95 commented Jan 18, 2023

@kennykerr in hindsight I'm not sure what kind of RVO or other optimizations Rust would perform here (also considering lack of #[inline]), but I hope the move semantics language feature doesn't come at the detriment of performance. The previous approach is definitely easier to write.

If so, the only place this might be useful is when updating fields already in a struct, though I suspect most would be constructed by moving DESC3 (and other struct values) out of a function, and into a struct constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support infallible return values

2 participants

@kennykerr @MarijnS95