Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
davidgarland opened this issue May 21, 2023 · 2 comments
Open

Comments

@davidgarland
Copy link

davidgarland commented May 21, 2023

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:

  • Is there a good reason for the use of lensVL in Optics.TH?
  • Is there likely a performance penalty for mixing Optics.TH's lensVL definitions with custom ones that were defined using lens?
    • Does this performance penalty go away if I were to write custom lenses in terms of lensVL instead? (If so, why do the docs seem to implicitly discourage use of lensVL?)

(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
Copy link
Collaborator

arybczak commented May 22, 2023

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
Copy link
Collaborator

arybczak commented May 22, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants