Removing redundant folds

[riz0id]

Removes several redundant folds that were used in Message / MessageField instances:

• Changes foldMap f . Map.toList in favor of Map.foldrWithKey
• Removes identities Foldable.toList :: [(k, v)] -> [(k, v)]

[evanrelf]

Looks good, nice job 👍 Only one note:

I'm not against reformatting things when you're already in the code - e.g. the Cabal file, multi-line type signatures, etc. - but it does pollute the diff and makes it harder to review.

When you make these formatting changes in the future, I highly recommend you use multiple commits on your branch. If I can breeze through a formatting-related commit (especially if I can turn on whitespace ignore mode, and see that it's purely whitespace changes!), that means I can spend more time grokking the meat of your PR.

Even better, break the important stuff into multiple commits to "tell a story" about why you're making the change! I went and looked up the FieldNumber type and fieldNumber function in proto3-wire, but if those changes had lived in their own commit with a comment about how it was purely newtype unwrapping and a Num instance, that would've been super appreciated as a reviewer.

None of this is required, and maybe other people don't care about this stuff as much as I do. But I know as a reviewer it's super appreciated when PRs are designed to be reviewed, if that makes sense 🙂

[j6carey]

Overall looks fine, but I do have a few comments.

[j6carey]

These days proto3-suite uses the reverse builders from Proto3.Wire.Reverse in proto3-wire. By building from right to left we can just measure the actual size of a submessage just written in order to figure out what length prefix to write out.

But a reverse builder makes a left fold more efficient. Also, it is generally good for speed to use etaBuildR to delay inspection of the value being encoded until we actually have some buffer into which to write that encoding. So this would be my suggestion:

   let encodeField :: k -> v -> Encode.MessageBuilder
        encodeField k v = Encode.embedded num (encodeMessage 1 (k, v))
     in etaBuildR $ M.foldlWithKey (\acc k -> etaBuildR ((acc <>) . encodeField k)) mempty

Now, I haven't actually tried to compile that suggestion, so there might be a typo. And to be 100% sure of the optimization issues we would need to check the core generated by GHC. But that is the kind of code that usually works best with the reverse builder.

[evanrelf]

we would need to check the core generated by GHC

@j6carey Do you think it would be worth using inspection-testing in proto3-wire or proto3-suite to ensure optimizations take effect in Core?

[j6carey]

@evanrelf , that's an interesting idea, but probably should not block this PR, IMO.

[evanrelf]

I agree, should not block this PR 👍 Just wanted to mention the idea in general.

[j6carey]

As above, it is probably better to use a left fold and employ etaBuildR.

[j6carey]

This seems much more verbose, and yet it should compile to the same thing. What's the benefit of this new syntax?

[riz0id]

No benefit, I just preferred it because foldMap is so overloaded.

[j6carey]

How about foldMap @Maybe?

[riz0id]

Sure

[j6carey]

fmap of a coercion can be slower than just a coercion. Perhaps in this case it is the same speed because the Functor in question is:

newtype Parser input a = Parser { runParser :: input -> Either ParseError a }

Also, that newtype constructor is indeed imported into your module. So the compiler might be able to see that there's no need to actually pattern-match on the Either just to coerce its Right.

But still, the old code made clear that the coercion was zero-cost, and for that reason I would prefer the old code in this particular case.

[riz0id]

Ah sorry, I'll switch these back.

[riz0id]

Didn't know GHC inspects patterns when simplifying coercions, I thought it always erased them.

[j6carey]

The point is that without knowing that a Parser is, GHC must hold out the possibility that fmap coerce does more than just coercion. That is, fmap might have a side effect unrelated to the argument that you pass. (Though possibly such a side effect would violate some Functor law, that would be outside of what GHC can rely upon.)

Of course, in this case GHC does know Parser, and so this is more of a style preference to reduce the amount of thought the reader puts into it.

[j6carey]

Another thing: I've talked to some folks, and they'd prefer to keep the explicit fieldNumber, basically as documentation.