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

Fix the sqlboiler benchmark #269

Merged
merged 2 commits into from
May 28, 2018
Merged

Fix the sqlboiler benchmark #269

merged 2 commits into from
May 28, 2018

Conversation

aarondl
Copy link
Contributor

@aarondl aarondl commented May 28, 2018

Did not update the readme numbers since I don't have the same hardware and
don't have the other ORMs set up properly.

BenchmarkKallaxInsertWithRelationships-8      	    1000	   1536213 ns/op	   10991 B/op	     225 allocs/op
BenchmarkSQLBoilerInsertWithRelationships-8   	    1000	   1646870 ns/op	    7328 B/op	     215 allocs/op

This patch significantly speeds up the InsertWithRelationships benchmark. Typically transactions should be being used in calls like this. The API that SQLBoiler has makes it a concious choice when to use them, props to Kallax for having these happen automatically when appropriate.

Also fixes an error where the benchmarks no longer run (go vet now runs automatically as of Go 1.10).

Did not update the readme numbers since I don't have the same hardware and
don't have the other ORMs set up properly.
@roobre
Copy link
Contributor

roobre commented May 28, 2018

Hello,

Looks fine to me and I certainly can merge this right now, but benchmark results on README.md probably wouldn't get updated until the next release (which honestly can take a while). In order to be fair, I'm perfectly OK if you want to add a note to the benchmark results on README.md (like *This results are outdated as of 2018-05-28 and will get updated soon, or something along those lines).

- Add disclaimer about out of date benchmark
- Replace out of date benchmark with XXX
- Tune wording around sqlboiler's performance
@aarondl
Copy link
Contributor Author

aarondl commented May 28, 2018

Thanks @roobre. I did take the liberty! I hope the changes I've made are acceptable and thanks for being fair about it and allowing me to make the disclaimer.

It was a lot of fun profiling kallax and sqlboiler (did some flame graphs) and also seeing the different approaches we both took in the APIs.

I'd also like to share something I found interesting about the way kallax works too. If you look at it's use of the squirrel cache, that cache is never cleared. Prepared statements use memory on the database and the client and are meant to have Close() called on them when they're finished being used, and in Kallax this grows unbounded depending on how many queries you have in your app. I also found in my benchmarking that it barely affects performance at all, and is almost surely a detriment if you're only executing a query once or infrequently. Food for thought anyway.

Thanks :)

@roobre
Copy link
Contributor

roobre commented May 28, 2018

I hope the changes I've made are acceptable and thanks for being fair about it and allowing me to make the disclaimer.

They look indeed good to me. Of course, it's the least I can do :D

[...] that cache is never cleared. Prepared statements use memory on the database and the client and are meant to have Close() called on them when they're finished being used, and in Kallax this grows unbounded depending on how many queries you have in your app.

I was aware of memory being leaked (while investigating #263), but not about the exact place. Thanks for sharing your findings!

[Statement caching] barely affects performance at all, and is almost surely a detriment if you're only executing a query once or infrequently.

That's certainly something I did not know. As I only miantain kallax recently, I don't know why prepared statements where chosen on the first place. Perhaps for the free sql sanitizing? What I do know is squirrel was a temporary dependency, and kallax was meant to generate its own sql in the future. That plan was unfortunately abandoned, since the original authors shifted their efforts away from kallax.

A fix for that memory leak is something that indeed needs addressing. I will try to get some free time and actually work on a fix.

Anyway, I'm merging this to get the README updated. I'll hopefully rerun the benchmarks and get them a full refresh soon. Thanks for your effort, @aarondl!

@roobre roobre merged commit cf0ccd9 into src-d:master May 28, 2018
@aarondl
Copy link
Contributor Author

aarondl commented May 29, 2018

Thanks for the merge. A couple more notes on caching/performance/prepared statements:

I benchmarked 10,000 inserts with a for loop, prepared statements, and a transaction. They took 8.6s, 8.2s and 800ms respectively. I also have a hunch that the sql library itself uses prepared statements for every single query by default for the sql sanitizing, so creating one manually doesn't affect performance much. It seems a bit improbable so I just re-checked and those results are consistent.

There's a way to turn off the cache so you can test easily https://godoc.org/gopkg.in/src-d/go-kallax.v1#Store.DisableCacher (not sure if you know since you aren't one of the original authors) which affects this: https://github.com/src-d/go-kallax/blob/v1.3.4/store.go#L138

@aarondl aarondl deleted the fix-sqlboiler-benchmark branch May 29, 2018 01:28
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