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

Reduce loading time #62

Merged
merged 2 commits into from
Apr 7, 2015
Merged

Reduce loading time #62

merged 2 commits into from
Apr 7, 2015

Conversation

plribeiro3000
Copy link
Member

This is related to fog/fog#3430

@geemus After starting this refactor i think this wont make a great difference since we cannot specify which file to load from gemspec as we can do from Gemfile.

Also, how do you think we should handle the relative requires you mentioned? We can't use require_relative yet since we still support 1.8.7. =s

@plribeiro3000 plribeiro3000 changed the title Add fog-aws to reduce load time Add fog-aws file to reduce load time Feb 25, 2015
@geemus
Copy link
Member

geemus commented Feb 25, 2015

We should be able to do something based off File.dirname(__FILE__) to do it relatively still, I think?

I don't think the gemspec will need changing, so much as changing it in fog/lib/fog.rb if I'm not totally mistaken. Does that make more sense?

@plribeiro3000
Copy link
Member Author

If we don't need to change the gemspec whats the idea behind your first point here?

@geemus
Copy link
Member

geemus commented Mar 10, 2015

@plribeiro3000 so, first off, I'm hoping I'm not totally mistaken here. But I believe when you require a gem it includes that gems lib in to the search path. We now have a TON of gems that have lib/fog in them. So when you require something like lib/fog/foo, it might have to look through ALL the fog gems to find it. By instead using relative requires, it avoids the search path issue. Does that make better sense/help? I may just misunderstand this mechanism also, but I think it works something like that.

@plribeiro3000
Copy link
Member Author

@geemus It makes sense but i think it won't help much if we don't explicity say it to load this new files.

I know we are doing this in the lib/fog/fog.rb but when including a gem dependency it requires the file with the same name of the gem. So the require for fog-atmos here can be changed but the original file will still be required automatically.

Is that correct? What do you think?

@geemus
Copy link
Member

geemus commented Mar 12, 2015

Hmm. I guess those will have to remain, but if they just browse the path once (and then inside each sub-gem it does things relatively) I think it might help? in /lib/fog/fog.rb we could also consider changing to relative requires for the stuff that isn't yet extracted. I guess we really should pick an example, try it and benchmark, to get a better sense of whether this is even the right path or a false lead.

@geemus
Copy link
Member

geemus commented Mar 13, 2015

Actually, upon further consideration... I wonder if the better/more proper fix wouldn't be to make it so that these, broken-out gems were changed so that instead of having lib/fog they had lib/fog-#{provider} as the main directory, matching their gem name. This actually is more inline with expectations around gem naming/files and should accomplish the same thing in a simpler/more sensible way. What do you think?

@plribeiro3000
Copy link
Member Author

@geemus I think it makes sense. But i think bundler would not work out of the box. The user will always have to require the gem but it might me a small change to improve everything.

I will try to get this done asap.

@geemus
Copy link
Member

geemus commented Mar 13, 2015

Will it break bundler?

@hone - halp? Am I breaking the world by doing this? And/or could my suspicions be valid?

@plribeiro3000
Copy link
Member Author

ok @geemus. I'm using relative paths now. Don't merge this yet. I'm gonna create a branch in all providers doing that and point to those branchs from my fork of fog and we can see if it will help at all. =)

Just give me a few minutes please!

@geemus
Copy link
Member

geemus commented Mar 23, 2015

@plribeiro3000 cool, glad that it is clearer now. I reached out to some other people to see if they thought this might help, but I guess if we try it maybe we can see more clearly. Probably a good thing to do regardless.

@geemus
Copy link
Member

geemus commented Mar 23, 2015

I wonder if the sort of mv lib/fog/aws lib/fog-aws part will still be needed also though...

@plribeiro3000
Copy link
Member Author

@geemus Me too. But as i believe this will be more trickier i will try to see the result of the relative requires first. I plan to have a branch based version of fog in my fork using relative requires everywhere next weekend. We shall see the results.

@geemus
Copy link
Member

geemus commented Mar 23, 2015

Cool, I reached out to one of the bundler guys also to see if he thinks the problem I described might actually be the culprit or just a dead end. So hopefully between your experiment and their thoughts we can at least figure out if this is worth pursuing further.

@plribeiro3000
Copy link
Member Author

@geemus @fcheung

This is a huge diff but sounds like it did the trick. This is a start point so we can discuss about it and find the best approach to tackle it.

The changes are:

  • Use autoload so extra files only get loaded when needed
  • Use relative paths
  • Move the logic in the fog/softlayer/core file to the fog/softlayer since this is the only one bundler will load automatically.
  • Remove some unnecessary requires in the models
  • Move some files so the path will match the namespace

@plribeiro3000
Copy link
Member Author

@lanej Your feedback is important as well. Sorry if i missed you in the comment above. 😓

@plribeiro3000 plribeiro3000 changed the title Add fog-aws file to reduce load time Reduce loading time Apr 6, 2015
@lanej
Copy link
Member

lanej commented Apr 6, 2015

@plribeiro3000 no worries.

I love it as it lies.

Only possible issue is what ruby is doing with autoload. https://www.ruby-forum.com/topic/3036681. I've been under the impression that it will disappear, but it's still there and I haven't found any more recent commentary on the subject.

@plribeiro3000
Copy link
Member Author

@lanej Sure thing. @tokengeek has brought this to my attention as well sometime ago.

But i could not find another way to fix this issue. IMO the issue lies in the amount of files it has to load for every provider. fog has a lazy loading mechanism so its safe while the provider is there. But once we extract it we will start hitting this issue.

Perhaps you have another idea?

@lanej
Copy link
Member

lanej commented Apr 7, 2015

@plribeiro3000 beyond doing our own thread-safe const_missing hack (:puke:). not really. i say we ride autoload till the end. just wanted to make sure we all knew about the baggage.

@plribeiro3000
Copy link
Member Author

@lanej Yeah. I think that in the end we will end up doing some fancy lazy loading solution if autoload dies.

@lanej
Copy link
Member

lanej commented Apr 7, 2015

@plribeiro3000 hopefully by that point, mri conjures up a respectable substitute. I am in favor of merging this as it sits.

@plribeiro3000
Copy link
Member Author

@lanej awesome. Let me know when you merge and release. =)

lanej added a commit that referenced this pull request Apr 7, 2015
@lanej lanej merged commit 5903a78 into fog:master Apr 7, 2015
@geemus
Copy link
Member

geemus commented Apr 7, 2015

Thanks!

On Tue, Apr 7, 2015 at 3:08 PM, Josh Lane notifications@github.com wrote:

Merged #62 #62.

Reply to this email directly or view it on GitHub
#62 (comment).

@plribeiro3000 plribeiro3000 deleted the fix-loading-time branch April 7, 2015 20:42
@lanej
Copy link
Member

lanej commented Apr 7, 2015

@plribeiro3000
Copy link
Member Author

Awesome. Thanks!

@geemus
Copy link
Member

geemus commented Apr 8, 2015

Thanks!

On Tue, Apr 7, 2015 at 5:04 PM, Paulo Henrique Lopes Ribeiro <
notifications@github.com> wrote:

Awesome. Thanks!

Reply to this email directly or view it on GitHub
#62 (comment).

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.

3 participants