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

Rework field errors #479

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sfriedel
Copy link
Contributor

Working on #476 I ran into an issue where error propagation would not work anymore. Digging deeper into it I realized that the cause was the copying of the executionContext because after the copy an append to the Errors inside the executionContext would not have any effect anymore.

This PR is an attempt to fix this by removing executionContext.Errors and instead returning the errors.

Since there is not a single list of errors any more but several small lists which are then concatenated I was expecting some decreased performance and increased allocations. Suprisingly though, running the benchmarks yields the opposite result:

$ benchcmp bench-master.txt bench.txt

benchmark                       old ns/op     new ns/op     delta
BenchmarkListQuery_1-8          130990        129563        -1.09%
BenchmarkListQuery_100-8        728213        675093        -7.29%
BenchmarkListQuery_1K-8         6067942       5647696       -6.93%
BenchmarkListQuery_10K-8        58260527      54346503      -6.72%
BenchmarkListQuery_100K-8       396411927     365213308     -7.87%
BenchmarkWideQuery_1_1-8        86148         85807         -0.40%
BenchmarkWideQuery_10_1-8       214063        213150        -0.43%
BenchmarkWideQuery_100_1-8      1370126       1357655       -0.91%
BenchmarkWideQuery_1K_1-8       12830726      12707064      -0.96%
BenchmarkWideQuery_1_10-8       111114        106337        -4.30%
BenchmarkWideQuery_10_10-8      353785        340916        -3.64%
BenchmarkWideQuery_100_10-8     2652377       2558949       -3.52%
BenchmarkWideQuery_1K_10-8      26060706      25021311      -3.99%

benchmark                       old allocs     new allocs     delta
BenchmarkListQuery_1-8          994            988            -0.60%
BenchmarkListQuery_100-8        7098           6597           -7.06%
BenchmarkListQuery_1K-8         64189          59186          -7.79%
BenchmarkListQuery_10K-8        637983         587972         -7.84%
BenchmarkListQuery_100K-8       4178587        3850871        -7.84%
BenchmarkWideQuery_1_1-8        644            641            -0.47%
BenchmarkWideQuery_10_1-8       1687           1675           -0.71%
BenchmarkWideQuery_100_1-8      12047          11945          -0.85%
BenchmarkWideQuery_1K_1-8       115374         114372         -0.87%
BenchmarkWideQuery_1_10-8       846            825            -2.48%
BenchmarkWideQuery_10_10-8      2906           2795           -3.82%
BenchmarkWideQuery_100_10-8     23683          22672          -4.27%
BenchmarkWideQuery_1K_10-8      230419         220405         -4.35%

benchmark                       old bytes     new bytes     delta
BenchmarkListQuery_1-8          55344         54063         -2.31%
BenchmarkListQuery_100-8        630859        528174        -16.28%
BenchmarkListQuery_1K-8         5870254       4845625       -17.45%
BenchmarkListQuery_10K-8        58509580      48267034      -17.51%
BenchmarkListQuery_100K-8       383504922     316387856     -17.50%
BenchmarkWideQuery_1_1-8        37823         37214         -1.61%
BenchmarkWideQuery_10_1-8       91374         88751         -2.87%
BenchmarkWideQuery_100_1-8      630969        608102        -3.62%
BenchmarkWideQuery_1K_1-8       6167118       5942913       -3.64%
BenchmarkWideQuery_1_10-8       57365         53588         -6.58%
BenchmarkWideQuery_10_10-8      203667        179724        -11.76%
BenchmarkWideQuery_100_10-8     1716480       1490952       -13.14%
BenchmarkWideQuery_1K_10-8      17916231      15673802      -12.52%

Benchmarks results were produced by multiple runs of go test -run=none -bench=. -benchmem and taking the best result. I was seeing some variability in the bench results (~2-3s in the total runtime) so maybe someone can verify those results.

@Fontinalis @chris-ramon What do you think of this approach?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 92.267% when pulling 3636cef on civist:rework-field-errors into 199d20b on graphql-go:master.

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

Successfully merging this pull request may close these issues.

2 participants