Skip to content

Conversation

@byroot
Copy link
Member

@byroot byroot commented Jan 30, 2025

While upgrading the lobsters benchmark in yjit-bench I noticed 2.8% of the overall time spent in Dir[], all coming from FileUpdateChecker, which shouldn't be a thing in production.

After tracking it down, it comes from mission-control.

Profile: https://share.firefox.dev/4hBdygq
Ref: ruby/ruby-bench#358

NB: I'm really unfamiliar with the gem, so don't assume this PR is correct.

initializer "mission_control-jobs.importmap", after: "importmap" do |app|
MissionControl::Jobs.importmap.draw(root.join("config/importmap.rb"))
MissionControl::Jobs.importmap.cache_sweeper(watches: root.join("app/javascript"))
if app.config.reloading_enabled?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, great catch! This looks correct to me 👍 I see that in the original importmap-rails gem they use this condition instead:

if app.config.importmap.sweep_cache && !app.config.cache_classes

Maybe this one is better? At least for consistency 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      def reloading_enabled?
        enable_reloading
      end

      def enable_reloading
        !cache_classes
      end

So reloading_enabled? is !cache_classes but with a more readable name, so I think if anything we might as well change it on the import-rails side.

I'll add the sweep_cache condition though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nice, agree! 👍 👍

While upgrading the lobsters benchmark in yjit-bench I noticed
2.8% of the overall time spent in `Dir[]`, all coming from
FileUpdateChecker, which shouldn't be a think in production.

After tracking it down, it comes from mission-control.

Profile: https://share.firefox.dev/4hBdygq
Ref: ruby/ruby-bench#358
@byroot byroot force-pushed the stop-wathing-files-in-production branch from 9d998e3 to d2944e5 Compare January 30, 2025 08:42
@rosa rosa merged commit 67f7f6f into main Jan 30, 2025
6 checks passed
@rosa rosa deleted the stop-wathing-files-in-production branch January 30, 2025 08:46
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