[#36] allow changing almost everything in Config using System Properties

[nezda]

[#36] allow changing almost everything in Config using System Properties

• note: used underscores instead of periods used in the joni.debug* options
• had to tweak Search.java for the change to compile

[headius]

I love it! Will review and chat with other maintainers.

[headius]

Looks great overall! Just a few minor changes needed.

[headius]

Is this fixing a separate bug? If so we should move it to another PR because it's not config-related.

[nezda]

when I changed USE_SUNDAY_QUICK_SEARCH to be initialized from system property it no longer compiled

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project joni: Compilation failure: Compilation failure:
[ERROR] /Users/nezda/code/joni.git/src/org/joni/Search.java:[438,59] variable tlen1 might not have been initialized
[ERROR] /Users/nezda/code/joni.git/src/org/joni/Search.java:[446,59] variable tlen1 might not have been initialized

so I tried to fix.

[headius]

I pinged @enebo to look at this too.

[lopex]

yeah, it was constant propagated since those interface fields are essentially static finals

[lopex]

It will disable constant propagations for those flags, I dont have an opinion on it though

[nezda]

It will disable constant propagations for those flags, I don't have an opinion on it though

Seems like conservative thing to do is preserve whatever performance constant propagation gets us and just don't let USE_SUNDAY_QUICK_SEARCH be configurable (and revert my change to `Search.java). Doesn't preclude it being made configurable in the future.

[lopex]

Oh, I was referring to all flags, those affecting parser shouldnt matter though.
But for example this one - USE_CEC (combination explostion check) was never used and tested and is a part of very hot code - opFail().
@headius, @enebo some of the other flags are part of hot paths in stack manipulations and bytecode interpreter - do you think it will affect performance by much?

[nezda]

It is interesting an implicitly final variable initialized in that way stops being treated as a constant (but it seems clear something is going on related to that since code failed to compile without modification). I suppose final local variables could be introduce to ferry in values from those "constants" in that interface?

[lopex]

static finals where a direct translation for c #ifdef macros - I dont recall anything that could be a substitute for that in java.

[nezda]

I always thought final variables were basically equivalent to c #ifdef. I think this is my first counter-example.

I could introduce a Configuration object with all final fields initialized in a constructor which would have benefit different instances within same jvm could have different configurations.

[lopex]

There's at least two types of finals in java, static finals complete disapear from bytecode. So, dead branches will also make the bytecode smaller.

[headius]

Yeah, @lopex is right that this will prevent javac from eliminating code paths that were impossible to reach with literal static final values. That's why that sunday search code you patched suddenly started failing; with the original true value the uncompilable path was never followed and javac did not error.

I think the utility of making these configurable is worth any possible bytecode size increase here, even though it may tweak the inlining decisions of various JVMs. It should not generally affect performance when the same things inline as before, but some methods may cross size thresholds that prevent them from being trivially inlinable after this change.

If we could come up with a good way to audit which methods are affected by this, we could always move the conditional bodies out to separate static methods so they are small and don't impact the size of the original. There would be an additional inline that has to happen, so we'd have to weigh that against shrinking methods. We can make some determination by benchmarking expression matches that hit such methods heavily.

Personally, I would be surprised if this ends up being observable, and since there's no way to make these configurable without breaking the javac dead code optimization it may just be a price we have to accept (if it really has a cost).

[headius]

I would probably say we should just merge and then look at doing some benchmarking to find any performance regressions. We want these things to be configurable to make joni more flexible (to users other than JRuby also), so there may just be additional work necessary to find any slowdowns and deal with them.

[nezda]

For what it's worth, I concur with vote to merge this. I'd also be surprised if this had any significant negative performance consequence.

[enebo]

One followup to places where we do decide something is performance sensitive. We can split some of these things into separate types so we end up with more copies of some part of the code at the benefit of not having these ifdef sections. I also think this should merge and if possible we can see if we really see any perf differences.

[headius]

Merged! We are debating some other fixes for release but otherwise we'll push this out soon.

[headius]

Moved to 2.2.0 because we're also landing some API changes (and this deprecates and changes some API as well).