-
Notifications
You must be signed in to change notification settings - Fork 25
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
⚗️ Perform experiments on gems #339
Conversation
55df637
to
aad755d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 190 190
Branches 90 90
=========================================
Hits 190 190 ☔ View full report in Codecov by Sentry. |
aad755d
to
8c5db56
Compare
Fixed rubocop lint issues. |
Thanks for making this PR, @pboling ! Since we now have multiple gems with the same class names, I'm wondering:
Does that make sense? |
Regarding 1, yes, that's an easy switch. Regarding 2, I wondered the same. My only idea was to add a But... if one of the two colliding gems doesn't have an internal references to its own namespace it could work... |
Did you see the first link I shared, to rename the constant? Wouldn't that be simple and also handle internal references? Or am I misunderstanding? |
Thinking more about this, and I don't think it is a good idea to spend time figuring out how to get two gems with the namespace to be loaded at the same time for a benchmarking run.
|
But thinking about it again... I'll just give it a try, and see if it works, the same way as is done for the MemoWise GH checkout. |
5699bd4
to
c51b0a4
Compare
@JacobEvelyn sorry about the delay. I have a gem called Looking forward to your feedback! |
# compare against the local version when we run benchmarks | ||
Dir.mktmpdir do |directory| | ||
Dir["#{github_memo_wise_path}/lib/**/*.rb"].each do |file| | ||
Tempfile.open([File.basename(file)[0..-4], ".rb"], directory) do |tempfile| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in my testing of the new copy & re-namespace feature of gem_bench
, the Tempfile
approach did not work with some libraries, because it makes the filename unique, and thus incompatible with later internal require "my_gem/my_file"
statements. As a result I had to switch to using File
, but still nested inside of Dir.mktmpdir
for automatic cleanup.
The copied and re-namespaced gems are loaded into memory, but only after the copy and re-namespace is complete, and then the source is deleted. This allows much more complex gems to be copied, re-namespaced, and loaded.
dc24407
to
2bb7a9c
Compare
README.md
Outdated
| `(a, b:)` | 19.17x | 16.03x | 16.93x | 15.57x | | ||
| `(a, *args)` | 2.91x | 2.08x | 2.90x | 1.81x | | ||
| `(a:, **kwargs)` | 2.61x | 2.20x | 2.61x | 2.18x | | ||
| `(a, *args, b:, **kwargs)` | 2.13x | 1.79x | 1.99x | 1.71x | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown tables are intended to look good and be readable in plaintext! My IDE (RubyMine) does this automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I'm totally happy to have these nicely formatted, but would you be willing to change our benchmarks code so that it spits out a formatted table like this? Happy to help if that doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will take me a while. My IDE does it for me automatically, so I have never needed code to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps to get this merged quicker I should revert to no spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
568f338
to
936ab3f
Compare
74c58bc
to
11cbd2a
Compare
|`(a, *args)`|0.67x|1.45x| | ||
|`(a:, **kwargs)`|0.68x|1.88x| | ||
|`(a, *args, b:, **kwargs)`|0.64x|1.29x| | ||
Results using Ruby 3.3.5: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once everything else in this PR is good, I'll send you the latest benchmark output from GitHub Actions to stick in here (in case you don't have access to see it yourself).
Just took a closer look—overall pretty good, just a few small things to tidy up. Thanks so much for working on this! |
@JacobEvelyn The CI build is awaiting approval! |
c99e066
to
b488bf9
Compare
@pboling final comments in; if you could make those README changes and then squash everything into one commit with a descriptive commit message this will be good to merge! |
1b58659
to
a79270c
Compare
@JacobEvelyn Rebased on latest main, squashed to single commit. Looks ready to me. |
- 📌 Ruby 3.3.5 for development - 📌 Ruby 3.3.5 for benchmarking - ♻️ Refactor re-namespacing of colliding gems - ➕ gem_bench v2.0.3
a79270c
to
60e31d5
Compare
Thanks so much for all the work here @pboling ! |
Before merging:
README.md
and update this PRCHANGELOG.md
, add an entry following Keep a Changelog guidelines with semantic versioning