Modify Graal engine creation to allow dynamic creation#20810
Conversation
…e visibility, cache the Language instance. Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
Nadahar
added
the
additional testing preferred
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Nadahar
changed the title
|
@Nadahar will test it during the next days. |
|
I have compiled patched pythonscripting for openhab 5.1 and tested it in my production setup, which includes 141 file based rules. So far, everything appears to be working. What I have not tested is openhab 5.2, or whether jsscripting and pythonscripting interfere with each other in any way. |
|
I don't think there's any difference between 5.1 and 5.2 in this regard, I can't think of anything relevant that has changed lately.. what would be interesting is if you see what happens to your rules if you uninstall and reinstall the add-on without rebooting. |
|
Also, to make the test "valid", you should start OH with |
This worked before, as I only use pythonscripting on its own. I always delete the old kar file in the addons folder, which triggers an uninstall. Then I place the new kar file in the addons folder, which again triggers an install. A proper test would be:
this was exactly the scenario which was not working before. |
|
Ok, you have spent more time fighting this than I have. But, the reason uninstalling and reinstalling has worked in the past is actually a bug, because the |
totally agree ;-) I just expressed myself imprecisely. I'm certainly happy that things are getting better now, too. |
|
now, pythonscripting is running with "-Dtruffle.engine.DisableLanguageCache=true" as EXTRA_JAVA_OPTS, provided to my podman container without any problems. |
|
I tried to test the final scenario, but without success. First Time, when I activated the second addon. I got and then I recompiled it again with 'logger.info(">>>> TEST {}", System.getProperty("truffle.engine.DisableLanguageCache"));` to check if the property is active. (It was active) and I got After a restart, both engines are working. |
|
I'm not sure how to evaluate the "KAR test" above, there are too many unknowns involved with KAR files as I see it. It isn't even straight forward to build a "correct" KAR, and even if you do, I don't think we know which feature definitions will be used if the one in core and the one in the KAR are different..? At least one of the errors seem to be related to something unrelated to this issue, This does solve running both JS and Python from Eclipse. I doubt that's the only thing it solves, even if there are other things that don't work out there. The situation in Eclipse is this: When I try my customized Graal setup for Eclipse on a Linux VM, both work. When I try this on Windows, Python fails. The difference seems to be that of timing. Because of the static cache, for both to work, they must both be "initialized" in somewhat the same timeframe, so that by the time the first Graal based add-ons initialize, both "engines" are registered in Graal. Python has a lot of files/libs that must be unpacked, and since Windows use file locking, large number of operations on small files are typically quite a bit slower (every access must first check the "lock database"). This leads to Python not being ready in time for JS to "initialize", thus locking Python out of the static cache. I very much doubt this timing issue is unique to Eclipse, it's most likely a general problem, but it's difficult to control these timings to absolutely "prove it". This change (together with setting the system property in core) solves this whole timing issue, so that both engines are available/visible even if they initialize at different speeds. I therefor think that unless something "gets worse" by doing this, it should be merged, even if it doesn't solve all related problems. |
|
It should be mentioned that these changes (that I've had to apply locally) are what enabled me to look into openhab/openhab-core#5635 and #20853. I think it's very useful to be able to debug the Graal add-ons, so for that alone, I think this has value. I can't quite understand how that wouldn't have value for others as well..? |
|
@HolgerHees it looks like you where unable to test these changes. Need any help, maybe @Nadahar is able to assist here? |
|
I don't think we need this in for 5.2.0, if the main benefit is when developing. In that case, it might be better to merge it soon after 5.2.0 is released, so that we have lots of time to discover any potential side effects. I just don't want it to die/become forgotten, since it solves something that have at least been bothering me for a very long time. I strongly suspect that it will also have some benefit for users when installing the add-ons, even if it doesn't solve all problems. So, let's let this one wait for now. |
4 participants
This alters the classloader used during engine registration and
Languageretrieval, and caches theLanguageinstance. It must be seen together with openhab/openhab-core#5614, and the goal is to allow dynamic registration of Graal engines.A description of both changes is given here, and this should allow installing and uninstalling Graal based add-ons without having to restart OH. It should also prevent the risk of one of them being unavailable if the timing of their creation during startup is unfortunate.
It needs proper testing to make sure that creating these using the alternative classloader don't have other side effects, like failing to see resources that it previously could. I'm ill-equipped to do this testing, as I don't use any of these languages myself, and lack both "source rules" and the knowledge of how to properly test it.