Skip to content

Better usage of AMS::Model #56

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

Merged
merged 3 commits into from
Feb 6, 2018
Merged

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Feb 5, 2018

Nothing particularly intereting, just, if we're using AMS::Model,
might as well use it as designed.

This also seems to break two of the benchmarks on my machine, making AMS
not quite only 22-24x slower.

I'm not sure whether the benchmark is intended more for regression testing than for a goal.
I could also add benchmarks for the other two major AMS releases (:facepalm:).

Note: I am an AMS maintainer, though it isn't much being developed these days.

@shishirmk shishirmk changed the base branch from master to dev February 6, 2018 05:12
@shishirmk
Copy link
Collaborator

I changed the pull request to dev as there are many changes there that we havent merged into master yet.

The performance tests are mostly to catch performance regressions. Here is an example of how they help with design decisions. #49

@bf4 bf4 force-pushed the better_usage_of_ams_model branch from fdc8102 to 7da44a7 Compare February 6, 2018 06:23
@bf4
Copy link
Contributor Author

bf4 commented Feb 6, 2018

Thanks. Rebased. I also added a commit to correct the rubygems.org url. Wasn't sure if you'd want that as a separate PR.

@shishirmk
Copy link
Collaborator

Thank you for rebasing. Merging it in.

@shishirmk shishirmk merged commit fcca506 into Netflix:dev Feb 6, 2018
@bf4 bf4 deleted the better_usage_of_ams_model branch February 6, 2018 06:39
@bf4
Copy link
Contributor Author

bf4 commented Feb 6, 2018

wow, quick merge! Working late!

re:

The performance tests are mostly to catch performance regressions.

Well, in that case, AMS isn't under particularly active development, so I guess could be a stable benchmark. I've been interested in your benchmarking code, as I wrote some fancy stuff for AMS, but didn't end up using it much. I like how it's integrated and doesn't take too long.

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