Th optimization #1014
Merged
Th optimization #1014
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR moves the implementation of to/fromPersistValue for entities to be mostly a single function call to a helper function in the persistent-template codebase.
I believe this is better for a few reasons:
1. It provides a modest performance improvement, based on the benchmarks + the general principle that compiling more code takes longer. I was sort of worried this kind of small improvement wouldn't help, but it seems noticeable, especially on the high model count code. (See below for benchmark data)
2. It makes the code in persistent-template much easier to read. Before there were separate calls in Template Haskell to create functions like `fpv`, `<=<` and `getPersistMap`, which then were combined together with an InfixE constructor. With the new version, the implementation is in regular Haskell which makes it much easier to read and debug.
3. (Minor) It makes the generated code nicer. Here's what the generated code looks like as a comparison:
```
instance PersistField Person where
toPersistValue
= \ ent_asIe
-> (PersistMap
$ (zip ((map pack) ["name", "age", "foo", "address"]))
(map toPersistValue $ toPersistFields ent_asIe))
fromPersistValue
= ((\ x_asIf
-> let
columns_asIg
= unordered-containers-0.2.10.0:Data.HashMap.Strict.Base.fromList
x_asIf
in
(fromPersistValues
$ (map
(\ name_asIh
-> case
(unordered-containers-0.2.10.0:Data.HashMap.Base.lookup
(pack name_asIh))
columns_asIg
of
Just v_asIi -> v_asIi
Nothing -> PersistNull)
$ ["name", "age", "foo", "address"])))
Control.Monad.<=< getPersistMap)
```
```
instance PersistField Person where
toPersistValue = Database.Persist.TH.entityToPersistValueHelper
fromPersistValue
= Database.Persist.TH.entityFromPersistValueHelper
["name", "age", "foo", "address"]
```
This readability improvement is multiplied given this code is repeated for each model.
<hr>
Benchmark data:
Master:
```
Benchmark persistent-th-bench: RUNNING...
benchmarking mkPersist/From File
time 564.7 μs (520.9 μs .. 600.5 μs)
0.954 R² (0.926 R² .. 0.973 R²)
mean 741.2 μs (661.2 μs .. 869.9 μs)
std dev 354.3 μs (261.8 μs .. 468.2 μs)
variance introduced by outliers: 99% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/1x10
time 3.811 ms (3.731 ms .. 3.883 ms)
0.994 R² (0.990 R² .. 0.997 R²)
mean 4.101 ms (3.995 ms .. 4.281 ms)
std dev 412.5 μs (316.1 μs .. 520.1 μs)
variance introduced by outliers: 63% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/10x10
time 4.383 ms (4.157 ms .. 4.582 ms)
0.980 R² (0.966 R² .. 0.991 R²)
mean 4.212 ms (4.104 ms .. 4.357 ms)
std dev 403.8 μs (321.6 μs .. 484.2 μs)
variance introduced by outliers: 61% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/100x10
time 70.67 ms (58.60 ms .. 81.89 ms)
0.932 R² (0.815 R² .. 0.988 R²)
mean 67.00 ms (60.89 ms .. 73.02 ms)
std dev 10.48 ms (8.125 ms .. 13.51 ms)
variance introduced by outliers: 53% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/1000x10
time 589.8 ms (386.9 ms .. 883.8 ms)
0.964 R² (0.952 R² .. 1.000 R²)
mean 591.7 ms (505.3 ms .. 643.3 ms)
std dev 85.73 ms (35.06 ms .. 115.4 ms)
variance introduced by outliers: 24% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x1
time 1.003 ms (895.0 μs .. 1.114 ms)
0.921 R² (0.876 R² .. 0.973 R²)
mean 938.0 μs (896.5 μs .. 1.012 ms)
std dev 177.6 μs (120.9 μs .. 259.8 μs)
variance introduced by outliers: 91% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x10
time 4.736 ms (4.352 ms .. 5.099 ms)
0.936 R² (0.891 R² .. 0.969 R²)
mean 4.912 ms (4.679 ms .. 5.193 ms)
std dev 794.2 μs (657.8 μs .. 1.052 ms)
variance introduced by outliers: 82% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x100
time 40.23 ms (38.66 ms .. 41.56 ms)
0.996 R² (0.993 R² .. 0.999 R²)
mean 41.79 ms (40.13 ms .. 43.95 ms)
std dev 3.834 ms (1.897 ms .. 6.677 ms)
variance introduced by outliers: 32% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x1000
time 413.6 ms (332.0 ms .. 597.9 ms)
0.976 R² (0.961 R² .. 1.000 R²)
mean 402.5 ms (346.1 ms .. 436.2 ms)
std dev 56.18 ms (25.25 ms .. 77.80 ms)
variance introduced by outliers: 24% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/20x10
time 8.615 ms (8.396 ms .. 8.858 ms)
0.996 R² (0.994 R² .. 0.998 R²)
mean 9.083 ms (8.881 ms .. 9.486 ms)
std dev 848.0 μs (372.2 μs .. 1.410 ms)
variance introduced by outliers: 52% (severely inflated)
benchmarking mkPersist/Nullable/Increasing model count/40x10
time 21.07 ms (20.23 ms .. 22.30 ms)
0.989 R² (0.978 R² .. 0.996 R²)
mean 21.86 ms (21.11 ms .. 22.44 ms)
std dev 1.437 ms (1.065 ms .. 2.157 ms)
variance introduced by outliers: 27% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/60x10
time 34.78 ms (34.02 ms .. 35.36 ms)
0.999 R² (0.998 R² .. 1.000 R²)
mean 34.60 ms (33.86 ms .. 35.09 ms)
std dev 1.204 ms (714.8 μs .. 1.963 ms)
variance introduced by outliers: 11% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/80x10
time 46.61 ms (45.44 ms .. 47.33 ms)
0.998 R² (0.996 R² .. 1.000 R²)
mean 46.22 ms (44.07 ms .. 47.19 ms)
std dev 2.782 ms (1.090 ms .. 5.138 ms)
variance introduced by outliers: 20% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/100x10
time 58.42 ms (56.42 ms .. 60.54 ms)
0.997 R² (0.993 R² .. 0.999 R²)
mean 58.34 ms (55.54 ms .. 60.29 ms)
std dev 4.332 ms (2.119 ms .. 6.925 ms)
variance introduced by outliers: 24% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/1000x10
time 571.5 ms (402.9 ms .. 848.2 ms)
0.968 R² (0.955 R² .. 1.000 R²)
mean 566.9 ms (484.0 ms .. 615.2 ms)
std dev 82.00 ms (28.47 ms .. 111.5 ms)
variance introduced by outliers: 24% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x20
time 7.389 ms (7.204 ms .. 7.573 ms)
0.996 R² (0.993 R² .. 0.998 R²)
mean 7.733 ms (7.582 ms .. 8.151 ms)
std dev 639.3 μs (284.0 μs .. 1.246 ms)
variance introduced by outliers: 48% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x40
time 16.43 ms (15.05 ms .. 17.60 ms)
0.979 R² (0.963 R² .. 0.992 R²)
mean 17.00 ms (16.42 ms .. 17.58 ms)
std dev 1.404 ms (1.162 ms .. 1.762 ms)
variance introduced by outliers: 38% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x60
time 27.13 ms (26.37 ms .. 27.64 ms)
0.998 R² (0.997 R² .. 0.999 R²)
mean 27.45 ms (27.16 ms .. 27.93 ms)
std dev 758.1 μs (490.4 μs .. 1.220 ms)
benchmarking mkPersist/Nullable/Increasing field count/10x80
time 36.40 ms (35.54 ms .. 37.34 ms)
0.998 R² (0.996 R² .. 0.999 R²)
mean 36.44 ms (35.76 ms .. 37.73 ms)
std dev 1.706 ms (1.042 ms .. 2.850 ms)
variance introduced by outliers: 12% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x100
time 45.77 ms (44.63 ms .. 46.82 ms)
0.998 R² (0.996 R² .. 0.999 R²)
mean 45.19 ms (43.52 ms .. 46.35 ms)
std dev 2.715 ms (1.329 ms .. 4.449 ms)
variance introduced by outliers: 20% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x1000
time 449.2 ms (342.7 ms .. 656.1 ms)
0.973 R² (0.960 R² .. 1.000 R²)
mean 457.6 ms (403.9 ms .. 498.8 ms)
std dev 55.63 ms (28.60 ms .. 76.37 ms)
variance introduced by outliers: 23% (moderately inflated)
Benchmark persistent-th-bench: FINISH
Completed 2 action(s).
```
This branch:
```
Benchmark persistent-th-bench: RUNNING...
benchmarking mkPersist/From File
time 496.6 μs (486.2 μs .. 505.8 μs)
0.976 R² (0.954 R² .. 0.989 R²)
mean 620.9 μs (569.1 μs .. 694.4 μs)
std dev 214.9 μs (162.8 μs .. 265.6 μs)
variance introduced by outliers: 97% (severely inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/1x10
time 3.306 ms (3.246 ms .. 3.356 ms)
0.994 R² (0.987 R² .. 0.998 R²)
mean 3.459 ms (3.395 ms .. 3.542 ms)
std dev 238.4 μs (181.7 μs .. 323.9 μs)
variance introduced by outliers: 45% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/10x10
time 3.341 ms (3.281 ms .. 3.419 ms)
0.995 R² (0.992 R² .. 0.998 R²)
mean 3.366 ms (3.324 ms .. 3.431 ms)
std dev 172.3 μs (111.3 μs .. 263.5 μs)
variance introduced by outliers: 32% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/100x10
time 48.26 ms (47.03 ms .. 49.44 ms)
0.998 R² (0.995 R² .. 0.999 R²)
mean 48.08 ms (45.73 ms .. 49.33 ms)
std dev 3.307 ms (1.561 ms .. 5.603 ms)
variance introduced by outliers: 22% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing model count/1000x10
time 513.0 ms (374.7 ms .. 585.4 ms)
0.991 R² (0.980 R² .. 1.000 R²)
mean 473.3 ms (401.8 ms .. 502.0 ms)
std dev 49.87 ms (11.48 ms .. 66.27 ms)
variance introduced by outliers: 23% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x1
time 663.1 μs (657.3 μs .. 669.2 μs)
0.999 R² (0.999 R² .. 1.000 R²)
mean 667.2 μs (662.5 μs .. 672.7 μs)
std dev 18.05 μs (14.66 μs .. 26.59 μs)
variance introduced by outliers: 18% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x10
time 3.307 ms (3.263 ms .. 3.375 ms)
0.999 R² (0.997 R² .. 1.000 R²)
mean 3.283 ms (3.255 ms .. 3.334 ms)
std dev 118.4 μs (73.30 μs .. 213.9 μs)
variance introduced by outliers: 20% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x100
time 35.59 ms (35.11 ms .. 36.15 ms)
0.999 R² (0.998 R² .. 1.000 R²)
mean 35.47 ms (34.77 ms .. 36.02 ms)
std dev 1.289 ms (685.1 μs .. 2.081 ms)
variance introduced by outliers: 11% (moderately inflated)
benchmarking mkPersist/Non-Null Fields/Increasing field count/10x1000
time 420.5 ms (399.9 ms .. 439.1 ms)
1.000 R² (0.999 R² .. 1.000 R²)
mean 358.0 ms (313.4 ms .. 381.8 ms)
std dev 42.77 ms (6.404 ms .. 55.13 ms)
variance introduced by outliers: 23% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/20x10
time 7.743 ms (7.568 ms .. 7.906 ms)
0.995 R² (0.992 R² .. 0.997 R²)
mean 8.500 ms (8.227 ms .. 8.951 ms)
std dev 1.006 ms (702.7 μs .. 1.458 ms)
variance introduced by outliers: 65% (severely inflated)
benchmarking mkPersist/Nullable/Increasing model count/40x10
time 19.59 ms (19.22 ms .. 19.90 ms)
0.998 R² (0.996 R² .. 0.999 R²)
mean 20.11 ms (19.81 ms .. 20.49 ms)
std dev 808.4 μs (538.2 μs .. 1.264 ms)
variance introduced by outliers: 13% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/60x10
time 30.77 ms (30.04 ms .. 31.58 ms)
0.998 R² (0.996 R² .. 0.999 R²)
mean 31.54 ms (31.11 ms .. 31.98 ms)
std dev 959.4 μs (714.1 μs .. 1.262 ms)
benchmarking mkPersist/Nullable/Increasing model count/80x10
time 41.57 ms (40.49 ms .. 42.73 ms)
0.998 R² (0.996 R² .. 0.999 R²)
mean 41.72 ms (40.05 ms .. 42.60 ms)
std dev 2.434 ms (1.011 ms .. 4.337 ms)
variance introduced by outliers: 20% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/100x10
time 52.92 ms (51.38 ms .. 54.01 ms)
0.999 R² (0.998 R² .. 1.000 R²)
mean 52.40 ms (49.44 ms .. 53.38 ms)
std dev 2.829 ms (783.1 μs .. 4.937 ms)
variance introduced by outliers: 14% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing model count/1000x10
time 563.3 ms (461.8 ms .. NaN s)
0.996 R² (0.992 R² .. 1.000 R²)
mean 510.5 ms (457.6 ms .. 539.6 ms)
std dev 52.01 ms (265.1 μs .. 67.31 ms)
variance introduced by outliers: 23% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x20
time 6.763 ms (6.542 ms .. 6.979 ms)
0.985 R² (0.972 R² .. 0.993 R²)
mean 7.352 ms (7.150 ms .. 7.616 ms)
std dev 680.2 μs (546.2 μs .. 827.9 μs)
variance introduced by outliers: 53% (severely inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x40
time 15.96 ms (15.22 ms .. 16.70 ms)
0.988 R² (0.975 R² .. 0.995 R²)
mean 16.11 ms (15.62 ms .. 16.51 ms)
std dev 1.133 ms (911.5 μs .. 1.536 ms)
variance introduced by outliers: 32% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x60
time 24.38 ms (23.88 ms .. 24.75 ms)
0.999 R² (0.997 R² .. 0.999 R²)
mean 24.41 ms (24.13 ms .. 24.79 ms)
std dev 745.1 μs (531.5 μs .. 1.159 ms)
benchmarking mkPersist/Nullable/Increasing field count/10x80
time 31.90 ms (31.33 ms .. 32.48 ms)
0.999 R² (0.998 R² .. 1.000 R²)
mean 32.32 ms (31.95 ms .. 32.87 ms)
std dev 909.3 μs (587.1 μs .. 1.379 ms)
benchmarking mkPersist/Nullable/Increasing field count/10x100
time 40.17 ms (39.28 ms .. 41.15 ms)
0.999 R² (0.998 R² .. 1.000 R²)
mean 40.24 ms (38.96 ms .. 40.88 ms)
std dev 1.851 ms (859.9 μs .. 3.122 ms)
variance introduced by outliers: 12% (moderately inflated)
benchmarking mkPersist/Nullable/Increasing field count/10x1000
time 478.9 ms (467.2 ms .. 497.3 ms)
1.000 R² (1.000 R² .. 1.000 R²)
mean 404.4 ms (329.9 ms .. 432.6 ms)
std dev 51.08 ms (8.929 ms .. 65.08 ms)
variance introduced by outliers: 23% (moderately inflated)
Benchmark persistent-th-bench: FINISH
Completed 2 action(s).
```
I was thinking this PR is kinda small and I could do some other small optimization PRs before doing a larger release, but we could also release this as a patch release if we want.
MaxGabriel
force-pushed
the
thOptimization
branch
from
ecb8644
to
96842db
Compare
MaxGabriel
commented
Jan 11, 2020
| entityToPersistValueHelper :: (PersistEntity record) => record -> PersistValue | ||
| entityToPersistValueHelper entity = PersistMap $ zip columnNames fieldsAsPersistValues | ||
| where | ||
| columnNames = map (unHaskellName . fieldHaskell) (entityFields (entityDef (Just entity))) |
There was a problem hiding this comment.
I don't know why there is a Monad constraint on the entityDef function. It looks like the workaround for this elsewhere in the codebase is to wrap entity in a Just.
There was a problem hiding this comment.
It's an artifact of a time before Proxy, I believe - the idea is that you can write Nothing :: Maybe Person to signal the type, or return undefined or similar.
MaxGabriel
commented
Jan 11, 2020
| fieldsAsPersistValues = map toPersistValue $ toPersistFields entity | ||
|
|
||
| entityFromPersistValueHelper :: (PersistEntity record) | ||
| => [String] -- ^ Column names, as '[String]' to avoid extra calls to "pack" in the generated code |
There was a problem hiding this comment.
I don't know if this is a real optimization tbh. The previous code was using String already, maybe for this reason or maybe for no reason? It avoids "pack" * numberOfFieldNames is all
parsonsmatt
approved these changes
Jan 13, 2020
| @@ -1,5 +1,7 @@ | |||
| ## Unreleased changes | |||
|
|
|||
| * Small optimization/code cleanup to generated Template Haskell code size, by slimming the implementation of to/fromPersistValue for Entities. [#1014](https://github.com/yesodweb/persistent/pull/1014) | |||
There was a problem hiding this comment.
I'd be happy to release this as 2.8.0.1.
Suggested change
| * Small optimization/code cleanup to generated Template Haskell code size, by slimming the implementation of to/fromPersistValue for Entities. [#1014](https://github.com/yesodweb/persistent/pull/1014) | |
| ## 2.8.0.1 | |
| * Small optimization/code cleanup to generated Template Haskell code size, by slimming the implementation of to/fromPersistValue for Entities. [#1014](https://github.com/yesodweb/persistent/pull/1014) |
There was a problem hiding this comment.
Ok, I think that's a good idea too. My other optimization effort failed, and I don't currently have more ideas
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Slight code size optimization of to/fromPersistValue for entities
This PR moves the implementation of to/fromPersistValue for entities to be mostly a single function call to a helper function in the persistent-template codebase.
I believe this is better for a few reasons:
fpv,<=<andgetPersistMap, which then were combined together with an InfixE constructor. With the new version, the implementation is in regular Haskell which makes it much easier to read and debug.This readability improvement is multiplied given this code is repeated for each model.
Benchmark data:
(an easy way to compare is to select the benchmark name, Command E, Command G to switch back and forth between master/branch)
Master:
This branch:
I was thinking this PR is kinda small and I could do some other small optimization PRs before doing a larger release, but we could also release this as a patch release if we want.
Before submitting your PR, check that you've:
(n/a) on all counts
@sincedeclarations to the HaddockAfter submitting your PR: