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

Use common setup for Sequel and Rails benchmarks #104

Merged
merged 4 commits into from Aug 29, 2017

Conversation

Projects
None yet
3 participants
@bmarkons
Copy link
Collaborator

bmarkons commented Aug 27, 2017

The idea is to move setup from benchmark scripts to shared setup scripts in support/setup.

@bmarkons bmarkons force-pushed the make-common-benchmark-setup branch from 5939bad to 6ed2191 Aug 27, 2017

@bmarkons bmarkons force-pushed the make-common-benchmark-setup branch 3 times, most recently from 74f3590 to 88ee24e Aug 27, 2017

@bmarkons bmarkons force-pushed the make-common-benchmark-setup branch from 88ee24e to d3d0813 Aug 28, 2017

class User < ActiveRecord::Base; end

attributes = {}

COUNT.times do |i|
25.times do |i|

This comment has been minimized.

@robin850

robin850 Aug 28, 2017

Member

It's a bit late to ask about this since you wrote the db_setup two months ago but why a require_relative or something like that wouldn't work ? This change makes me think that this is less than ideal to spawn a shell process to execute the setup script ; you can't share data between the setup script and the actual benchmark script. If COUNT changes in the future for whatever reason, this value may not be properly updated.

This comment has been minimized.

@bmarkons

bmarkons Aug 28, 2017

Collaborator

Maybe I could move COUNT constant to support/helper and use it from there?

For some reason I didn't want to mess Sequel code with Active Record, since setup scripts are written using Active Record. But there is probably nothing I could lose with that approach. Though It felt safer to do it in separate process. Not quite sure.

I had some time thinking what would be the best way to organize our bench suite. Probably having them as gems would be useful. That way I could simply require setup gem and call appropriate setup method. Also I think we need to have tests for our benchmarks, to make our self sure our benchmarks are correct.

This comment has been minimized.

@robin850

robin850 Aug 29, 2017

Member

Though It felt safer to do it in separate process.

Since the code was directly present in the different files, why would it feel safer for you ?

Probably having them as gems would be useful. That way I could simply require setup gem and call appropriate setup method.

You could simulate this directly with a require_relative or pushing the setup folder to the load paths array and using require. I may not see the direct benefits but with gems, you would have to cut a release each time you want changes to be propagated directly on the site. This may add some useless overhead.

This comment has been minimized.

@bmarkons

bmarkons Aug 29, 2017

Collaborator

Yeah, you're probably right. There is no specific need to run setup in spawned shell process. We only need to require these setup files in benchmarks. I am looking forward to fix this 👍

Primarily I wanted to organize them them in gems to achieve loading to path with gemspec. I would require these gems from path without cuting releases. Do you still think this is not a good idea?

This comment has been minimized.

@robin850

robin850 Aug 30, 2017

Member

I would require these gems from path without cuting releases.

I guess there's no point to "gemify" them then. Things can be done more easily with require_relative since these files are in the same repository as benchmarks. What do you think ? Tell me if I'm missing something obvious. 😅

This comment has been minimized.

@bmarkons

bmarkons Aug 30, 2017

Collaborator

I am the one confused here 😆 -- I thought convenience of loading setup scripts into path and having test suite for our scripts comes easily with making gems. So you are saying I can simply achieve it with require_relative setup?

Also I really want to make test suite for our scripts -- I think it is necessary, what do you think?

It looks like I will probably need your help with this -- it's the next thing I want to work on.

Thanks 🙇

This comment has been minimized.

@robin850

robin850 Sep 4, 2017

Member

Sorry for the late answer, I've been pretty busy recently ! 😊

So you are saying I can simply achieve it with require_relative setup?

Yeah, actually, in a nutshell, require loads Ruby files and look into the load paths to find the given files (you can inspect them through $: ; by default, it's basically just the paths where gems can be loaded). This allows you to give a gem name to require rather than giving an absolute path.

require_relative loads Ruby files as well but assumes that the given path is relative to the current file's location.

Gems are handy for sharing and redistribution but since the setup scripts are in the same repository, passing through RubyGems will only add overhead without any benefit ; you can directly use require_relative to load them from benchmark scripts.

Also I really want to make test suite for our scripts -- I think it is necessary, what do you think?

Yes, that would be cool to make sure that benchmarks that are comparable are actually doing the same work but to be honest, I'm not sure how that could be achieved. For Active Record and Sequel for example, you could make sure that the generated string is the same for both but I don't know if it would be possible to spot discrepancies like this.

Feel free to tell me if something isn't clear or if I'm missing something / misunderstanding you. :-)

@bmarkons bmarkons force-pushed the make-common-benchmark-setup branch from e475c7f to 5ebb001 Aug 28, 2017

@bmarkons bmarkons changed the title Use common setup for Sequel and Rails benchmarks [WIP] Use common setup for Sequel and Rails benchmarks Aug 28, 2017

@bmarkons bmarkons merged commit af2f7c5 into master Aug 29, 2017

@bmarkons bmarkons deleted the make-common-benchmark-setup branch Aug 29, 2017

@tgxworld

This comment has been minimized.

Copy link
Member

tgxworld commented on d3d0813 Sep 5, 2017

Ah I think it is fine to duplicated all of these. The reason why I would prefer the scripts to be verbose is because the full script is displayed on the rubybench.org when we view a benchmark. It is hard for someone else to figure out what bm_save_setup does when viewing on rubybench.org.

This comment has been minimized.

Copy link
Collaborator

bmarkons replied Sep 5, 2017

With this change, I wanted to make sure we are doing the same setup for Sequel and Active Record benchmarks, to make correct and fair comparison.

We could display both setup script used and benchmark script one next to another? Having separated setup is much more error prone, since one is written using Sequel while the other one using Active Record.

I understand it's important to have setup displayed but we simply need to make UI follow this change.
Sam also recommended this change here in #88 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment