Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Gemspec cache resurrected, avoid side effects #1829

Merged
merged 3 commits into from Apr 13, 2012

Conversation

dekellum
Copy link
Contributor

@dekellum dekellum commented Apr 9, 2012

The gemspec cache is per #1635, previously merged to master at 3d4163a, but suffering a compatibly setback with d24e788 merged from the 1.1 branch, and reverted in 6b45e28. This pull restores the feature and adds a fix for the spec failure.

@indirect
Copy link
Member

indirect commented Apr 9, 2012

Great, thanks! Do you happen to have benchmarks for the new version of the patch?

@dekellum
Copy link
Contributor Author

dekellum commented Apr 9, 2012

The impact of this cache is very dependent on the what is computed in each gemspec and how many gemspecs the project has. What I've observed without the cache on master is that each gemspec is otherwise loaded (eval) twice as part of bundler/setup. I have a 17 gemspec project with a top level Gemfile. Without the cache, the second load of each gemspec is taking ~30ms per gem real time, for a total of ~500ms. This is time removed with the cache, and is perceptively worth while for my case given bundler/setup is required for each test run.

Obviously the gain will be much more incremental on a single gemspec project assuming low constant time for eval of the gemspec. But the change is pretty minimal and possibly more "correct": load each gemspec once? There have only been a few spec compatibility issues found when merging new specs from the 1-1 branch to master and no observed problems with actual bundler usage (I've been using the patch for a while with above referenced project and everything else in my dev environments.)

Can you accommodate this on second review?

indirect pushed a commit that referenced this pull request Apr 13, 2012
Gemspec cache resurrected, avoid side effects
@indirect indirect merged commit 51424a7 into rubygems:master Apr 13, 2012
@indirect
Copy link
Member

Thanks for the fix, and I have merged now that the tests all pass.

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

Successfully merging this pull request may close these issues.

None yet

2 participants