well-typed / optics

Optics as an abstract interface
374 stars 24 forks source link

Performance best practices regarding lens vs lensVL and Optics.TH? #490

Open davidgarland opened 1 year ago

davidgarland commented 1 year ago

I notice this comment in the implementation of the lens function:

-- Do not define lens in terms of lensVL, mixing profunctor-style definitions
-- with VL style implementation can lead to subpar generated code,
-- i.e. updating often gets and then sets as opposed to updating in place.

Reading this, in addition to reading the warning on the van Laarhoven encoding section saying

The van Laarhoven encoding of lenses is isomorphic to the profunctor encoding used internally by optics, but converting back and forth may have a performance penalty.

makes me think that I ought to outright avoid use of lensVL in my code.

What confuses me, though, is that I see in Optics.TH's documentation all of the generated lenses use lensVL, and using -ddump-splices confirms this is indeed what's generated.

My questions are:

(I don't consider this a massive flaw in the documentation, though some clarification on this being added might be nice-- if this repo had a "Discussion" tab added I would've asked this there instead.)

arybczak commented 1 year ago

These comments might be outdated and/or wrong.

I don't think we have any inspection tests that check how a combination of vl/profunctor lenses optimize, it would be nice to add some (and update the documentation if necessary).

arybczak commented 1 year ago

Just to clarify, I wrote the first comment and the bit Do not define lens in terms of lensVL refers to the lens function that's being defined, not lenses in general, it should be still relevant.

As for the second claim, I think it refers to converting between the VL and profunctor encoding with lensVL/toLensVL? But I'm not sure, I don't remember writing it. @adamgundry did you do it?

It's somewhat vague and might be unsubstantiated, inspection tests would be good to check.