Increase the amount of freezing #1233
Conversation
fdr
commented
Feb 12, 2024
•
fdr
commented
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
Searching "@@" in the codebase returns bunch of results. Should we do something about those? |
|
Yeah, I suppose we should, sadface |
Pre-patch, if you start `pry` and immediately try to load `CloverWeb` it will crash.
In the same vein as part of 651c3e7, some constants are handled specially in `loader.rb`. I missed the special constant loaded by the initial Unreloader instantiation.
Clover isn't intended to depend on class modifications after an initial load phase. There's no reason to restrict freezing to core classes or models, and using `loader.rb`'s existing complications to itemize classes under our control to freeze is broad and economical. Note there is a small change in semantics to freezing models: previously, there were only left un-frozen in test (i.e. they were frozen in `RACK_ENV=development`), now they are only frozen when `RACK_ENV=production`. Reported-by: Burak Yucesoy <2082070+byucesoy@users.noreply.github.com>
This works, but I wonder if there's a way to improve this with metaprogramming; it would be good if the lazy expressions in the "getters" that are cached were executed at load time, because, it would be annoying if they failed dynamically, at first call.
| @@ -210,6 +210,8 @@ def create_certificate | |||
| ).map(&:to_pem) | |||
| end | |||
|
|
|||
| @@dns_zone = nil | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
