Reduce size of errors.

[emilio]

This improves performance significantly without costing too much in error reporting ability.

Reviewed upstream in https://phabricator.services.mozilla.com/D320989

This is of course a breaking change, but it also allows most parsing routines to have simpler signatures.

[nicoburns]

Do you have a link to or a summary of the performance numbers you're getting for this change? I'm in agreement with dshin that this is a bit of a shame. On the other hand this is undoubtedly a hot path that merits optimization.

---

I don't intend to block this change on this, but while on this topic: the try_parse() API always seemed not-quite-right to me. For exactly this reason: that we end up constructing (rather awkwardly) a bunch of token errors that we then end up throwing away.

It seems likely to me that there would be a way to refactor that API in such a way that doesn't require that, although I haven't put in the thought to validate that or work out what that might me.

[emilio]

I have a build linked from https://bugzilla.mozilla.org/show_bug.cgi?id=2065622 which shaves ~10% in that benchmark, of course that's not just this change, but this change is a significant amount.

Here is a comparison for just this change tho, which shows significant, high-confidence speedometer wins.

Tangent, because I think the above (plus the nicer API shape in general) is enough to justify this, but the per-error location tracking never seemed particularly useful to me?

Apart of it being overkill for a lot of the thrown-away errors, it's also just... not good? I.e., if you look at the changes in that phabricator revision, most of them end up better because they now have the whole declaration context.

I would imagine that for IDE use cases you could want the precise span, but also, maybe not? Underlining the declaration or the declaration value seems probably fine to me.

[SimonSapin]

I agree this feels unfortunate, but we’ve known for a while that error types with large size_of hurt performance

I was tempted to suggest boxing errors instead, but with the try_parse pattern there are plenty of "errors" that end up unused

[SimonSapin]

This is now the only field of this struct. I guess keeping the struct instead of using the enum directly makes migration easier?

And similarly for the ParseError struct below

[emilio]

Yeah, pretty much.

[SimonSapin]

This is of course a breaking change

Regarding releasing: I made a semver-incompatible version bump in #442 but haven’t released it yet, so you don’t need to make another one when you’re ready to make a release

[tiaanl]

For color at least this is totally fine as error reporting during parsing is usually yes or no without reason.