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.

Sign up for GitHub

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

Jump to bottom

Increase the amount of freezing #1233

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Increase the amount of freezing #1233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bbb45e
Add missing requires to CloverWeb
fdr Feb 12, 2024
5ff765c
Add missing autoloaded constant name
fdr Feb 12, 2024
09da56b
Expand and centralize class freezing
fdr Feb 12, 2024
93115cc
Initialize class variables to nil to be able to freeze classes
fdr Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 0 additions & 10 deletions clover.rb
View file
Open in desktop
Expand Up @@ -6,16 +6,6 @@
require "roda"

class Clover < Roda
def self.freeze
# :nocov:
unless Config.test?
Sequel::Model.freeze_descendents
DB.freeze
end
# :nocov:
super
end

route do |r|
r.on "api" do
r.run CloverApi
Expand Down
2 changes: 2 additions & 0 deletions clover_web.rb
View file
Open in desktop
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require "mail"
require "roda"
require "tilt"
require "tilt/erubi"

Expand Down
2 changes: 2 additions & 0 deletions lib/billing_rate.rb
View file
Open in desktop
Expand Up @@ -3,6 +3,8 @@
require "yaml"

class BillingRate
@@rates = nil

def self.rates
@@rates ||= YAML.load_file("config/billing_rates.yml")
end
Expand Down
2 changes: 2 additions & 0 deletions lib/github.rb
View file
Open in desktop
Expand Up @@ -30,6 +30,8 @@ def self.installation_client(installation_id)
client
end

@@runner_labels = nil

def self.runner_labels
@@runner_labels ||= YAML.load_file("config/github_runner_labels.yml").to_h { [_1["name"], _1] }
end
Expand Down
4 changes: 2 additions & 2 deletions loader.rb
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@
Unreloader.autoload("#{__dir__}/db.rb") { "DB" }
Unreloader.autoload("#{__dir__}/ubid.rb") { "UBID" }

AUTOLOAD_CONSTANTS = ["DB", "UBID"]
AUTOLOAD_CONSTANTS = ["DB", "UBID", "Clover"]

# Set up autoloads using Unreloader using a style much like Zeitwerk:
# directories are modules, file names are classes.
Expand Down Expand Up @@ -83,7 +83,7 @@
AUTOLOAD_CONSTANTS.freeze

if Config.production?
AUTOLOAD_CONSTANTS.each { Object.const_get(_1) }
AUTOLOAD_CONSTANTS.map { Object.const_get(_1) }.each(&:freeze)
Copy link
Member

@byucesoy byucesoy Feb 13, 2024 •
edited

Choose a reason for hiding this comment

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

Does this do 2 loops or is Ruby runtime smart enough to optimize it to single loop?

If it does 2 loops, you can do

AUTOLOAD_CONSTANTS.each { Object.const_get(_1).freeze }

to shave off few miliseconds.

Copy link
Collaborator Author

@fdr fdr Feb 19, 2024 •
edited

Choose a reason for hiding this comment

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

It does two passes, and that's what I want I think, IIRC AUTOLOAD_CONSTANTS.each { Object.const_get(_1).freeze } doesn't work, if future loaded constants patch ones previously frozen. I think one-passing it was the first thing I tried?

end

def clover_freeze
Expand Down
2 changes: 2 additions & 0 deletions model/page.rb
View file
Open in desktop
Expand Up @@ -16,6 +16,8 @@ def active
include ResourceMethods
semaphore :resolve

@@pagerduty_client = nil

def pagerduty_client
@@pagerduty_client ||= Pagerduty.build(integration_key: Config.pagerduty_key, api_version: 2)
end
Expand Down
2 changes: 2 additions & 0 deletions prog/postgres/postgres_resource_nexus.rb
View file
Open in desktop
Expand Up @@ -210,6 +210,8 @@ def create_certificate
).map(&:to_pem)
end

@@dns_zone = nil
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that this works. So existence of the variable is enough and .freeze does not freeze the value. It kinda defeats the purpose of freezing, but still better than what we have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's because the type may change but not the existence of the class variable. I actually wrote this as a prototype. Like you, I found it unsatisfying, because there is interesting dynamic behavior that could fail at run time while modifying the class. I'm not sure if I want to do go through with this hack.


def self.dns_zone
@@dns_zone ||= DnsZone[project_id: Config.postgres_service_project_id, name: Config.postgres_service_hostname]
end
Expand Down