-
Notifications
You must be signed in to change notification settings - Fork 48
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
Eliminate the use of error
in Generics.SOP.GGP
#116
Comments
I'm really not sure if changing |
It's important to note that the use of |
I think that for |
I'm afraid I don't understand what you mean here.
I agree that it shouldn't be unreasonably slow! If you check the benchmark results that I provide in the original issue description, you'll notice that my redesign gives results comparable to the current design, so that shouldn't be a concern. |
Side note about benchmarks, maybe https://hackage.haskell.org/package/criterion-compare should be extended to produce textual tables, as looking at two long criterion outputs (which are not side-by-side) and trying to compare is not job for a human |
So in short the proposal is to do
|
Can we close this given that #119 is merged? Or are there additional insights in this conversation that should be extracted into their own issue? |
Yes, let's close this now that #119 has landed. |
While I managed to get rid of most uses of
error
in #77, there is one use oferror
inGenerics.SOP.GGP
that I couldn't remove:generics-sop/generics-sop/src/Generics/SOP/GGP.hs
Lines 210 to 211 in 240a163
This is rather bothersome, since if you look carefully at how
gSumFrom
is implemented, thaterror
argument can never be reached. Surely there must be a way to refactorgSumFrom
to avoid this use oferror
!After some pondering, I realized that the reason this
error
currently exists is due to the design ofGSumFrom
:generics-sop/generics-sop/src/Generics/SOP/GGP.hs
Lines 138 to 141 in 240a163
gSumFrom
always requires anSOP I xss
argument, which makes it awkward to invoke fromgfrom
, as it requiresxss ~ '[]
. I realized that we can avoid thisSOP I xss
argument entirely by splitting upGSumFrom
into smaller parts. Here is the design I am envisioning:Some observations about this redesign:
gSumFrom
now has the much simpler typea x -> SOP I (ToSumCode a '[])
. This allowsgfrom
to eliminate theerror
argument entirely.gSumSkip
now lives in its ownGSumPad
class. There only need to be two instances forGSumSkip
:M1 C c a
and(a :+: b)
, sincegSumSkip
is only ever used to append elements to the right of aCode
.The
GSumFrom (a :+: b)
instance has changed slightly. TheR1
case still usesgSumPad
to append elements to the right of theCode
. However, theL1
case uses a differentGSumPad
class instead to prepend elements to the left of theCode
. TheGSumPad
class makes use of the(++)
type family, which is being proposed in append and split NP #115.The
GSumPad (M1 C c a)
is implemented using thepad_SOP
function, which in turn uses thepad_NS
function:If you squint at the definition of
pad_NS
, you'll notice that it simply walks over the structure of theNS
argument. This means thatpad_NS
can be implemented in a more efficient way:This is very similar to how existing functions like
coerce_NS
have both a fastunsafeCoerce
-using implementation and a safe, internal-only implementation.You might wonder if this refactoring changes the performance characteristics of
Generics.SOP.GGP
. I ran thegenerics-sop
benchmarks before and after these changes to see how theGGP
-related benchmarks were affected. Thankfully, the changes appear to be mostly negligible. Here are the benchmarks results before these changes:And after these changes:
The text was updated successfully, but these errors were encountered: