Building needs is slow about fortitude HOT 14 CLOSED

Hmmm — this is definitely interesting. If you get a chance, one thing that would help a lot is additional debugging. Fortitude actually fires off ActiveSupport::Notifications.instrument with a name of fortitude.rebuilding when it rebuilds anything; it adds :what, :why, :originating_class, and :class to the payload. So something like this early in your startup sequence:

ActiveSupport::Notifications.subscribe("fortitude.rebuilding") do |name, start, finish, id, payload|
  $stderr.puts "Rebuilding #{payload[:what]} on #{payload[:class]} (started from #{payload[:originating_class]}) because #{payload[:why]} at\n    #{caller.join("\n    ")}"
end

…would print out a ton of information that would be really useful to me in figuring out why it's rebuilding things so often.

In the mean time, I'm going to look at seeing what it would be like to avoid this until the widget is actually used — you're right that that would be a whole lot faster in situations like yours.

from fortitude.

OK — I think I’ve got it licked. The branch ageweke/lazy_needs makes needs methods evaluation lazy, which I think ought to speed this up a great deal. (Right now, the ActiveSupport::Notifications lie about what’s going on, so trust the speed, not those.)

I’m going to fix up the Notifications stuff, add a spec for it, and then merge it to master, but, if you get a chance, let me know if this helps at all…

from fortitude.

Side note: are there any benchmarks we can point to here? Happy to share
mine but I would assume you have some, somewhere.

On Friday, November 14, 2014, Andrew Geweke [email protected]
wrote:

OK — I think I’ve got it licked. The branch ageweke/lazy_needs makes needs
methods evaluation lazy, which I think ought to speed this up a great deal.
(Right now, the ActiveSupport::Notifications lie about what’s going on,
so trust the speed, not those.)

I’m going to fix up the Notifications stuff, add a spec for it, and then
merge it to master, but, if you get a chance, let me know if this helps at
all…

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

Adam Becker
510.928.9111
@AdamJacobBecker

from fortitude.

@ageweke Thanks for the patch, it's definitely a lot faster but still feels pretty slow. I'll spend some more time investigating it. Want me to still run that active record subscribe snippet?

from fortitude.

Sorry for the delay — a marathon intervened. 🏃 🏃 🏃 😫

@leafo — Am I correct in assuming that the Asset Aggregator is only loading these widget classes, not instantiating any of them? The fix pushes needs compilation time to widget instantiation time, so, if for some reason AA is actually instantiating widgets, that would definitely cause some slowness.

I just pushed additional code to ageweke/lazy_needs that both fixes specs and fixes the ActiveSupport::Notifications support above. I'm going to take a look and if there's anything else I can find that might be causing slowness at class-load time, but, in the mean time, updating to the very head of that branch (if you don't, the ActiveSupport::Notifications stuff will actively lie to you) and using that code above to print out what's going on would in fact be really, really useful to me. I'm happy to just look at a giant dump of the entire output…

from fortitude.

@ajb — I do in fact have benchmarks; IMHO they're simultaneously great and terrible. Great because I put a ton of time into creating a benchmark that actually replicates the rendering of a real-world, extremely complex HTML page in several different view languages (ERb, HAML, Fortitude/Erector); terrible because it's super-janky code right now — perfectly reliable as a benchmark, but difficult to run unless you know exactly what you're doing. I intend to use my oop_rails_server gem to clean it up and make it really easy to run all kinds of benchmarks against many different implementations, but I'm not there yet.

If you have any benchmarks, I'd love to see them! Have you done much performance work on your branch of Erector vs. the "official" mainline? I've only run my benchmarks vs. the "official" mainline so far.

Also, this may be clear, but the stuff @leafo is talking about is solely about the speed of class loading. Asset Aggregator is a tool I developed when I worked at Scribd, where Leaf works (although I think it's been heavily modified since them); it loads all your widgets at boot time to grab CSS that it lets you put directly in the widgets. (In fact, a reimplementation of this idea — which I think is really cool — is exactly what my "other" project is about, the one I want to finish before writing Fortitude docs, exactly so that I can write the docs using them both.) So the performance here is all about just class-loading. If you have any benchmarks for that already, that would be fantastic; I may write one if I can't track down the issue he's having soon enough.

from fortitude.

BTW, I merged the above stuff to master right now since I have good tests for it and hasn't been giving me any issues. Let me know how it works for you.

from fortitude.

Other things I've found that could be causing slowness:

• If you are, for some reason, calling any of the configuration methods (like start_and_end_comments, enforce_id_uniqueness, etc.) after all the widgets get loaded. Calling one of these methods on a widget class causes it and all of its subclasses to rebuild relevant methods, which can get pretty expensive if there are lots of them. If you just do it in an initializer or someplace similar, it should be basically free, or very close.
• Seems unlikely, but you could try commenting out the Ruby hooks I have to detect localized methods being added (

  fortitude/lib/fortitude/widget/localization.rb

  Lines 10 to 26 in e05a6a6

  	# RUBY CALLBACK
  	def method_added(method_name)
  	super(method_name)
  	check_localized_methods!
  	end
  	# RUBY CALLBACK
  	def method_removed(method_name)
  	super(method_name)
  	check_localized_methods!
  	end
  	# RUBY CALL
  	def include(*args)
  	super(*args)
  	check_localized_methods!
  	end

  ) to see if for some reason those are making it really slow. (Fortitude lets you define a method called, e.g., localized_content_es, and if your locale is set to :es, it will run that method in place of content when you render that widget. This is, IMHO, a more-maintainable/elegant method of doing this than the way Rails/ERb does it, which basically means copying your view over wholesale for different locales, if you need something that the standard t/I18n support can't handle.)

Beyond that, it's not immediately clear to me what it could be. I would definitely enable that ActiveSupport::Notifications block I mentioned above and take a look at the output, or just send it to me. If even that doesn't help, I'll try building a benchmark that mimics what I think is going on as closely as I can. I'd also be really interested in seeing timings of Fortitude vs. Erector in this case, so I can get an idea what to compare it to.

Thanks again for the bug report!

from fortitude.

OK, from a conversation with @leafo — the culprit seems to be the localization-method detection stuff. Removing this cuts out ≥ 2/3 of the remaining overhead in starting things up.

I’m going to investigate ASAP (though it may have to wait until tomorrow) if there’s an easy way to make this extremely fast (as I don’t want it causing almost any overhead at all), or, if not, to simply make it a toggle-able option. It’s very much in the category of “nice to have”, not “have to have”, and would probably be used by very few people anyway — so no reason to have it causing slowdowns of any real amount for almost anybody.

from fortitude.

I just pushed to master the removal of the localized-content autodetection stuff — there's now a configuration option, use_localized_content_methods, that must be set instead if you want to use it. (Overhead of having it set should be very low, and much lower than the autodetection stuff was.)

I'm going to look and see if there are any obvious wins on making "needs" method building even faster, too…

from fortitude.

OK, I just pushed a very simple template-compilation system to the ageweke/compile_templates branch. It seems to speed up needs compilation by a further 2-3x, which I think should make your load times considerably faster than Erector at this point. Let me know what you see — assuming it passes tests and seems robust, I'll merge it to master. Now on to take a look at this other bug…

from fortitude.

Thanks,

The timer I mentioned that was recording (lazy) needs creation time now reports 0.02863666 seconds instead of 8.051985357 for a initial request

from fortitude.

Holy crap, that's awesome! I had no idea it would be that huge of an improvement. I'm delighted. I'll merge it to master shortly.

from fortitude.

OK, merged to master. Assuming no issues in CI, I plan to cut 0.0.9 tomorrow with this + all the other recent improvements.

from fortitude.