Skip to content

Conversation

erizocosmico
Copy link
Contributor

@erizocosmico erizocosmico commented Aug 2, 2017

Depends on #214, just review changes from last commit.

Previous:

BenchmarkKallaxInsertWithRelationships-4      	     200	   5412829 ns/op	   21537 B/op	     481 allocs/op
BenchmarkKallaxInsert-4                       	     300	   4225965 ns/op	    3690 B/op	      88 allocs/op

Current:

BenchmarkKallaxInsertWithRelationships-4      	     200	   5734055 ns/op	   10220 B/op	     218 allocs/op
BenchmarkKallaxInsert-4                       	     500	   3914440 ns/op	    1185 B/op	      29 allocs/op

Allocs were cut in more than a half (almost a third without relationships) and it's slightly faster.

TL;DR: just by removing entirely the Squirrel query builder usage the memory usage was cut in half. We should move away from it as soon as possible, at least for inserts, updates and deletes which are fairly simple without it.
Queries might prove to be slightly more difficult but I still think it's worth the effort if we want to reduce in half the memory footprint of the framework

Note that it still uses the cached statements proxy from Squirrel. When we move away from it, we should either use it or make our own. It's the reason why our version, even being more complicated, it's slightly faster than plain database/sql inserts (without prepared statements).

@codecov
Copy link

codecov bot commented Aug 2, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@8bfec75). Click here to learn what that means.
The diff coverage is 94.73%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #216   +/-   ##
========================================
  Coverage          ?   79.2%           
========================================
  Files             ?      19           
  Lines             ?    3645           
  Branches          ?       0           
========================================
  Hits              ?    2887           
  Misses            ?     548           
  Partials          ?     210
Impacted Files Coverage Δ
store.go 87.83% <94.73%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bfec75...01969ea. Read the comment docs.

@Serabe
Copy link
Contributor

Serabe commented Aug 2, 2017

By moving away from squirrel for updates and deletes, you mean for specific record updates and deletes, not for batch ones, don't you?

Building queries can get really complicated really quick for non-trivial cases.

@erizocosmico
Copy link
Contributor Author

There are no batch updates/deletes.

Yes, querying can get ugly. But the only complex things we're doing are joins and counts. Not like we have to implement a full SQL builder. It's not like we're gonna do this now, but it would be nice for the future to move to a non-immutable query builder, because this one does a lot of allocs.

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
@erizocosmico erizocosmico merged commit 90dca92 into src-d:master Aug 3, 2017
@erizocosmico erizocosmico deleted the perf/insert branch August 3, 2017 13:37
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