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

Add pg gem scope_all benchmark #76

Merged
merged 1 commit into from
Jul 26, 2017

Conversation

bmarkons
Copy link
Contributor

@bmarkons bmarkons commented May 14, 2017

Add pg suite with pg/postgres_scope_all benchmark.

@bmarkons bmarkons force-pushed the add-bm-raw-scope-all branch 3 times, most recently from c9f4cfd to 5c64a99 Compare May 15, 2017 10:39
@bmarkons bmarkons changed the title Add raw scope_all benchmark Add raw scope_all benchmark [wip] May 15, 2017
@bmarkons bmarkons force-pushed the add-bm-raw-scope-all branch 2 times, most recently from 5175d96 to e2025e7 Compare May 15, 2017 21:05
@bmarkons bmarkons changed the title Add raw scope_all benchmark [wip] Add raw scope_all benchmark May 16, 2017
@bmarkons bmarkons force-pushed the add-bm-raw-scope-all branch 4 times, most recently from 5b94f74 to 24b639d Compare May 17, 2017 14:27
@bmarkons bmarkons changed the title Add raw scope_all benchmark Add pg gem scope_all benchmark May 17, 2017

Benchmark.pg("raw_pg/postgres_scope_all", time: 5) do
str = ""
conn.exec("SELECT * FROM users").each_row{ |row| str << "name: #{row[0]} email: #{row[1]}\n" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interestingly this can be done a fraction quicker if you avoid the #each_row and instead just use a double while loop iteration cause you can avoid a bunch of block transitions, its very very minor

Copy link
Contributor Author

@bmarkons bmarkons May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamSaffron interesing note 🤔 I guess I should leave it as it is because I used iterating blocks in other scope_all benchmarks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it is a distraction, we should focus on getting that graph up on ruby bench, 3 lines on one benchmark

@SamSaffron
Copy link
Member

@tgxworld can we merge this?

require 'pg'
require_relative 'support/benchmark_pg'

# Connect to POSTGRES database with provided values or use default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll prefer if we leave out the comment here. I believe the code is self explanatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgxworld yeah, It is. I was kind of in doubt because of defaults. I'll leave it out 👍

require_relative 'support/benchmark_pg'

# Connect to POSTGRES database with provided values or use default
conn = PG.connect(host: ENV.fetch("HOST", "localhost"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer

conn = PG.connect(
  host: ENV.fetch("HOST", "localhost"),
  port: ..
)

The problem I have with flushing indentations is that changing anything in the first line forces us to re-indent all the lines polluting the commit history.

@bmarkons
Copy link
Contributor Author

bmarkons commented May 23, 2017

@tgxworld, fixed stuff you've suggested :)

@robin850
Copy link
Member

As Noah mentioned in #79, there's a tiny schema inconsistency. Is that intentional that the admin, created_at and updated_at attributes aren't present in the PostgreSQL schema ? That would certainly be better to make sure that results can be compared apples to apples.

Looks great though ! 👍

@bmarkons
Copy link
Contributor Author

Nice catch! I omitted created_at and updated_at by mistake.

Although none of scope_all benchmarks has admin field in schema. Admin field is present only in default_scope benchmarks.

I am not sure if scope_all benchmarks are about to be compared with default_scope benchmarks, these two are of different type. I had only in mind comparing scope_all (or default_scope) across AR, sequel or pg implementations.

@robin850
Copy link
Member

Yeah, you're right Marko ! That looks great then !

@bmarkons
Copy link
Contributor Author

bmarkons commented Jul 10, 2017

@tgxworld
Do we want setup on RubyBench for pg and mysql gems same as for ActiveRecord and Sequel?
If yes, I think this one can be merged so I can continue with adding other benchmarks..

The next step would be to make and push docker image for pg suite.

@bmarkons bmarkons merged commit 4e62098 into ruby-bench:master Jul 26, 2017
@bmarkons bmarkons deleted the add-bm-raw-scope-all branch July 26, 2017 07:21
@tgxworld
Copy link
Member

@bmarkons Sorry for missing this. I guess we should enable this since the benchmarks have been added :)

@bmarkons
Copy link
Contributor Author

bmarkons commented Jul 27, 2017

Sure, I am writing driver for pg suite right now and Dockerfile.. :)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants