LDEV-5887 add LUCEE_PRECISE_MATH=disabled #2694
Conversation
| String str = SystemUtil.getSystemPropOrEnvVar("lucee.precise.math", null); | ||
| // If set to "disabled", ignore it here - ThreadLocalPageContext handles the global kill switch | ||
| if ("disabled".equalsIgnoreCase(str)) str = null; | ||
|
|
There was a problem hiding this comment.
why is this not simply a boolean we never use the word "disabled" for any flag, should simply be a boolean.
also there is no trim to remove whitespaces.
should be
Boolean b = Caster.toBoolean(SystemUtil.getSystemPropOrEnvVar("lucee.precise.math", null),null);
if(b!=null) {...}
else {str = ConfigFactoryImpl.getAttr(root, "preciseMath");...}
| */ | ||
| private static final String PRECISE_MATH_GLOBAL = SystemUtil.getSystemPropOrEnvVar( "lucee.precise.math", null ); | ||
| public static final boolean PRECISE_MATH_GLOBALLY_DISABLED = "disabled".equalsIgnoreCase( PRECISE_MATH_GLOBAL ); | ||
|
|
There was a problem hiding this comment.
same her, just one and a boolean and not "disabled", this is also the wrong place to do this. ThreadLocal classes should only be used for thread local related stuff.
|
|
||
| public static boolean isPreciseMathGloballyDisabled() { | ||
| return PRECISE_MATH_GLOBALLY_DISABLED; | ||
| } |
There was a problem hiding this comment.
this is a new pattern i really not wanna indroduce? having a getter "isWhateverGlobally" and "isWhatever" and specially not "Disabled" what inverse the logic
| public static boolean preciseMath(PageContext pc) { | ||
| // Global kill switch - when set to "disabled", completely bypass all precise math | ||
| if (PRECISE_MATH_GLOBALLY_DISABLED) return false; | ||
|
|
There was a problem hiding this comment.
with different name that is fine to keep
| // Check if precise math has been globally disabled | ||
| if (ThreadLocalPageContext.isPreciseMathGloballyDisabled() && preciseMath) { | ||
| throw new RuntimeException( "Precise math has been globally disabled via lucee.precise.math=disabled. Cannot enable it per-application." ); | ||
| } |
There was a problem hiding this comment.
i am not certain, but i have the feeling that a cfconfig setting could trigger this, that would mean that certain cfconfig do not work at all, if that enviroment setting is set. that should not happen. breaking a build because of a setting
There was a problem hiding this comment.
Didn't we change the configuration hierarchy to make env var/sys prop always override cfconfig?
https://luceeserver.atlassian.net/browse/LDEV-5642
it's off by default, do you mean breaking our CI build?
| "envvar": "LUCEE_PRECISE_MATH", | ||
| "desc": "A boolean value. If enabled, this improves the accuracy of floating-point calculations but makes them slightly slower", | ||
| "desc": "Controls precise math behavior. Values: true (enable BigDecimal for accuracy), false (use Double for performance), 'disabled' (global kill switch - completely bypass all precise math checks, throws error if applications try to enable it)", | ||
| "category": "performance", |
There was a problem hiding this comment.
when i am reading this is am still on the fence, we now have a setting you can not overwrite at all at runtime, in any way.
why do we have this for "precise math", but not for "local mode", i feel if we introduce this, we need to have a general pattern in mind for ALL settings and that needed to be implemented in a way, that does not need to add all this changes for a single setting.
IF we do this, we need a way to make this MUCH simpler, this adds a big layer of complexity to the configuration.
with switching to single mode i tried to simplify things, not only for developers, but also for us, this goes in the opposite direction.
All this is a good proof of concept, but we need to make a general plan for this.
i just did go through Lucee and i have made all env var setting overrule settings made in cfconfig. but now we add again different behaviour for different settings.
There was a problem hiding this comment.
I picked precise math here as it's a real hotspot and this static final approach allows the jvm to completely eliminate the runtime overhead
localmode didn't really show up in any jfr profiling
zspitzer
force-pushed
the
LDEV-5887-precise-disable
branch
from
d626a12 to
6e36dce
Compare
2 participants
https://luceeserver.atlassian.net/browse/LDEV-5887