⚗️ Perform experiments on gems
- Adds alt_memery to benchmarking
- Adds memoist3 to benchmarking
- Fixes #338
Before merging:
- [x] Copy the table printed at the end of the latest benchmark results into the
README.mdand update this PR - [x] If this change merits an update to
CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
f9f1825) to head (60e31d5). Report is 2 commits behind head on main.
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Fixed rubocop lint issues.
Thanks for making this PR, @pboling ! Since we now have multiple gems with the same class names, I'm wondering:
- Can we switch the table to display the gem name (e.g.
alt_memery) instead of the class name? I think that would be much more readable here. - Can we find a creative way to get around the limitation of running the same class names next to each other? I'd really like to reserve the 2.7 table for gems that don't support 3, not just for less-maintained ones. Some possible approaches would be renaming the constant (my preferred path) or loading the code a different way.
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 refinement which aliases the module namespace to a new constant, and then using that refinement in a block closure / context. I think it wouldn't work though, because within the gem all references would still be to the old namespace, and there would be live overloading of the namespace happening.
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.
- The namespaces will collide immediately when required by bundler, unless we add
require: false, and require them manually, which incurs complexity, maintenance, and overhead for little value. - We would have to rewrite the code of one of each of the colliding gems, in the same way that is already being done for the GitHub checkout of memo_wise. This could be made to work, but modifying an installed gem is fraught with problems, and the complexity is, IMO, simply not worth it for this script.
- A simpler solution would be to have two scripts, one running a "heat" of gem comparisons without any colliding namespaces, and the other running a different heat of gem comparisons with the gems left out of the first heat.
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.
@JacobEvelyn sorry about the delay. I have a gem called gem_bench, which was the perfect home for the copy & re-namespacing of a gem so that it can be benchmarked against itself, or a fork of itself, and it took me awhile to distill that and test it. All of that complexity (copy and re-namespace) is now black-boxed, and it still happens before the benchmarking part starts, so it won't affect results.
Looking forward to your feedback!
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!
@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!
@JacobEvelyn Rebased on latest main, squashed to single commit. Looks ready to me.
Thanks so much for all the work here @pboling !