Box error values in Results

[jimblandy]

Use String in variants of naga::front::wgsl::Error, not Box<str>.

Change the WGSL front end to always return Error values boxed, so that the size of Error does not affect the performance of success paths. This reduces the minimum size of Results in the WGSL front end from 48 bytes to 8.

Delete test naga::front::wgsl::error::test_error_size. Since Error is always returned boxed, its size should have no effect on Result size.

[jimblandy]

Edit: I take it all back - this change is actually needed for #6833.

That PR introduces Error variants for ill-typed calls to builtin functions that point out which argument is ill-typed, what type was given, and what types would have been allowed. These variants are larger than 48 bytes, even if we use Box<str> instead of String.

In other words, if we want to generate good diagnostics, shaving off a few capacity fields here and there isn't enough to keep your [edit] errors Results small. You really do have to completely decouple the size of Error from the size of Result.

Original comment:

This was a bit stubborn of me. I'm happy to listen to the preferences of active Naga contributors as to whether this is actually desirable or not. My rationale:

• This is better than the current solution: the minimum size of Result is now 8, not 48.

• Rather than fielding test_error_size failures from time to time, we can do whatever we want with Error; it doesn't have any effect if an error isn't raised.

• String is much more common than Box<str>, so I think it's less distracting for contributors and maintainers.

• This is arguably simpler than the current solution, since we don't need to write into here and there. But only arguably: in exchange, we must use Box::new when returning an error.

[ErichDonGubler]

Just one issue before approving (as a conversation item against the diff).

praise: It does seem like the move to a Box is important, esp. if your motivation is to avoid creating very large errors in your work in #6443.

nitpick: I'm not sure that the change from Box<str> is well enough motivated to convince me that we can accept memory pressure for the sake of simplicity when we are considering issues like #5295. Aggregate size of Errors can still matter once we move to an architecture that gathers errors instead of necessarily changing control flow, and might not return before gathering quite a lot of them. I'd prefer to break this change out to something separate, and maybe decouple it from your current commit stack.

Still, we don't have benchmarks governing this, so I don't see it as a blocking issue, regardless of how I feel.

---

_{I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂}

[jimblandy]

Aggregate size of Errors can still matter once we move to an architecture that gathers errors instead of necessarily changing control flow

Using String in Error instead of Box<str> changed the size of Error from 48 bytes to 64 bytes. That's less than a single line of text. Is this really a concern?

[jimblandy]

praise: It does seem like the move to a Box is important, esp. if your motivation is to avoid creating very large errors in your work in #6443.

Thank you! But that is not my motivation. Out of habit, I used the summary of my last commit as the title of the PR, but the current title does not properly explain why this change is needed.

The problem is that large error types make for large result types, which affect the performance of every function call that uses them, regardless of whether or not an error occurs. I would actually like to be free to build quite large error types.

[jimblandy]

Okay, the original PR, #6146, was concerned with another angle I hadn't considered, which was stack usage. However, that PR claims that boxing reduced performance by 2%, which I find hard to believe.

[jimblandy]

Here's what I'm seeing from this PR:

wgpu$ cargo bench -p wgpu-benchmark wgsl-in
   Compiling naga v24.0.0 (/home/jimb/rust/wgpu/naga)
   Compiling wgpu-hal v24.0.0 (/home/jimb/rust/wgpu/wgpu-hal)
   Compiling wgpu-core-deps-windows-linux-android v24.0.0 (/home/jimb/rust/wgpu/wgpu-core/platform-deps/windows-linux-android)
   Compiling wgpu-core v24.0.0 (/home/jimb/rust/wgpu/wgpu-core)
   Compiling wgpu v24.0.0 (/home/jimb/rust/wgpu/wgpu)
   Compiling wgpu-benchmark v24.0.0 (/home/jimb/rust/wgpu/benches)
    Finished `bench` profile [optimized + debuginfo] target(s) in 33.16s
     Running benches/root.rs (target/release/deps/wgpu_benchmark-055283810d844a17)
front/shader: wgsl-in   time:   [0.0000 ps 0.0000 ps 0.0000 ps]
                        thrpt:  [0.0000   B/s 0.0000   B/s 0.0000   B/s]
                 change:
                        time:   [-48.422% -0.9497% +95.919%] (p = 0.99 > 0.05)
                        thrpt:  [-48.958% +0.9588% +93.882%]
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

This doesn't seem like a blocking concern.

[jimblandy]

I certainly want to see Naga producing multiple errors per compilation, like any respectable compiler.

But I think we should agree that once we've produced, say, 1000 error messages, we've gone far beyond anything that would be valuable to a user. We probably want to cap the number of errors we produce before giving up at around 100 or so.

That suggests that individual error records could be tens of kilobytes in size and still only use a megabyte, which is hardly prohibitive. (After all, if Naga had succeeded in generating output, we would have passed the shader along to the GPU driver's compiler...)

So I'm just not convinced that the size of Error, in the neighborhood we're in today, is a concern for the multi-error case.

[jimblandy]

I measured how much stack space the too_many_unclosed_loops test uses on an x86_64 machine using Rust 1.85.

• Without this PR:

  • test profile: 1035kB
  • release profile: 70kB

• With this PR:

  • test profile: 611kB
  • release profile: 57kB

So this PR is better at addressing stack size concerns than the PR that replaced String with Box<str>.

[ErichDonGubler]

What I mean by "large errors" is "large errors on the stack". So, I think we're in violent agreement there. 😉

[ErichDonGubler]

I think the discussion that was valuable for this PR has been had, and I don't see any blockers.