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

vrp benchmark #2096

Merged
merged 7 commits into from
Aug 21, 2020
Merged

vrp benchmark #2096

merged 7 commits into from
Aug 21, 2020

Conversation

braktar
Copy link
Contributor

@braktar braktar commented Aug 19, 2020

As mentioned on the discussion forum, we are currently still using v0.18.0 as the performances for large models are, in the recent versions, greatly slowest.

This PR add a benchmark test with a large model.

With version v0.18, the input is treated in 13.934s

With version v1.4 and the default param builder : 88.520s
With version v1.4 and the hash param builder : 39.292s
With version v1.4 and hashie mash as param builder : 108.923s

@dblock
Copy link
Member

dblock commented Aug 19, 2020

Thanks for the benchmark. If you can, please post the associated profiler outputs in a gist or here.

I might take a look, but making no promises. This is an interesting problem to solve.

@braktar
Copy link
Contributor Author

braktar commented Aug 19, 2020

Here are the time to process the instance through multiple versions of grape

version time(s)
0.18.0 5,8
0.19.2 5,8
1.0.0 3,9
1.0.1 68
1.0.2 72
1.0.3 68
v1.1.0 73
v1.2.0 79
v1.2.1 81
v1.2.2 79
v1.2.3 82
v1.2.4 73
v1.2.5 90
v1.3.0 error
v1.3.1 error
v1.3.2 error
v1.3.3 87
v1.4.0 90

Moreover, here are the time profile for versions 0.18.0, 1.0.0, 1.0.1 and 1.4.0
https://gist.github.com/braktar/74a97be4a77a745262fb6bb739d5777a

@dblock
Copy link
Member

dblock commented Aug 19, 2020

I see a bisect between 1.0.0 and 1.0.1 as a next step!

@braktar
Copy link
Contributor Author

braktar commented Aug 19, 2020

I not really sure of the logic behind the meets_dependency? method. My assumption is that currently a param checks that all its child meet its own dependency, even before checking that it requires to really check it ?

Btw, changing the order makes the benchmark met a total time around 3 seconds

@dblock
Copy link
Member

dblock commented Aug 19, 2020

I not really sure of the logic behind the meets_dependency? method. My assumption is that currently a param checks that all its child meet its own dependency, even before checking that it requires to really check it ?

Btw, changing the order makes the benchmark met a total time around 3 seconds

I don't know much about that change, git blame and cc them here for comment?

@dblock
Copy link
Member

dblock commented Aug 19, 2020

The fact that all specs pass tells me that it's a fix. Try seeing which specs break if you remove the parent check, it should give us a hint on why that's there.

Update CHANGELOG, lets try to convince ourselves that it's correct and merge?

@braktar
Copy link
Contributor Author

braktar commented Aug 20, 2020

parameter @dependent_on is only active if the params or one of its parent belongs to a 'given' declaration.

Until now, we check that all the current params and their childs meets their dependency which first consist into verifying that one of the parent has the variable @dependent_on active.

I've added a spec to validate my assumption.

@rnubel as you've introduced the related change #1661 and #1625 of the meets_dependency? method. Is the logic correct ?

@dblock
Copy link
Member

dblock commented Aug 20, 2020

I merged #2097, care to rebase this please?

Can I please have another pair of eyes from @dm1try @dnesteryuk too?

@dblock
Copy link
Member

dblock commented Aug 20, 2020

and + @wanabe, probably coincidental but #2097 deals with dependencies

@braktar
Copy link
Contributor Author

braktar commented Aug 21, 2020

The branch is rebased and the conflict has been managed

I didn't find any interference in the grape behaviour with #2097

@dblock dblock merged commit 192a2a2 into ruby-grape:master Aug 21, 2020
@dblock
Copy link
Member

dblock commented Aug 21, 2020

This is great work @braktar, thank you. I merged it.

@braktar
Copy link
Contributor Author

braktar commented Aug 21, 2020

Thanks, that's a great step for us ! It divides by 3 the time to compute our current biggest instances.
Moreover, it will allow us to update grape-swagger and finally fix the doc ! 🎉

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

Successfully merging this pull request may close these issues.

2 participants